Bug 100179

Created attachment 170287 [details] Patch

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.

+timothy_horton for the CG change (see changelog)

Created attachment 170295 [details] Patch

+senorblanco for skia review +eseidel, +noel.gordon fyi

Comment on attachment 170295 [details] Patch I don't grok all of the special cases, but the skia stuff looks ok.

Comment on attachment 170295 [details] Patch r=me

Comment on attachment 170295 [details] Patch Clearing flags on attachment: 170295 Committed r132384: <http://trac.webkit.org/changeset/132384>

All reviewed patches have been landed. Closing bug.

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.

Fix in bug 100319

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.

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.)

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?

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?

(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)

(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?

(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.

(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.

(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.

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.

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.

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.

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.

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 :-)