Bug 100179 – Honor image orientation in GraphicsContextSkia

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Nico Weber]

Honor image orientation in GraphicsContextSkia

[Nico Weber]

Created attachment 170287 [details]
Patch

[WebKit Review Bot]

Attachment 170287 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/graphics/skia/ImageSkia.cpp:592:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nico Weber]

+timothy_horton for the CG change (see changelog)

[Nico Weber]

Created attachment 170295 [details]
Patch

[Nico Weber]

+senorblanco for skia review

+eseidel, +noel.gordon fyi

[Stephen White]

Comment on attachment 170295 [details]
Patch

I don't grok all of the special cases, but the skia stuff looks ok.

[Stephen White]

Comment on attachment 170295 [details]
Patch

r=me

[WebKit Review Bot]

Comment on attachment 170295 [details]
Patch

Clearing flags on attachment: 170295

Committed r132384: <http://trac.webkit.org/changeset/132384>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Nico Weber]

cc'd people: It looks like this isn't quite correct for inline <img> tags. It works for image documents (which I had checked in all orientations in Safari manually ), and tests passed with the apple port -- I forgot they don't run pixel tests by default. I'm looking at the regression now.

[Nico Weber]

Fix in bug 100319

[Eric Seidel (no email)]

Comment on attachment 170295 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=170295&action=review

> Source/WebCore/platform/graphics/ImageOrientation.h:45
> +    OriginRightTop = 8, // 270 degree CW rotation

The spec says 8 is Left/Bottom.  Why the change?

8 = The 0th row is the visual left-hand side of the image, and the 0th column is the visual bottom.

[Nico Weber]

Comment on attachment 170295 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=170295&action=review

>> Source/WebCore/platform/graphics/ImageOrientation.h:45
>> +    OriginRightTop = 8, // 270 degree CW rotation
> 
> The spec says 8 is Left/Bottom.  Why the change?
> 
> 8 = The 0th row is the visual left-hand side of the image, and the 0th column is the visual bottom.

Right, "The 0th row is the visual left-hand side of the image, and the 0th column is the visual bottom." describes a 90 degree rotation to the left, i.e. a 270 degree clockwise rotation. (The 0th row is the topmost row.)

[Eric Seidel (no email)]

Comment on attachment 170295 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=170295&action=review

>>> Source/WebCore/platform/graphics/ImageOrientation.h:45
>>> +    OriginRightTop = 8, // 270 degree CW rotation
>> 
>> The spec says 8 is Left/Bottom.  Why the change?
>> 
>> 8 = The 0th row is the visual left-hand side of the image, and the 0th column is the visual bottom.
> 
> Right, "The 0th row is the visual left-hand side of the image, and the 0th column is the visual bottom." describes a 90 degree rotation to the left, i.e. a 270 degree clockwise rotation. (The 0th row is the topmost row.)

Sure, I don't disagree on your math.  It's the enum name change which concerns me?

[Eric Seidel (no email)]

Comment on attachment 170295 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=170295&action=review

>>>> Source/WebCore/platform/graphics/ImageOrientation.h:45
>>>> +    OriginRightTop = 8, // 270 degree CW rotation
>>> 
>>> The spec says 8 is Left/Bottom.  Why the change?
>>> 
>>> 8 = The 0th row is the visual left-hand side of the image, and the 0th column is the visual bottom.
>> 
>> Right, "The 0th row is the visual left-hand side of the image, and the 0th column is the visual bottom." describes a 90 degree rotation to the left, i.e. a 270 degree clockwise rotation. (The 0th row is the topmost row.)
> 
> Sure, I don't disagree on your math.  It's the enum name change which concerns me?

Rather, I agree that -90 = 270.  But Origin Left Bottom != Origin Right Top, and the spec seems to say 8 = Origin Left Bottom, no?

[Nico Weber]

