917595 - Image aspect ratio not preserved while scaling for images with an EXIF orientation set

Status: VERIFIED FIXED

(Core :: Layout: Images, Video, and HTML Frames, defect)

Description

[Yuki Kaze]

Attachment: Image displaying the problem.

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release)
Build ID: 20130917030214

Steps to reproduce:

Open direct link to image larger than current browser resolution


Actual results:

Image while in the scaled down (zoomed out) mode is not preserving the correct aspect ratio


Expected results:

It should preserve aspect ratio?

Comment 1

[Yuki Kaze]

After further checking it seems to only happen to images created by my iPhone. Not sure what is causing it but it's only a problem in Firefox exif data of the image for anyone who wants it and would know what is wrong with Firefox that would cause this https://privatepaste.com/af5a67194a

Comment 2

[dagger.bugzilla]

This is caused by bug 298619. The scaling still uses the original image orientation.

Comment 3

[Loic]

Yes, the image is orientated according the EXIF data and scaled to the browser window.

Comment 4

[rsx11m]

Not surprisingly, seen on Windows too with the effect described in comment #2. Thus, the EXIF orientation has to be applied with respect to the max-width/max-height styles as well.

Comment 5

[Seth Fowler [:seth] [:s2h]]

Taking this.

Comment 6

[Seth Fowler [:seth] [:s2h]]

Attachment: Respect image-orientation in zoomed image documents.

For some reason I never uploaded my patch for this bug.

Minimal try job here: https://tbpl.mozilla.org/?tree=Try&rev=3d8f63138f29

Comment 7

[dagger.bugzilla]

That patch assumes image-orientation is always set to from-image, which isn't necessarily the case, e.g.:
data:text/html;charset=utf-8,<iframe src%3D"http%3A//farm4.staticflickr.com/3826/9028972457_6fa76877df_o.jpg">

