Bug 19688 – Add autodetection of image orientation from EXIF information

• Component: Images
• Status: RESOLVED FIXED

[Indrek Siitan]

Since so many people still post their images directly to web, either on pages or just as plain files in a directory, it would be useful if Safari autodetected their orientation from EXIF information if that was present.

Example image: http://tfr.cafe.ee/kama/IMG_0263.JPG

The EXIF info contains "Image Orientation: Top; Left Hand" - that is, for viewing as intended, it should be rotated 90 degrees counterclockwise.

<rdar://problem/4126979>

[Mark Rowe (bdash)]

To clarify, are you're suggesting that this data be used when viewing a standalone image, or also when images are embedded in an HTML page?

[Indrek Siitan]

This is probably better to be used only for standalone images - since no other browsers support it, using it for embedded images would probably lead to too much confusion. 

[David Carson]

Created attachment 29372 [details]
Rotate the image based on the EXIF orientation

[Darin Adler]

(In reply to comment #3)
> This is probably better to be used only for standalone images - since no other
> browsers support it, using it for embedded images would probably lead to too
> much confusion. 

Standalone images can be inside <iframe> or <object> elements. I think the same arguments apply to those as to <img> elements. I am not sure this is a good change.

[David Carson]

> I am not sure this is a good change.
 
How about if it is wrapped in a FEATURE() macro?

[Alexey Proskuryakov]

I think that this could be a nice feature for images that are really standalone - i.e., the document URL matches image URL, and the document is the main one.

[Indrek Siitan]

Yes, that is what I meant (see my original description) and that is what's clarified in the previous info here.

[David Carson]

Created attachment 29517 [details]
Re-worked patch to put ENABLE flags around the feature.

[David Carson]

Created attachment 29674 [details]
Updated patch

Added validation to the returned EXIF value as some images, such as:
http://remodelingthislife.com/wp-admin/images/DSC_4652-1.JPG
has an EXIF orientation set, but set to an invalid number, in this case set to zero.

[Greg Bolsinga]

Comment on attachment 29674 [details]
Updated patch

> +#if ENABLE(RESPECT_EXIF_ORIENTATION)
> +    int m_orientation;
> +#endif

This isn't initialized in the contstructor, nor cleared in FrameData::clear().

> +#if ENABLE(RESPECT_EXIF_ORIENTATION)    
> +    // EXIF orientation specified by EXIF spec
> +    static const int ImageEXIFOrientationTopLeft = 1;
> +    static const int ImageEXIFOrientationTopRight = 2;
> +    static const int ImageEXIFOrientationBottomRight = 3;
> +    static const int ImageEXIFOrientationBottomLeft = 4;
> +    static const int ImageEXIFOrientationLeftTop = 5;
> +    static const int ImageEXIFOrientationRightTop = 6;
> +    static const int ImageEXIFOrientationRightBottom = 7;
> +    static const int ImageEXIFOrientationLeftBottom = 8;
> +#endif

Could this be an enum?

>      bool frameHasAlphaAtIndex(size_t); 
> +    int frameOrientationAtIndex(size_t);
>  

This needs to be wrapped in #if ENABLE(RESPECT_EXIF_ORIENTATION). It should return the enumerated type.

> +#if ENABLE(RESPECT_EXIF_ORIENTATION)
> +    int orientationAtIndex(size_t) const; // EXIF image orientation
> +#endif

It should return the enumerated type.


> +#if ENABLE(RESPECT_EXIF_ORIENTATION)    
> +    int exifOrientation = frameOrientationAtIndex(m_currentFrame);
> +
> +    float currHeight = (exifOrientation & 0x01) ? CGImageGetHeight(image) : CGImageGetWidth(image);
> +#else
>      float currHeight = CGImageGetHeight(image);
> +#endif

Can there be a helper method that does the & 0x01 work? isExifOrientationEnabled?


>      // Flip the coords.
>      ctxt->setCompositeOperation(compositeOp);
>      CGContextTranslateCTM(context, adjustedDestRect.x(), adjustedDestRect.bottom());
>      CGContextScaleCTM(context, 1, -1);
> +#if ENABLE(RESPECT_EXIF_ORIENTATION)
> +    if (exifOrientation > BitmapImage::ImageEXIFOrientationTopLeft) {
> +        CGContextConcatCTM(context, transform);
> +        if (!(exifOrientation & 0x01)) {
> +            // The destination rect will have it's width and height already reveresed 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());
> +        }
> +    }
> +#endif

What does exifOrientation == 0 mean? Should there be an ASSERT to verify that value doesn't make it through?

Why isn't the transform concatenated when ImageEXIFOrientationTopLeft?

> +#if ENABLE(RESPECT_EXIF_ORIENTATION)
> +        // If the image is rotated in either direction, we need to swap w and h
> +        // Even numbers (2,4,6 & 8) mean the image is rotated.
> +        if (orientationAtIndex(index) & 0x01)
> +            result = IntSize(w, h);
> +        else 
> +            result = IntSize(h, w);
> +#else
>          result = IntSize(w, h);
> +#endif

This can use isExifOrientationEnabled

> +#if ENABLE(RESPECT_EXIF_ORIENTATION)
> +int ImageSource::orientationAtIndex(size_t index) const 

Return an enum?

> +{
> +    int orientation = BitmapImage::ImageEXIFOrientationTopLeft;
> +    CFDictionaryRef properties = CGImageSourceCopyPropertiesAtIndex(m_decoder, index, imageSourceOptions());
> +    if (properties) {
> +        // The numbers returned from kCGImagePropertyOrientation match the EXIF specification
> +        // so we can just use the value directly.
> +        CFNumberRef orientationProperty = (CFNumberRef)CFDictionaryGetValue(properties, kCGImagePropertyOrientation);
> +        if (orientationProperty != NULL)
> +            CFNumberGetValue(orientationProperty, kCFNumberIntType, &orientation);
> +        CFRelease(properties);
> +        // Validate the orientation returned, and if it is outside known values, use
> +        // the default.
> +        if (orientation < BitmapImage::ImageEXIFOrientationTopLeft || orientation > BitmapImage::ImageEXIFOrientationLeftBottom)
> +            orientation = BitmapImage::ImageEXIFOrientationTopLeft;
> +    }
> +    return orientation;
> +}
> +#endif
> +
>  IntSize ImageSource::size() const
>  {
>      return frameSizeAtIndex(0);
>

[Eric Seidel (no email)]

Comment on attachment 29674 [details]
Updated patch

No response to Greg's comment in over 3 weeks, marking r-.

[Alexey Proskuryakov]

*** Bug 50847 has been marked as a duplicate of this bug. ***

[Eric Seidel (no email)]

Looks like David got distracted.  I'll post a new patch for review.

[Eric Seidel (no email)]

The given example image http://tfr.cafe.ee/kama/IMG_0263.JPG seems to have an EXIF Orientation "1" (normal), so isn't a very good test case. :)

[Eric Seidel (no email)]

http://www.kotikone.fi/kuukkeli/antin_exiftran.html is a good test case.

[Eric Seidel (no email)]

Created attachment 77660 [details]
Updated patch, but doesn't seem to work quite right

[Eric Seidel (no email)]

I must have broken things when moving the code around.  Images with EXIF just don't draw with my patch. :)  It's also possible that other parts of the Image subsystem have changed since the original patch was written and there need to be additional modifications to make such a patch work.

If we're going to land this, we'll need to create a set if tests which verifies that we only apply EXIF rotations for stand-alone images.  Which means that we'll probably need to move this code *out* of ImageCG and into ImageDocument instead.

I think that applying EXIF rotations to all images would probably break the web since no other browser supports such.

[Eric Seidel (no email)]

Created attachment 77661 [details]
Test images (zip)

I made a set of 9 test images (attached in a zip), which show values 1-8 as well as value 9 (which should be invalid and thus treated as value 1).

[Eric Seidel (no email)]

Ok, so I've written tests, and mostly got David's patch working (due to changes in how Images are drawn, the transforms need to be adjusted.  I haven't figured out exactly how yet though.)

It's unclear to me how we can do this w/o breaking the web.

The proposals:

1.  Turn auto-exif-rotate on for all images.
 -- This is what David's patch does (and may be what Mobile Safari does already?)  I would expect that to break the web.  No other browser respects EXIF, so we'd be the odd man out.  Websites which already attempt to compensate for browser behavior with things like http://www.nihilogic.dk/labs/exif/ or http://byrev.org/bookmarks/wordpress-exif-orientation-plugin/ or numerous others would have to special case WebKit not to do anything. :)  This one seems no-go.

2.  Turn on auto-rotate for images based on CSS or an <img> atribute.
-- This would mean introducing some new CSS property or value that authors could set and WebKit would then autorotate.  I'm not sure how this would affect <canvas> if at all.  This is possible, but still would have the same "every EXIF-rotate hack needs to know to turn itself off for WebKit" problem.

3.  Turn on auto-rotate only for ImageDocument, only when the main document.
-- This was proposed by others in this bug as well as in crbug.com/56845.  This would be possible, but still might be confusing to users when images viewed standalone show one way and then when used in an <img> tag show another.  David's patch would mostly be discarded if we went this route, as we could do this entirely with some CSS transforms from within ImageDocument.cpp and wouldn't need any Image modification code at all.


In the end, authors can already work around this serverside by passing images through ImageMacick or another processor.  They can work around it clientside using things like http://www.nihilogic.dk/labs/exif/.  They can't change the browser's behavior for stand-alone ImageDocuments of course.

[Nico Weber]

FWIW, I like both 3 and 2.

"every EXIF-rotate hack needs to know to turn itself off for WebKit"  could have been said about gradient hacks and rounded corner hacks :-)

[Eric Seidel (no email)]

It appears that when David wrote the original patch, images used to be drawn at 0,0 always (and 0,0 was just translated to where we wanted it to be.  Now we pass the desired draw rect to CGContextDrawImage, which ends up translating in our re-oriented coordinate space. :(

I'll have to investigate why drawing was changed in this way, and if its OK to change it back to the old way to suport this kind of reorientation (we need to translate to the final image coords before we rotate, etc.)

[Eric Seidel (no email)]

The translation behavior changed in http://trac.webkit.org/changeset/46002.

[Eric Seidel (no email)]

Created attachment 77689 [details]
Working patch -- always repects exif, needs tests

[Alexey Proskuryakov]

> Websites which already attempt to compensate for browser behavior with things like http://www.nihilogic.dk/labs/exif/ or http://byrev.org/bookmarks/wordpress-exif-orientation-plugin/ or numerous others would have to special case WebKit not to do anything. :)

Do you have any examples of such?

Option #1 has been implemented and shipped in iOS already. But <http://www.kotikone.fi/kuukkeli/antin_exiftran.html> is broken as a result - the rotated image is scaled to fit its original rectangle.

[Eric Seidel (no email)]

Created attachment 77703 [details]
Working patch, ready for comment

[Eric Seidel (no email)]

https://bugzilla.mozilla.org/show_bug.cgi?id=298619 is mozilla's bug on the subject.

[Eric Seidel (no email)]

I'm looking or comments, particularly from Simon, or Hyatt, or Peter, or Adam or anyone else who has worked on the image subsystem recently.

I believe we should at least do proposal #3, and possibly proposal 2 or even 1.  The question becomes if this patch is following an acceptable approach to such.

[Eric Seidel (no email)]

(In reply to comment #25)
> > Websites which already attempt to compensate for browser behavior with things like http://www.nihilogic.dk/labs/exif/ or http://byrev.org/bookmarks/wordpress-exif-orientation-plugin/ or numerous others would have to special case WebKit not to do anything. :)
> 
> Do you have any examples of such?
> 
> Option #1 has been implemented and shipped in iOS already. But <http://www.kotikone.fi/kuukkeli/antin_exiftran.html> is broken as a result - the rotated image is scaled to fit its original rectangle.

I do not have data on the prevalence of EXIF-tagged images, or sites which expect them to be rotated vs. not.  It does seem to me that auto-rotating images is a bigger deal that color-matching, and if I remember correctly we've historically had compat troubles with the fact that we colormatch images and other browsers don't.

We need data here.  i don't have a good way to provide that for you yet.  sorry.

[Eric Seidel (no email)]

I'm sure we could make a better test for this.  Perhaps even one which does not require pixel dumps (by using canvas perhaps?), but I ran out of steam.  Constructing test images was kinda a pain. I wrote a python script to produce this test which I can attach.

[Eric Seidel (no email)]

Created attachment 77704 [details]
python script I used to generate the test images and test html

I wrote this script to assist in generating the various oriented images.

A slicker test could be constructed by first rotating the images and then using exif values to cause them to rotate back to "normal" when viewed with an exif-sensitive viewer.  Then it would be easy to see that the images were all correct.  But the test I've included in the patch was easier to come up with on a first-pass.  Depending on how much more code revision I do to the patch, I may update the test further.

[WebKit Review Bot]

Attachment 77703 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7213283

[Build Bot]

Attachment 77703 [details] did not build on win:
Build output: http://queues.webkit.org/results/7314249

[Eric Seidel (no email)]

Created attachment 77705 [details]
This implements option #1 (turns on EXIF rotation everywhere)

[Maciej Stachowiak]

Comment on attachment 77705 [details]
This implements option #1 (turns on EXIF rotation everywhere)

I suggest starting with approach #3 (only top-level standalone documents) and consider expanding later.

[WebKit Review Bot]

Attachment 77703 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7321286

[WebKit Review Bot]

Attachment 77703 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7307314

[WebKit Review Bot]

Attachment 77705 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7236310

[Cosmin Truta]

Here are my 2 cents worth of opinion:

I like Eric's patch. If I understand correctly, he implemented option 1, with the possibility to turn off the auto-rotation from the browser configuration flags.

This behavior is consistent with today's image viewers. They auto-rotate the images by default, and some of them offer the option to disable that if the user needs some band-aid (which shouldn't normally happen).

Although it may break the web right now, have different behavior on different browsers for a while, etc., the same thing happened with image viewers some 5-10 years ago, when they slowly recognized EXIF and EXIF-rotation. This is pretty much resolved in the land of image viewer/processor apps.
If certain websites are aware of the EXIF flag and rotating the images themselves, they can (and perhaps they should?) avoid embedding the rotation flag in already-rotated images, or reset it to "top-left". This will solve the rotation problem while keeping this flag at its proper meaning.

I have a slight preference for option 1 instead of option 2. The EXIF-rotate flag is the presentation attribute to respect, regardless what image viewer, image editor or web browser shows the image; there shouldn't be a need for another attribute.

I didn't like option 3, because it's too confusing. I like the idea of seeing the exact same image, regardless the context. Besides, I wouldn't be surprised if people will start filing bugs when they see one image in embedded in HTML and another image at the top level.

[Cosmin Truta]

The more I think about this, the more fond I am for option 1.

A proper workaround for a website that delivers already-rotated images is to either reset or remove the rotation flag in the image file. This is good both conceptually (because the communication is clear: "don't bother rotating this because I already did it") and practically (because all www browsers will work properly).

Would it be somehow possible to communicate this to the designers of the websites discussed here?

Workarounds that break at the time when the issue they address gets properly fixed, are bad workarounds.

[Alexey Proskuryakov]

Looking at this from a "cleanest design" standpoint is probably not the best approach. In ideal world, EXIF image rotation simply wouldn't exist - it's unnecessary, because all camera JPEGs can be quickly and losslessly rotated without it (see e.g. topic #10 in <http://www.faqs.org/faqs/jpeg-faq/part1/>).

I agree with Eric and Maciej on starting with option 3. It's hard to imagine a situation where being different from other browsers (and mail clients) would be beneficial.

[Peter Kasting]

Personally, I would implement option 1 initially:

* We wouldn't be diverging from _all_ other browsers (given the iOS mention in comment 25; and keeping all WebKit-based browsers the same seems like a good goal).

* The consequence of option 3, where images display differently as an <img> than as a direct file, seems very confusing to me and isn't a concept I want to release into the wild.

* I would expect the actual bustage rate to be low; but also,

* It's easy to leave this off by default but ship it turned on in some Chrome dev or beta builds, to collect data on how much stuff is broken.  We might even be able to instrument our builds to try and autodetect sites that rotate these kinds of images.

Having data seems like the best way to proceed.  Shipping option 1 to some Chromium dev users and asking them in our blog post to report bustage to us seems like the easiest way to start collecting data.

[Alexey Proskuryakov]

One of the consequences of being different from everyone else:
- you drag images into your e-mail in Mail.app, everything looks good;
- you send the e-mail to your friends or clients;
- images are rotated and distorted in their Outlook Express;
- you lose.

Same with making a quick Web site and testing it in your browser before sending a link, of course.

This isn't something that can be turned into measurable data with Chrome beta builds.

> keeping all WebKit-based browsers the same seems like a good goal

There are other ways to achieve this. As mentioned before, iOS is just broken with <img> elements and EXIF rotation, so it will need to change either way.

Are there any other reasons to prefer option 1, besides aligning with iOS? I also expect bustage to be relatively low, but  benefit is pretty much non-existent, especially given that EXIF rotation has been a misfeature and trouble from the start.

Please also note that option 3 is what has been originally requested in this bug. Why hijack it for more involved discussions?

[Peter Kasting]

(In reply to comment #43)
> - you send the e-mail to your friends or clients;
> - images are rotated and distorted in their Outlook Express;
> - you lose.
> 
> This isn't something that can be turned into measurable data with Chrome beta builds.

You are right that any data we collect won't inform how mail clients should act.

If you wanted Mail.app and Safari to be different, the only way to do it would be via a run-time pref.  I suppose we could do that.

Long-term, it does seem like the ideal outcome is that mail clients, like image viewers, should respect EXIF rotation data.  How to get from here to there is less clear.

> As mentioned before, iOS is just broken with <img> elements and EXIF rotation, so it will need to change either way.

True... but I would assume it would change by fixing the scaling issue, not by disabling EXIF rotation detection entirely.  Maybe that assumption is bad.

> Are there any other reasons to prefer option 1, besides aligning with iOS?

Convenience for users, whose images on the web will behave like they do in their image viewers?  If we're assuming users upload files directly to hosting sites they own, it seems reasonable to assume that a large fraction of them are also going to embed those images in some sort of page, even if that page is just a simple slideshow script.  Comment 0 even says "...either on pages...".

The consistency of option 1 -- or doing nothing -- both seem better to me than option 3 due to the fact that option 3 is just flat-out confusing to describe or encounter.  I can't imagine option 3 being the desired long-term end state; so if option 3 is a step towards long-term doing option 1, are we gaining anything by not testing out option 1 now?

> I also expect bustage to be relatively low, but  benefit is pretty much non-existent, especially given that EXIF rotation has been a misfeature and trouble from the start.

OK; but then it's fair to ask: are there reasons to prefer option 3 to doing nothing?  If the benefit of option 1 is minimal, isn't the benefit of option 3 equally minimal if not moreso?

> Please also note that option 3 is what has been originally requested in this bug. Why hijack it for more involved discussions?

I wasn't trying to hijack anything; Eric asked for opinions.

It does seem reasonable to consider our future plans when deciding what to implement now.

[Alexey Proskuryakov]

> If you wanted Mail.app and Safari to be different, the only way to do it would be via a run-time pref.

This seems undesirable to me. Basically, any client that can be used to create content should not autorotate when creating to avoid fooling the user. Drawing the line to choose reasonable defaults for every WebKit client seems almost impossible.

> I can't imagine option 3 being the desired long-term end state

There are practical use cases for it:
- Looking at images that are already on the Web (like http://tfr.cafe.ee/kama/IMG_0263.JPG), and whose authors don't care whether viewers has to lean their heads.
- Super quick image sharing - copy them to ~/Sites on Mac, and enable Web sharing. Why not display the images with correct orientation? Formally speaking, the potential for fooling oneself is still here, but I think that, the likelihood of any sort of embarrassment is extremely low.
- Drag and drop a file from Finder to Safari. Why not display it in correct orientation?
- Go to <http://www.dpreview.com/galleries/reviewsamples/photos/680788/img_0881?inalbum=canon-eos-60d-review-samples>, click "Original".

I don't think that there is a lot of explaining to do - it's just that if you write HTML markup, then you need to prepare your digital camera images - which you're already doing for downscaling. And if it's not HTML, then the browser tries to do a reasonable thing with whatever gets thrown at it.

> it seems reasonable to assume that a large fraction of them are also going to embed those images in some sort of page, even if that page is just a simple slideshow script

This may happen indeed. I don't expect it to be common enough to be an issue though - digital camera images are huge, and not-so-simple slideshow scripts know how to rotate and downscale. In particular, this won't happen on Flickr, Picasa or MobileMe galleries.

> OK; but then it's fair to ask: are there reasons to prefer option 3 to doing nothing?  
> If the benefit of option 1 is minimal, isn't the benefit of option 3 equally minimal if not moreso?

Yes, and this bug wasn't getting a lot of attention indeed. The issue keeps coming up though, and since there are valid use cases, and the risks are smaller, I've been convinced that we can do #3.

> I wasn't trying to hijack anything; Eric asked for opinions.

This wasn't directed at you or me alone. As Eric correctly described, his change lays the groundwork for any option, so it is probably best to land it with the least controversial user-visible change. I do not mean to belittle your opinion, but #3 seems least controversial now, if only counting voices against each option.

[Peter Kasting]

(In reply to comment #45)
> > If you wanted Mail.app and Safari to be different, the only way to do it would be via a run-time pref.
> 
> This seems undesirable to me. Basically, any client that can be used to create content should not autorotate when creating to avoid fooling the user. Drawing the line to choose reasonable defaults for every WebKit client seems almost impossible.

Not sure I agree, but I'm not sure I understand correctly either, so I'll leave that alone :)

> > I can't imagine option 3 being the desired long-term end state
> 
> There are practical use cases for it:

I agree, and that's why I preferred doing something which also addresses these to doing nothing.  I meant this more in the sense that it seemed like the objections to the broader change of option 1 were more in the vein of "let's not break the web, sites and other browsers would also need to change, etc.", the sorts of issues that can be ironed out in time, as opposed to a more philosophical "option 1 is fundamentally the wrong thing to do, ever".  But maybe that's exactly what you're saying above.

> if it's not HTML, then the browser tries to do a reasonable thing with whatever gets thrown at it.

I suppose one obvious analog is how browsers auto-shrink-to-fit standalone images but don't do the same with <img> tags.  That argues that option 3 might not be so confusing as I'm making it seem.

> As Eric correctly described, his change lays the groundwork for any option, so it is probably best to land it with the least controversial user-visible change.

If implementing option 3 is a subset of implementing option 1 (rather than a disjoint route), the only question I have is, if we do want option 1 someday, how we intend to get from here to there, e.g. gather data or whatever.  As long as we have some coherent view on what the future plan is, I'm OK with doing option 3 now; I think your examples and the auto-zooming I mention above convince me that option 3 is better than status quo.

[Alexey Proskuryakov]

> how we intend to get from here to there, e.g. gather data or whatever.

This is not an easy question to answer! Obviously, getting support from other vendors, as well as suggesting important scenarios where this would make a difference are some steps that can be expected. Finding out whether this would break sites that auto-rotate in JavaScript is of course required, too.

[Eric Seidel (no email)]

It seems my patch should be rather uncontroversial?   All it does is make this an option to enable.  Basically allowing iPhone to unfork were they to so choose.

[Eric Seidel (no email)]

Oh.  I guess my patch turns this on by default.  Which is I guess what makes it controversial. :)

[Eric Seidel (no email)]

I'm no longer actively working on this bug.

[Eric Seidel (no email)]

Would be nice to hear from ddkilzer as to why iPhone respects EXIF.  Since they're the main source of this EXIF content as far as I can tell.  Certainly it's the "photos shared from iphone" which seems to get the most complains from gmail users (at least the ones which reach my ears, undoubtably with some sampling bias).

[Alexey Proskuryakov]

Comment on attachment 77705 [details]
This implements option #1 (turns on EXIF rotation everywhere)

Actually marking r-. I think that we've agreed upon a good plan now, and many parts of this patch will be useful.

[David Kilzer (:ddkilzer)]

(In reply to comment #51)
> Would be nice to hear from ddkilzer as to why iPhone respects EXIF.  Since they're the main source of this EXIF content as far as I can tell.

We respect EXIF so that shared photos are drawn correctly.

See: <rdar://problem/6674885>

[Darin Adler]

The iOS motivation was photos that were being put into outgoing mail messages.

[Eric Seidel (no email)]

(In reply to comment #54)
> The iOS motivation was photos that were being put into outgoing mail messages.

Funny that the motivation for Chrome is user-complaints about incoming messages from iPhones. :)

[Eric Seidel (no email)]

@darin, @ap, @ddkilzer:

I would argue that iPhone being the only WebKit browser (and only browser, period) to respect EXIF hurts WebKit.  Consistency across platforms is one of the strengths of WebKit.

I think we should either lobby iOS to remove this feature, or we should make the rest of the WebKit ports agree.

The GMail team is very sad about iPhone images looking wrong in Gmail.  Obviously they could fix this server-side.  But that leaves Hotmail, Yahoo, etc. out to dry.  We should either make browsers agree.  Since the iPhone's behavior seems desirable to users, we should just implement option #1 (as my patch does) and be done with it.

[Alexey Proskuryakov]

Note that there is an active discussion in <https://bugzilla.mozilla.org/show_bug.cgi?id=298619>. That's probably a better place for advocacy.

[Robert O'Callahan]

Hey, no fair dumping the advocacy flamewar into our Bugzilla :-)

[Robert O'Callahan]

IMHO instead of trying to change the behavior of all Web browsers and other apps that consume JPEG, and breaking some unknown number of images on the Web, Apple should just fix their iOS apps to properly rotate images before emailing or uploading them. That approach has less recoding, less coordination, less developer confusion, a faster migration path, and zero risk.

[Robert O'Callahan]

Android behaves the same as iOS so it's not just Apple who would have to fix stuff. I'll carry on the discussion in https://bugzilla.mozilla.org/show_bug.cgi?id=298619.

[Eric Seidel (no email)]

roc points out that Android also respects EXIF.

Although I initially had "break the web" concerns, I've yet to see a website which would be broken by this.

I should apply my patch locally and try surfing various picture sharing sites to see if they'll be negatively affected by this.

There are 3 types of possible breakage:

1.  Images with height/width specified, which are now auto-rotated, and look squished.
2.  Images without height/width, which are now auto-rotated and cause other parts of the page to move (and possibly look wrong).
3.  Sites which use an existing client-side auto-rotate solution like: http://www.nihilogic.dk/labs/exif/ or http://byrev.org/bookmarks/wordpress-exif-orientation-plugin/which may now break.  (I know of no such site).


I've come around to believe that the breakage list is likely to be very small.  I would recommend we add this support to match existing image viewers, and unbreak mail sites and sites which include user-uploaded photos.

We should of course be careful when adding this to test more sophisticated photo-sharing sites like flikr, facebook, picassa, etc. to make sure we're not breaking them.

I'll tidy-up and re-post this patch next week.

[Adam Barth]

> Although I initially had "break the web" concerns, I've yet to see a website which would be broken by this.

How have you looked for such web sites?  Perhaps we should run an UMA experiment to see how many images would be broken?

[Eric Seidel (no email)]

(In reply to comment #62)
> > Although I initially had "break the web" concerns, I've yet to see a website which would be broken by this.
> 
> How have you looked for such web sites?

Just my own surfing.  A biased sampleset for sure.

> Perhaps we should run an UMA experiment to see how many images would be broken?

I'm not sure how to do that, but I would be strongly supportive of research.  We could also just flip it on (in cr-dev or otherwise) and see who screams (like we've done with many other such changes).

[Robert O'Callahan]

(In reply to comment #61)
> There are 3 types of possible breakage:
> 
> 1.  Images with height/width specified, which are now auto-rotated, and look squished.
> 2.  Images without height/width, which are now auto-rotated and cause other parts of the page to move (and possibly look wrong).
> 3.  Sites which use an existing client-side auto-rotate solution like: http://www.nihilogic.dk/labs/exif/ or http://byrev.org/bookmarks/wordpress-exif-orientation-plugin/which may now break.  (I know of no such site).

There's a fourth possibility:
4. Images with EXIF orientation that have been manually rotated to display correctly on the Web (or in a photo viewer that doesn't respect EXIF orientation, such as the Windows 7 built-in Photo Viewer), but the EXIF data was not updated (or was updated incorrectly) so now the EXIF orientation is incorrect.

(I just noticed that if you rotate the image in Windows Photo Viewer to have the correct orientation, it adds an EXIF tag with the correct orientation, but doesn't remove the original one! I wonder what kind of issues that triggers...)

[Eric Seidel (no email)]

(In reply to comment #64)
> There's a fourth possibility:
> 4. Images with EXIF orientation that have been manually rotated to display correctly on the Web (or in a photo viewer that doesn't respect EXIF orientation, such as the Windows 7 built-in Photo Viewer), but the EXIF data was not updated (or was updated incorrectly) so now the EXIF orientation is incorrect.
> 
> (I just noticed that if you rotate the image in Windows Photo Viewer to have the correct orientation, it adds an EXIF tag with the correct orientation, but doesn't remove the original one! I wonder what kind of issues that triggers...)

Another way of writing what you're saying is that the image has EXIF data, and looks OK today, but that EXIF data is wrong.

The interblags seem upset about Windows Photo Viewer's current behavior:
http://answers.microsoft.com/en-us/windows/forum/windows_7-pictures/windows-photo-viewer-or-live-photo-gallery-does/a161c8da-c1ce-4347-a92e-724f9e535c15

I expect that images with wrong EXIF data will look wrong in all of the other photo viewers on other platforms like Gtk and Mac as well as the iPhones and Android devices that we're trying to match.

[Robert O'Callahan]

It would be surprising if Windows 7 Photo Viewer and all Web browsers are the only image viewers that don't respect the orientation tag. Indeed, OpenOffice "Insert Picture" and Microsoft Paint don't either. Obviously anything that uses the IE ActiveX control (or Webkit, for that matter) wouldn't.

I can't find anywhere in the EXIF spec or related specs that says which tag to use if there's more than one orientation tag (what you get when Windows Photo Viewer rotates an image). It would be interesting to compare EXIF-aware viewers to see if there's interop on that. Here's an image you can try:
http://people.mozilla.org/~roc/IMG_20110526_110158.jpg
If it displays correctly, you're using the last EXIF orientation tag; if it displays incorrectly, you're using the first one.

There's at least a very strong argument that if you're producing JPEGs you should not use the EXIF orientation tag.

Maybe we should do something in browsers to deal with EXIF orientation, but I'm leery of creating more interop problems. I feel tempted to hold the line and keep things simple by ignoring it.

[Alexey Proskuryakov]

> I'm not sure how to do that, but I would be strongly supportive of research.  We could also just flip it on (in cr-dev or otherwise) and see who screams (like we've done with many other such changes).

My preference is to enable auto-rotation for full page image documents in WebKit nightlies (and perhaps in chromium builds if you want), and to discuss the rest separately.

[Tab Atkins]

(In reply to comment #60)
> Android behaves the same as iOS so it's not just Apple who would have to fix stuff. I'll carry on the discussion in https://bugzilla.mozilla.org/show_bug.cgi?id=298619.

This doesn't appear to be true.  If I visit http://xanthir.com/etc/exif using the Android browser on my Nexus S (running, I believe, the latest public release of Android OS), I get the same result as on desktop browsers - the middle F is upright, not rotated (which means the EXIF is being ignored).

[Robert O'Callahan]

I meant that Android behaves like iOS in that the camera application adds EXIF data for rotated images without actually rotating the image data before you email it to someone.

[Adele Peterson]

I'd like to second this suggestion to enable this for full page image documents.

(In reply to comment #67)
> > I'm not sure how to do that, but I would be strongly supportive of research.  We could also just flip it on (in cr-dev or otherwise) and see who screams (like we've done with many other such changes).
> 
> My preference is to enable auto-rotation for full page image documents in WebKit nightlies (and perhaps in chromium builds if you want), and to discuss the rest separately.

[Tim Horton]

(In reply to comment #70)
> I'd like to second this suggestion to enable this for full page image documents.

I'm going to pick this back up where Eric left off. Would anyone be opposed to this approach?:

1. Enable for full-page ImageDocuments.
2. Add a preference (WebKitShouldRespectImageOrientation or something like that) to enable it globally.
3. Do none of this behind a feature flag, because it makes a lot of the settings plumbing-through-WebCore terrifyingly disgusting, and seems unnecessary.

I have a patch based on Eric's that does most of these things; if there are no objections I'll post it after cleaning it up later tonight.

[Darin Adler]

(In reply to comment #71)
> 2. Add a preference (WebKitShouldRespectImageOrientation or something like that) to enable it globally.

I don't understand the proposal for this part. What does “globally” mean? In an entire process? For a page group? The string seems like a Mac “defaults write” key, so is this for Mac only?

[Tim Horton]

(In reply to comment #72)
> (In reply to comment #71)
> > 2. Add a preference (WebKitShouldRespectImageOrientation or something like that) to enable it globally.
> 
> I don't understand the proposal for this part. What does “globally” mean? In an entire process? For a page group? The string seems like a Mac “defaults write” key, so is this for Mac only?

By "globally" I meant "not just for full-page ImageDocuments, but for <img> and others too", primarily so non-browser WebKit clients can choose to respect EXIF orientation everywhere if they so choose.

The preference will be via the normal settings API, so, for a page group! (yes, the name I mentioned before was the Mac key; I'm only going to plumb it through for Mac for now, others can add support in their own ports if they want).

[Darin Adler]

(In reply to comment #73)
> By "globally" I meant "not just for full-page ImageDocuments, but for <img> and others too"

OK, got it.

[Tim Horton]

Created attachment 135273 [details]
preliminary patch, respecting orientation for ImageDocuments, with a setting to enable for all images

Two issues:

1. I don't think it's legitimate to set shouldRespectImageOrientation on a BitmapImage; my understanding is that that BitmapImage (via the image cache) could be shared between page groups with differing settings. However, my patch to plumb the setting through from all call sites and not require that had ballooned up to 350K or so, and I wasn't really OK with the mess it was making of Image/GraphicsContext/friends. Any ideas?

2. The drag images (as Eric noted) are drawn with the right size but the wrong rotation (because the rotation happens via a transform on the context, which certainly won't happen in the drag image case); this isn't OK either, though that might be better as a followup fix.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Source/WebCore/platform/graphics/BitmapImage.h:209:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebCore/platform/graphics/ImageOrientation.h:35:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 2 in 32 files


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

[Early Warning System Bot]

Comment on attachment 135273 [details]
preliminary patch, respecting orientation for ImageDocuments, with a setting to enable for all images

Attachment 135273 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12307994

[Build Bot]

Comment on attachment 135273 [details]
preliminary patch, respecting orientation for ImageDocuments, with a setting to enable for all images

Attachment 135273 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12313902

[Early Warning System Bot]

Comment on attachment 135273 [details]
preliminary patch, respecting orientation for ImageDocuments, with a setting to enable for all images

Attachment 135273 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12310886

[WebKit Review Bot]

Comment on attachment 135273 [details]
preliminary patch, respecting orientation for ImageDocuments, with a setting to enable for all images

Attachment 135273 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12313896

[Philippe Normand]

Comment on attachment 135273 [details]
preliminary patch, respecting orientation for ImageDocuments, with a setting to enable for all images

Attachment 135273 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12312911

[Gyuyoung Kim]

Comment on attachment 135273 [details]
preliminary patch, respecting orientation for ImageDocuments, with a setting to enable for all images

Attachment 135273 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12317087

[Collabora GTK+ EWS bot]

Comment on attachment 135273 [details]
preliminary patch, respecting orientation for ImageDocuments, with a setting to enable for all images

Attachment 135273 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12312914

[Tim Horton]

(In reply to comment #75)
> 2. The drag images (as Eric noted) are drawn with the right size but the wrong rotation (because the rotation happens via a transform on the context, which certainly won't happen in the drag image case); this isn't OK either, though that might be better as a followup fix.

Nevermind, I just got this working, so I'll include that in this patch.

[Eric Seidel (no email)]

Thanks for picking this up. :)

[Tim Horton]

Created attachment 135384 [details]
updated patch, adding DragImageMac orientation support, and hopefully fixing the non-mac build

[Philippe Normand]

Comment on attachment 135384 [details]
updated patch, adding DragImageMac orientation support, and hopefully fixing the non-mac build

Attachment 135384 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12320327

[Tim Horton]

Created attachment 135388 [details]
added notimplemented.h to the wrong file

[Build Bot]

Comment on attachment 135388 [details]
added notimplemented.h to the wrong file

Attachment 135388 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12325229

[WebKit Review Bot]

Comment on attachment 135388 [details]
added notimplemented.h to the wrong file

Attachment 135388 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12320417

New failing tests:
http/tests/canvas/webgl/origin-clean-conformance.html
fast/canvas/canvas-incremental-repaint.html
accessibility/aria-disabled.html
fast/canvas/canvas-toDataURL-webp.html
canvas/philip/tests/toDataURL.jpeg.primarycolours.html
canvas/philip/tests/toDataURL.png.complexcolours.html
fast/canvas/webgl/premultiplyalpha-test.html
fast/canvas/canvas-createPattern-fillRect-shadow.html
fast/canvas/webgl/tex-image-and-sub-image-2d-with-image.html
fast/canvas/canvas-scale-drawImage-shadow.html
fast/canvas/DrawImageSinglePixelStretch.html
canvas/philip/tests/toDataURL.jpeg.alpha.html
canvas/philip/tests/toDataURL.png.primarycolours.html
canvas/philip/tests/toDataURL.jpeg.quality.basic.html
fast/canvas/image-pattern-rotate.html
fast/canvas/drawImage-with-invalid-args.html
fast/canvas/canvas-drawImage-shadow.html
canvas/philip/tests/security.dataURI.html
fast/canvas/image-object-in-canvas.html
http/tests/inspector/inspect-element.html
fast/canvas/webgl/gl-teximage.html

[WebKit Review Bot]

Created attachment 135427 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Tim Horton]

Created attachment 135494 [details]
patch

[WebKit Review Bot]

Comment on attachment 135494 [details]
patch

Attachment 135494 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12322584

New failing tests:
fast/images/exif-orientation.html

[WebKit Review Bot]

Created attachment 135507 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Tim Horton]

(In reply to comment #93)
> (From update of attachment 135494 [details])
> Attachment 135494 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/12322584
> 
> New failing tests:
> fast/images/exif-orientation.html

Gotta skip the test everywhere except on mac.

[Darin Adler]

(In reply to comment #95)
> Gotta skip the test everywhere except on mac.

Or maybe put the test in platform/mac if that’s the long term plan?

[Tim Horton]

(In reply to comment #96)
> (In reply to comment #95)
> > Gotta skip the test everywhere except on mac.
> 
> Or maybe put the test in platform/mac if that’s the long term plan?

There's no reason other platforms couldn't adopt the change too; they'd just have to implement a few things in their ImageSource implementation and set up the setting.

[Tim Horton]

Created attachment 135778 [details]
patch, removing setting on Image and instead plumbing setting through everywhere

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:174:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 1 in 45 files


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

[Tim Horton]

Created attachment 135779 [details]
patch

[Philippe Normand]

Comment on attachment 135779 [details]
patch

Attachment 135779 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12341232

[Early Warning System Bot]

Comment on attachment 135779 [details]
patch

Attachment 135779 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12339203

[Early Warning System Bot]

Comment on attachment 135779 [details]
patch

Attachment 135779 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12340200

[Tim Horton]

Created attachment 135784 [details]
another round of platform specific things and a lying compiler

[Simon Fraser (smfr)]

Comment on attachment 135784 [details]
another round of platform specific things and a lying compiler

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

r=me but please consider using an enum in place of the bool parameter in some places.

> LayoutTests/fast/images/exif-orientation.html:22
> +<head>
> +<title>This test can only be run with layoutTestController (or by manually setting WebKitShouldRespectImageOrientation to true).</title>
> +<script>
> +if (window.layoutTestController)
> +    layoutTestController.overridePreference('WebKitShouldRespectImageOrientation', 1);
> +</script>
> +<style>
> +img { border: 1px solid black; }
> +div { display: inline-block; margin-right: 20px; margin-bottom: 10px; width: 100px; valign: top; }
> +</style>
> +</head>
> +<div><img src="resources/exif-orientation-1-ul.jpg"><br>Normal</div>
> +<div><img src="resources/exif-orientation-2-ur.jpg"><br>Flipped horizontally</div>
> +<div><img src="resources/exif-orientation-3-lr.jpg"><br>Rotated 180&deg;</div>
> +<div><img src="resources/exif-orientation-4-lol.jpg"><br>Flipped vertically</div>
> +<br>
> +<div><img src="resources/exif-orientation-5-lu.jpg"><br>Rotated 90&deg; CCW and flipped vertically</div>
> +<div><img src="resources/exif-orientation-6-ru.jpg"><br>Rotated 90&deg; CCW</div>
> +<div><img src="resources/exif-orientation-7-rl.jpg"><br>Rotated 90&deg; CW and flipped vertically </div>
> +<div><img src="resources/exif-orientation-8-llo.jpg"><br>Rotated 90&deg; CW</div>
> +<br>
> +<div><img src="resources/exif-orientation-9-u.jpg"><br>Undefined (invalid value)</div>

Could this be a dumpAsText(true), maybe also getting the image dimensions in JS to check for rotation. Otherwise the text in the test will make it break with every OS rev.

How about another test that that uses images with EXIF as CSS background and content images to check for rotation?

> Source/WebCore/platform/graphics/BitmapImage.cpp:186
> +        m_sizeRespectingOrientation = m_source.size(true);

Boo for unreadable bools at call sites. Maybe use an enum for the argument?

> Source/WebCore/platform/graphics/BitmapImage.cpp:197
> +        m_sizeRespectingOrientation = m_source.size(true);

Ditto.

[Tim Horton]

Created attachment 135920 [details]
may have gone overboard with the enum, want to check EWS

This patch will enable image orientation a) On Mac b) for full-page images c) also for <img> tags if the setting WebKitShouldRespectImageOrientation is set. After talking to Simon and Ted they think that's the most reasonable scope to start with.

[Tim Horton]

Created attachment 135921 [details]
conflicts

[WebKit Review Bot]

Comment on attachment 135921 [details]
conflicts

Attachment 135921 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12335584

[Gustavo Noronha (kov)]

Comment on attachment 135921 [details]
conflicts

Attachment 135921 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12335591

[Tim Horton]

Created attachment 135999 [details]
patch

[Philippe Normand]

Comment on attachment 135999 [details]
patch

Attachment 135999 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12341827

[Tim Horton]

Created attachment 136000 [details]
patch

[Philippe Normand]

Comment on attachment 136000 [details]
patch

Attachment 136000 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12336874

[Tim Horton]

(In reply to comment #113)
> (From update of attachment 136000 [details])
> Attachment 136000 [details] did not pass gtk-ews (gtk):
> Output: http://queues.webkit.org/results/12336874

make[1]: execvp: /bin/bash: Argument list too long
make[1]: *** [libWebCore.la] Error 127
make[1]: *** Waiting for unfinished jobs....
make[1]: Leaving directory `/home/phil/WebKit/WebKitBuild/Release'
make: *** [all] Error 2

Seems like there's something wrong with the gtk buildbot.

[WebKit Review Bot]

Comment on attachment 136000 [details]
patch

Attachment 136000 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12340843

New failing tests:
fast/images/exif-orientation-css.html

[WebKit Review Bot]

Created attachment 136005 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[WebKit Review Bot]

Comment on attachment 136000 [details]
patch

Attachment 136000 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12264879

New failing tests:
fast/images/exif-orientation-css.html

[WebKit Review Bot]

Created attachment 136014 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Simon Fraser (smfr)]

Comment on attachment 136000 [details]
patch

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

> LayoutTests/platform/mac/fast/images/exif-orientation-css-expected.txt:14
> +Undefined (invalid value)

Should this be here?

> LayoutTests/platform/mac/fast/images/exif-orientation-expected.txt:14
> +Undefined (invalid value)

Ditto.

> Source/WebCore/platform/graphics/GraphicsContext.cpp:486
>          // FIXME: Should be InterpolationLow
>          setImageInterpolationQuality(InterpolationNone);

/aside Is there a bug for this FIXME?

> Source/WebCore/platform/graphics/ImageSource.cpp:109
> +    if (shouldRespectOrientation == RespectImageOrientation)
> +        notImplemented();

Could you explain this notImplemented() with a comment?

> Source/WebCore/platform/graphics/ImageSource.cpp:117
> +    if (shouldRespectOrientation == RespectImageOrientation)
> +        notImplemented();

Ditto.

> Source/WebCore/rendering/RenderObject.h:38
> +#include "Settings.h"

Why this include in the header?

> Source/WebCore/rendering/RenderObject.h:860
> +    RespectImageOrientationEnum shouldRespectImageOrientation() const;

Maybe this should be virtual, and have an override on RenderImage? I think the name is a bit too general too. It's not clear in what situations this is consulted.

[Tim Horton]

(In reply to comment #119)
> (From update of attachment 136000 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136000&action=review
> 
> > LayoutTests/platform/mac/fast/images/exif-orientation-css-expected.txt:14
> > +Undefined (invalid value)
> 
> Should this be here?
> 
> > LayoutTests/platform/mac/fast/images/exif-orientation-expected.txt:14
> > +Undefined (invalid value)
> 
> Ditto.

Yes to both, they are testing "undefined/invalid" EXIF orientation.

> > Source/WebCore/platform/graphics/GraphicsContext.cpp:486
> >          // FIXME: Should be InterpolationLow
> >          setImageInterpolationQuality(InterpolationNone);
> 
> /aside Is there a bug for this FIXME?

https://bugs.webkit.org/show_bug.cgi?id=49002

I'll add a reference to it there.

> > Source/WebCore/platform/graphics/ImageSource.cpp:109
> > +    if (shouldRespectOrientation == RespectImageOrientation)
> > +        notImplemented();
> 
> Could you explain this notImplemented() with a comment?
> 
> > Source/WebCore/platform/graphics/ImageSource.cpp:117
> > +    if (shouldRespectOrientation == RespectImageOrientation)
> > +        notImplemented();
> 
> Ditto.

Sure!

> > Source/WebCore/rendering/RenderObject.h:38
> > +#include "Settings.h"
> 
> Why this include in the header?

Leftovers, will fix.

> > Source/WebCore/rendering/RenderObject.h:860
> > +    RespectImageOrientationEnum shouldRespectImageOrientation() const;
> 
> Maybe this should be virtual, and have an override on RenderImage? I think the name is a bit too general too. It's not clear in what situations this is consulted.

Maybe Antti or Andreas or someone could clarify, but I've always been afraid to add new virtuals to RenderObject and others.

[Tim Horton]

Landed for ImageDocument and <img> in http://trac.webkit.org/changeset/113486

I'm not sure what we should do with this bug -- close it and file another for the discussion about turning it on for all images?

[Adele Peterson]

I think we should close this one, and open a new one for the remaining issue.

[Eric Seidel (no email)]

It's definitely time to retire this bug. :)  But now that it's a setting ports can turn it on themselves, no?

[Nico Weber]

Re comment 66: That 2nd orientation tag is from the tiff descriptor for the embedded thumbnail in that image. The block that contains the "wrong" orientation also contains "width = 320", "height = 240", and embedded jpeg data for a thumbnail.