Closed Bug 917595 Opened 6 years ago Closed 6 years ago

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

Categories

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

26 Branch
x86_64
Linux
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla28
Tracking Status
firefox26 - affected
firefox27 + verified
firefox28 --- verified

People

(Reporter: Yukikaze, Assigned: seth)

References

(Blocks 2 open bugs, )

Details

Attachments

(3 files, 3 obsolete files)

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?
Component: Untriaged → File Handling
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
This is caused by bug 298619. The scaling still uses the original image orientation.
Yes, the image is orientated according the EXIF data and scaled to the browser window.
Blocks: exif
Component: File Handling → Layout: Images
Product: Firefox → Core
Flags: needinfo?(seth)
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.
Taking this.
Assignee: nobody → seth
Flags: needinfo?(seth)
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
For some reason I never uploaded my patch for this bug.

Minimal try job here: https://tbpl.mozilla.org/?tree=Try&rev=3d8f63138f29
Attachment #813420 - Flags: review?(khuey)
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 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.
Attachment #813420 - Flags: review?(khuey)
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.
Attachment #825067 - Flags: review?(bugs)
Attachment #813420 - Attachment is obsolete: true
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.
Attachment #825067 - Flags: review?(bugs) → review-
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.
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.
Attachment #825708 - Flags: review?(bugs)
Attachment #825067 - Attachment is obsolete: true
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.
Attachment #825709 - Flags: review?(bugs)
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
Attachment #825708 - Flags: review?(bugs) → review+
Attachment #825709 - Flags: review?(bugs) → review+
Blocks: 935825
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
Sadface. Will investigate.
Try job for reftests here:

https://tbpl.mozilla.org/?tree=Try&rev=9e9eb2db719e
Flags: needinfo?(seth)
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.
Attachment #825709 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/cdccdeb67076
https://hg.mozilla.org/mozilla-central/rev/bf15e2032c38
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Depends on: 942004
(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?
Flags: needinfo?(seth)
... 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).
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.
(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.
Flags: needinfo?(seth)
Summary: Image aspect ratio not preserved while scaling → Image aspect ratio not preserved while scaling for images with an EXIF orientation set
(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 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.
Attachment #825708 - Flags: approval-mozilla-beta?
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.
Flags: needinfo?(akeybl)
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).
Flags: needinfo?(akeybl)
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.
Attachment #825708 - Flags: approval-mozilla-release?
(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 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.
Attachment #825708 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flagging for verification and some exploratory testing around this use case.
Keywords: verifyme
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 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.
Attachment #825708 - Flags: approval-mozilla-release? → approval-mozilla-release-
Duplicate of this bug: 969257
Status: RESOLVED → VERIFIED
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.
(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.
Blocks: 971368
Further discussion of the issue described in comment 38 can take place in bug 971368.
Product: Core → Core Graveyard
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.