(This might be separate bug material, but can this be done in such a way that the orientation or reoriented dimensions are available to JS? .naturalWidth/height still return the pre-oriented dimensions, for instance. I maintain Old Default Image Style, which replaces the image resizer with a Javascript version to work around a bug, and thus would quite like access to them. They'd also be useful for the Page Info dialog and perhaps for B2G.)

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 813420 [details] [diff] [review]
Respect image-orientation in zoomed image documents.

I don't think I'm the correct reviewer for this, sorry.

Comment 9

[Seth Fowler [:seth] [:s2h]]

Attachment: Respect image-orientation in zoomed image documents.

Comment 7 makes a great point that the original patch didn't handle the iframe case correctly, because it assumes that |image-orientation| always has the value |from-image|. This new version relies on nsImageFrame to determine the intrinsic size of the image, which will properly take style information into account and do the right thing in all cases.

After perusing the blame for ImageDocument.cpp, I've also changed the reviewer to smaug.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 825067 [details] [diff] [review]
Respect image-orientation in zoomed image documents.

nsresult
> ImageDocument::OnStopRequest(imgIRequest *aRequest,
>                              nsresult aStatus)
> {
>+  // mImageContent can be null if the document is already destroyed
>+  if (mImageContent) {
>+    if (NS_FAILED(aStatus) && mStringBundle) {
>+      nsAutoCString src;
>+      mDocumentURI->GetSpec(src);
>+      NS_ConvertUTF8toUTF16 srcString(src);
>+      const PRUnichar* formatString[] = { srcString.get() };
>+      nsXPIDLString errorMsg;
>+      NS_NAMED_LITERAL_STRING(str, "InvalidImage");
>+      mStringBundle->FormatStringFromName(str.get(), formatString, 1,
>+                                          getter_Copies(errorMsg));
>+
>+      mImageContent->SetAttr(kNameSpaceID_None, nsGkAtoms::alt, errorMsg, false);
>+    } else if (NS_SUCCEEDED(aStatus)) {
>+      // Pull an updated size from the content frame to account for any size
>+      // change due to e.g. EXIF orientation.
>+      nsIFrame* contentFrame = mImageContent->GetPrimaryFrame();
>+      IntrinsicSize size = contentFrame->GetIntrinsicSize();
contentFrame may very well be null here.

>+
>+      if (size.width.GetUnit() == eStyleUnit_Coord)
>+        mImageWidth = nsPresContext::AppUnitsToFloatCSSPixels(size.width.GetCoordValue());
>+      if (size.height.GetUnit() == eStyleUnit_Coord)
>+        mImageHeight = nsPresContext::AppUnitsToFloatCSSPixels(size.height.GetCoordValue());

Always {} with if


But why you need to go through layout?
Couldn't you get the same information from aRequest->GetImage()->intrinsicSize?
Event though the result is the same, it feels more correct since that way the code doesn't start to depend on layout.

some test would be really nice.

Comment 11

[Seth Fowler [:seth] [:s2h]]

Thanks for the review, Olli!

(In reply to Olli Pettay [:smaug] from comment #10)
> But why you need to go through layout?
> Couldn't you get the same information from
> aRequest->GetImage()->intrinsicSize?
> Event though the result is the same, it feels more correct since that way
> the code doesn't start to depend on layout.

No, the result isn't the same. (That's essentially what this bug is about.) I have to go through layout because I need to get the size after any |image-orientation| property (or any other hypothetical future CSS property affecting image dimensions) is applied. There's really no way around it.

Comment 12

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Respect image-orientation in zoomed image documents.

Here's an updated version of the patch. I addressed the review comments, and also restructed the patch a bit because I realized doing this work in OnStopRequest was racy. I now update the size in a listener for the HTMLImageElement's 'load' event, which eliminates the racy behavior.

Comment 13

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Reftests for zoomed image documents with EXIF orientations.

Here are reftests for both the top-level image document case and the case where the document is embedded in an iframe. They verify that the behavior in each case matches what our stylesheets request - in top-level image documents, |image-orientation: from-image| makes us take EXIF orientation into account, while in other image documents we do not. The top-level case fails without the patch in part 1 and passes with it applied.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 825708 [details] [diff] [review]
(Part 1) - Respect image-orientation in zoomed image documents.


>       target = do_QueryInterface(mImageContent);
>+      target->AddEventListener(NS_LITERAL_STRING("load"), this, false);
>       target->AddEventListener(NS_LITERAL_STRING("click"), this, false);
Hmm, both these should use system event group.
Could you file a followup about that. Assign to me.

>     }
> 
>     target = do_QueryInterface(aScriptGlobalObject);
>     target->AddEventListener(NS_LITERAL_STRING("resize"), this, false);
>     target->AddEventListener(NS_LITERAL_STRING("keypress"), this, false);
Ah, these too should use system event group

Comment 15

[Seth Fowler [:seth] [:s2h]]

Thanks for the review, Olli! I've filed the followup you requested as bug 935825.

Pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/0dc2e0f4fbcc
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe2be5137635

Comment 16

[Phil Ringnalda (:philor)]

While OS X does in fact enjoy your reftests, neither Linux nor Windows do, finding that the part in the center where the colors come together make their heads swim.

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/0ee1f8e16b54

Comment 17

[Seth Fowler [:seth] [:s2h]]

Sadface. Will investigate.

Comment 18

[Seth Fowler [:seth] [:s2h]]

Try job for reftests here:

https://tbpl.mozilla.org/?tree=Try&rev=9e9eb2db719e

Comment 19

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Reftests for zoomed image documents with EXIF orientations.

I don't think I can do better than make the tests fuzzier in this case, unfortunately, since I have to test JPEGs. There are surprisingly large 'fuzziness' differences between platforms, although they're undetectable to the naked eye.

Comment 20

[Seth Fowler [:seth] [:s2h]]

Pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/cdccdeb67076
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf15e2032c38

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cdccdeb67076
https://hg.mozilla.org/mozilla-central/rev/bf15e2032c38

Comment 22

[rsx11m]

(Quoting GreenRep from bug 298619 comment #107)
> testing with Firefox 26.0 on Linux (but also tested on Win7), also in a
> fresh profile: firefox rotates
> http://farm4.staticflickr.com/3826/9028972457_6fa76877df_o.jpg as expected
> but gets the *aspect ratio wrong*! It is still shown in landscape format
> (and scaled to 25 %). A click on the image to zoom in to 100% shows it
> correctly. A zoom out to fit to screen makes the aspect ratio wrong again. 
> Interestingly something similar happens with Chromium 31.0: wrong aspect
> ratio, zoom in: correct, zoom out: correct aspect ratio (so only the inital
> load is wrong in Chromium).
> 
> I could not find many other images which behave similarly.  
> http://www.kotikone.fi/kuukkeli/Antin_kuvakalleria/anti_clock_wise_90_web.
> jpg and
> http://www.kotikone.fi/kuukkeli/Antin_kuvakalleria/clock_wise_90_web.jpg get
> fat/wrong if the window is made small enough to trigger Firefox zoom feature.
> 
> After searching a bit in bugzilla it seems that this problem was known (bug
> 917595) and is fixed for Firefox 28. Hmmm, a bit unfortunate that the
> rotation by EXIF is already switched on in Firefox 26, isn't it?

Unfortunately the patch here didn't land on the branches, thus the bug is exposed in yesterday's Firefox 26.0 release. Could you nominate this for mozilla-beta so that it can hit the releases with 27.0 already, if not mozilla-release?

Comment 23

[GreenRep]

... note that this bug seems to exposed (if I understand correctly) for many of the images for which fixing bug 298619 was thought to be a fix: high res images from digital photo cameras. So: yes, they may now be displayed in the correct orientation (if the EXIF info is correct) but the aspect ratio is wrong (as typically non-resized digital photos are bigger than the usual browser window).

Comment 24

[GreenRep]

Shouldn't "EXIF" and "orientation" be mentioned in this bug's title? (see comment 1) Suggestion: Image aspect ratio not preserved while scaling for images rotated by EXIF orientation tag.  In fact I would have recognized then that I have just found the exact duplicate of this bug.

Comment 25

[Seth Fowler [:seth] [:s2h]]

(In reply to GreenRep from comment #24)
> Shouldn't "EXIF" and "orientation" be mentioned in this bug's title? (see
> comment 1) Suggestion: Image aspect ratio not preserved while scaling for
> images rotated by EXIF orientation tag.  In fact I would have recognized
> then that I have just found the exact duplicate of this bug.

That's a good idea; I've updated the bug title.

Comment 26

[Seth Fowler [:seth] [:s2h]]

(In reply to rsx11m from comment #22)
> Unfortunately the patch here didn't land on the branches, thus the bug is
> exposed in yesterday's Firefox 26.0 release. Could you nominate this for
> mozilla-beta so that it can hit the releases with 27.0 already, if not
> mozilla-release?

Yes, this is an unfortunate oversight. I'll nominate this for 27.

Comment 27

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 825708 [details] [diff] [review]
(Part 1) - Respect image-orientation in zoomed image documents.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 298619
User impact if declined: Image documents for images with an EXIF orientation set may be displayed with the wrong aspect ratio. (Depending on the orientation.)
Testing completed (on m-c, etc.): Rode the trains to Aurora already.
Risk to taking this patch (and alternatives if risky): Very little risk.
String or IDL/UUID changes made by this patch: None.

Comment 28

[Seth Fowler [:seth] [:s2h]]

Alex, I'm not sure how to express this via our flag system, but if we do a point release for FF 26 I'd like to request that this get uplifted for it.

Comment 29

[rsx11m]

Comment on attachment 825708 [details] [diff] [review]
(Part 1) - Respect image-orientation in zoomed image documents.

You'd just have to set approval-mozilla-release? for this patch, unless there is a specific release branch already (doesn't look like that).

Comment 30

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 825708 [details] [diff] [review]
(Part 1) - Respect image-orientation in zoomed image documents.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 298619
User impact if declined: Image documents for images with an EXIF orientation set may be displayed with the wrong aspect ratio. (Depending on the orientation.)
Testing completed (on m-c, etc.): Rode the trains to Aurora already.
Risk to taking this patch (and alternatives if risky): Very little risk.
String or IDL/UUID changes made by this patch: None.

Comment 31

[Seth Fowler [:seth] [:s2h]]

(In reply to rsx11m from comment #29)
> You'd just have to set approval-mozilla-release? for this patch, unless
> there is a specific release branch already (doesn't look like that).

Alright, did so. Thanks.

Comment 32

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 825708 [details] [diff] [review]
(Part 1) - Respect image-orientation in zoomed image documents.

Seth, I'm not sure if we'd consider this on a chemspill but let's get it to Beta at least and I'm going to leave a ? on tracking 26 so it does come up in queries should there be a call for a dot release.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/bdc531daf211
https://hg.mozilla.org/releases/mozilla-beta/rev/d4345aedf36e

Comment 34

[u279076]

Flagging for verification and some exploratory testing around this use case.

Comment 35

[Bogdan Maris, Desktop QA]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:28.0) Gecko/20100101 Firefox/28.0
Mozilla/5.0 (X11; Linux x86_64; rv:28.0) Gecko/20100101 Firefox/28.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0

Verified as fixed on latest Aurora 28.0a2 (buildID: 20131223004002) and Firefox 27 beta 2 (buildID: 20131216183647).
Also did some exploratory testing using some scenarios that can be found in this etherpad https://etherpad.mozilla.org/firefox28-0a2-EXIF.
Based on the above, marking this as verified fixed on 27,28.

Comment 36

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 825708 [details] [diff] [review]
(Part 1) - Respect image-orientation in zoomed image documents.

This isn't a chemspill driver, so closing out the flags for 26, it will ship with 27.

Comment 38

[GreenRep]

Well, not fully fixed (tested with FF27). 

firefox rotates  http://farm4.staticflickr.com/3826/9028972457_6fa76877df_o.jpg correctly, but still(!) gets the *aspect ratio wrong* while loading (without cache - ctrl+f5). Well and only shows the lower right part (45°) of the image while loading. Very strange.

Comment 39

[Seth Fowler [:seth] [:s2h]]

(In reply to GreenRep from comment #38)
> Well, not fully fixed (tested with FF27). 
> 
> firefox rotates 
> http://farm4.staticflickr.com/3826/9028972457_6fa76877df_o.jpg correctly,
> but still(!) gets the *aspect ratio wrong* while loading (without cache -
> ctrl+f5). Well and only shows the lower right part (45°) of the image while
> loading. Very strange.

That's unfortunate, I agree. It's an artifact of the way this is implemented: to avoid a race condition, we don't update the size of the image until the image's load event fires. That means that you will see the image rotated correctly but with the unrotated size while the image is loading. (Which results in the aspect ratio being wrong.)

I'd like to get this fixed, but this bug isn't the place to do it. I'll file a new bug based upon your comment above.

Comment 40

[Seth Fowler [:seth] [:s2h]]

Further discussion of the issue described in comment 38 can take place in bug 971368.