(In reply to comment #15)
> (From update of attachment 170295 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=170295&action=review
> 
> >>>> Source/WebCore/platform/graphics/ImageOrientation.h:45
> >>>> +    OriginRightTop = 8, // 270 degree CW rotation
> >>> 
> >>> The spec says 8 is Left/Bottom.  Why the change?
> >>> 
> >>> 8 = The 0th row is the visual left-hand side of the image, and the 0th column is the visual bottom.
> >> 
> >> Right, "The 0th row is the visual left-hand side of the image, and the 0th column is the visual bottom." describes a 90 degree rotation to the left, i.e. a 270 degree clockwise rotation. (The 0th row is the topmost row.)
> > 
> > Sure, I don't disagree on your math.  It's the enum name change which concerns me?
> 
> Rather, I agree that -90 = 270.  But Origin Left Bottom != Origin Right Top, and the spec seems to say 8 = Origin Left Bottom, no?

If you rotate an image 90 deg to the left to undistort it, then its origin was in the right top, which is the name after my cl. I think the old name didn't match the spec. (Also see the other two documents linked from the cl description)

[Eric Seidel (no email)]

(In reply to comment #16)
> > >>>> Source/WebCore/platform/graphics/ImageOrientation.h:45
> > >>>> +    OriginRightTop = 8, // 270 degree CW rotation
> > >>> 
> > >>> The spec says 8 is Left/Bottom.  Why the change?
> > >>> 
> > >>> 8 = The 0th row is the visual left-hand side of the image, and the 0th column is the visual bottom.
> > >> 
> > >> Right, "The 0th row is the visual left-hand side of the image, and the 0th column is the visual bottom." describes a 90 degree rotation to the left, i.e. a 270 degree clockwise rotation. (The 0th row is the topmost row.)
> > > 
> > > Sure, I don't disagree on your math.  It's the enum name change which concerns me?
> > 
> > Rather, I agree that -90 = 270.  But Origin Left Bottom != Origin Right Top, and the spec seems to say 8 = Origin Left Bottom, no?
> 
> If you rotate an image 90 deg to the left to undistort it, then its origin was in the right top, which is the name after my cl. I think the old name didn't match the spec. (Also see the other two documents linked from the cl description)

I'm trying to work backwards from a litteral reading of the spec:
8 = The 0th row is the visual left-hand side of the image, and the 0th column is the visual bottom.

that means the origin is at the "Visual left hand side" and the "visual bottom", which to me seems like Origin Left Bottom, or?

[noel gordon]

(In reply to comment #17)

> I'm trying to work backwards from a litteral reading of the spec:
> 8 = The 0th row is the visual left-hand side of the image, and the 0th column is the visual bottom.

Yeap.

> that means the origin is at the "Visual left hand side" and the "visual bottom", which to me seems like Origin Left Bottom, or?

Left-Bottom is correct, but that describes the 0th row and 0th column positions only. They are not origins, nor should they be talked about as if they were.

[noel gordon]

(In reply to comment #16)
> > > Sure, I don't disagree on your math.  It's the enum name change which concerns me?

The name change resulted in the selection of an incorrect transformFromDefault() (that code wasn't altered), and that broke fast/images/exif-orientation.html ...

> > 
> > Rather, I agree that -90 = 270.  But Origin Left Bottom != Origin Right Top, and the spec seems to say 8 = Origin Left Bottom, no?
> 
> If you rotate an image 90 deg to the left to undistort it, then its origin was in the right top, which is the name after my cl. 

Sure but exif orientation labels, Left-Bottom etc, are not origins - they only describe the 0th row and column position which is not the same thing.

For example, Left-Bottom is the 0th row-column position for orientation 8.  The enum label change made orientation 8 images use the transformFromDefault() for orientation 6.  No wonder fast/images/exif-orientation.html failed.

[noel gordon]

(In reply to comment #16)
> I think the old name didn't match the specs. (Also see the other two documents linked from the cl description)

The old names matched the spec, I believe.  Reading from JEITA CP-3451, page 18, http://www.exif.org/Exif2-2.PDF, the orientation defines the position of the 0th row and 0th column.  Writing them down I get:

1 Top-Left
2 Top-Right
3 Bottom-Right
4 Bottom-Left
5 Left-Top
6 Right-Top
7 Right-Bottom
8 Left-Bottom

http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/ImageOrientation.h?rev=113486#L38 is the old code. It says:

OriginTopLeft = 1, // default
OriginTopRight = 2, // mirror along y-axes
OriginBottomRight = 3, // 180 degree rotation
OriginBottomLeft = 4, // mirror along the x-axes
OriginLeftTop = 5, // -90 degree rotation + mirror along x-axes
OriginRightTop = 6, // 90 degree rotation
OriginRightBottom = 7, // 90 degree rotation + mirror along x-axes
OriginLeftBottom = 8, // -90 degree rotation

These old names exactly match the EXIF 2.2 (current) spec an I think we should restore and use them to avoid further confusion.

[Nico Weber]

I think both patches match the spec, they just look at it in a different way.

The original names asked "if the image origin is transformed by the orientation, where does it end up?", while the current names ask "which point on the image should be transformed to the origin when applying the orientation transform?"

The transforms corresponding to this are inverse of each other. The first four transforms are reflections, and they are self-inverse, so it doesn't make a difference for them. The other four transforms are rotations, and for them the interpretation is important. I think it effectively amounts to deciding if the affinetransform returned by this class should be applied in a left-handed or a right-handed coordinate system. The original behavior needed a left-handed system, the current behavior a right-handed one. WebKit's "default" coordinate system is right-handed I think (origin in the upper left), so the current behavior seems a bit more natural to me. But if people prefer, I can revert the enum rename and tweak the skia drawing code to work with a lefthanded imageorientation transform instead.

[noel gordon]

Given that these names were correct, how did the CG port work?  Reading from ...

http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp?rev=117083#L369

the code first pre-translates the destRect (accounting for inverted y co-ords on the mac) and sets the destRect origin to 0,0

  // Flip the coords.
  CGContextTranslateCTM(context, adjustedDestRect.x(), adjustedDestRect.maxY());
  CGContextScaleCTM(context, 1, -1);
  adjustedDestRect.setLocation(FloatPoint());

The code then applies transformFromDefault() for the orientation:

  if (orientation != DefaultImageOrientation) {
      CGContextConcatCTM(context, orientation.transformFromDefault(adjustedDestRect.size()));
      if (orientation.usesWidthAsHeight()) {
      // The destination rect will have it's width and height already reversed for the orientation of
      // the image, as it was needed for page layout, so we need to reverse it back here.
      adjustedDestRect = FloatRect(adjustedDestRect.x(), adjustedDestRect.y(), adjustedDestRect.height(), adjustedDestRect.width());
      }
  }

and finally paints the image.  Seems sensible to me, works fine for imageDocument, and passes fast/images/exif-orientation.html.  Skia could do something similar.

[noel gordon]

Created attachment 170662 [details]
exif-orientation-method-one.git.diff

Assuming the changes the CG are reverted, this git diff for ImageSkia.cpp makes fast/images/exif-orientation.html pass locally for me.

Note sure why I need to scale(-1,1) for exif orientations 5-8 to make that test pass, added a FIXME about it.

[noel gordon]

Created attachment 170665 [details]
exif-orientation-method-two.git.diff

Assuming the CG changes were reverted, this second approach writes out the transforms and translations in place.

So I don't use transformFromDefault(), which is sad, but the code might help spot why the scale(-1,-1) is needed for Skia.  The scale(-1,-1) is embedded in the affine transform for orientations 5-8 here, and again, fast/images/exif-orientation.html passes locally for me.

[Nico Weber]

Alright, a different fix is in https://bugs.webkit.org/show_bug.cgi?id=100401 . By popular request, it reverts the changes to ImageOrientation (and to the mac port code) and instead makes the skia code work with the left-hand coordinate frame.

I'm happy with either patch. The new patch does have the property that it's more similar to the currently checked-in code, but I'm not sure if that's an advantage or not :-)