Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

apply EXIF rotation information if libgd is linked with libexif #478

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

Tee0125
Copy link

@Tee0125 Tee0125 commented Oct 4, 2018

orientation fix.

@cmb69
Copy link
Contributor

cmb69 commented Oct 4, 2018

Thanks! That would solve #341. Just a few quick remarks:

  • if we apply auto-rotation/flipping, there should be a way to detect whether this happens; a constant may be sufficient
  • there should be some tests
  • the CMake build chain should also support this
  • we already have gdImageFlipHorizontal|Vertical|Both, so no need to duplicate the functionality
  • if we add functions combining rotation and flipping, these probably should be exposed
  • the added rotation/flipping functions always create truecolor images, so copying the palette appears to be superflous

@Tee0125
Copy link
Author

Tee0125 commented Oct 5, 2018

gdImageFlipHorizontal|Vertical|Both changes source image itself, but rotating 90/270 degree can not be implemented like that because image size need to be changed.

So for the consistency, I made a new functions for flipH/V which allocate a new image and copying pixels with horizontal/vertical flipping.

How about changing name of add functions to gdImageCloneRotate90/180/270/FlipH/FlipHRotate90/... , and move to gd.c? (Also applying palette copy, ... to renamed functions)

Copy link
Member

@vapier vapier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all of the rotate functions do the same thing except for the gdImageCreateTrueColor and gdImageSetPixel calls. imo we should create one internal static transformation function which all the others build off of with a callback to do the actual reindexing.

untested, but should show what i mean. hopefully the overhead of the callbacks isn't significant perf wise. it does make the code a lot easier to maintain imo.

typedef int (*rotateHelper)(gdImagePtr dst, int x, int y);

static gdImagePtr gdImageRotate(gdImagePtr src, int ignoretransparent, int newWidth, int newHeight, rotateHelper transX, rotateHelper transY)
{
...
    dst = gdImageCreateTrueColor(newWidth, newHeight);
...
        for (uY = 0; uY<src->sy; uY++) {
            int newY = transY(uY);
            for (uX = 0; uX<src->sx; uX++) {
                int newX = transX(uX);
...
                if (ignoretransparent && c == dst->transparent) {
                    gdImageSetPixel(dst, newX, newY, dst->transparent);
                } else {
                    gdImageSetPixel(dst, newX, newY, c);
                }
...
static int rot90x(gdImagePtr dst, int x, int y)
{
    return y;
}
static int rot90y(gdImagePtr dst, int x, int y)
{
    return dst->sy - x - 1;
}
gdImagePtr gdImageRotate90(gdImagePtr src, int ignoretransparent)
{
    return gdImageRotate(src, ignoretransparent, src->sy, src->sx, rot90x, rot90y);
}

src/gd_jpeg.c Outdated Show resolved Hide resolved
src/gd_jpeg.c Outdated Show resolved Hide resolved
src/gd_jpeg.c Outdated Show resolved Hide resolved
src/gd_jpeg.c Outdated Show resolved Hide resolved
src/gd_jpeg.c Outdated Show resolved Hide resolved
src/gd_rotate.c Outdated Show resolved Hide resolved
src/gd_rotate.c Outdated Show resolved Hide resolved
src/gd_rotate.c Outdated Show resolved Hide resolved
src/gd_rotate.c Outdated Show resolved Hide resolved
src/gd_rotate.c Show resolved Hide resolved
@Tee0125
Copy link
Author

Tee0125 commented Feb 15, 2019

@vapier : code is updated as you requested. please review again

@pierrejoye pierrejoye mentioned this pull request Aug 26, 2021
@pierrejoye pierrejoye added this to the GD 2.5 milestone Aug 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants