997604 - Image is incorrectly updated and displayed during the sequential decoding if there is Orientation EXIF tag (0x112)

Status: RESOLVED FIXED

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

Description

[Ted Kapustin]

Attachment: RotatedImageBug.zip

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/34.0.1847.116 Safari/537.36

Steps to reproduce:

If a JPEG image has EXIF Orientation tag (0x112, decimal 274), and the desired orientation involves 90 or 270 degrees rotation (so the vertical and horizontal dimensions are swapped), the image is not shrunk and updated properly (apparently, the rotation is not taken into account, and horizontal and vertical directions are confused). this takes place only if the image is shrunk to fit in the client part of the window. I discovered this bug when implementing a support for JPEG-XR image format in Firefox (bug 500500). I tested Firefox on JPEG images that have the Orientation tag to make sure the bug is in Firefox, not in my code.
When the image is not shrunk (i.e. the window is big enough to show the whole image without shrinking it), everything works fine.


Actual results:

When a JPEG image with Orientation tag having value 5 is being decoded sequentially, and the window is not big enough for displaying the whole image, the rows of the shrunk image appear in wrong places, and the image that appears has wrong proportions. Rows of the image appear as stairs (see the attached screenshot), from top to bottom, and instead of vertical rows (the image is rotated!) horizontal stripes are drawn on the screen.
If the image is not stretched (when the window is big enough), vertical lines of image are drawn, from left to right, and the proportions are correct.


Expected results:

The rotated image should appear (as it is decoded sequentially) the same, no matter whether it is shrunk or not. Apparently, the behavior described above is due to a bug.
By the way, Internet Explorer does not change the orientation of images that have EXIF tag 274. Chrome displays such images correctly as they are being decoded.
The attached ZIP file contains the source image and two screenshots. The forst screenshot was made while the image was being downloaded and decoded (I used a an HTTP server application that imitated slow resource downloading). the second screenshot shows the same image after it has been completely downloaded.
The bug is present in both v.28 release and in v.31 Nightly I downloaded on April 10, 2014

Comment 1

[Ted Kapustin]

So it should be relatively easy to locate the code that causes the bug: when the image is scaled (because it does not fit in window), the bug manifests itself; when the window is big enough to contain the entire image, everything works fine.
I must have to add that my build has WebGL disabled, if it matters.
The bug must be reproduced with slow Internet connection (or using an HTTP server that simulates slow resource downloading), so bands of decoded lines are drawn, otherwise the image will be downloaded, decoded and drawn too fast to notice the glitches.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Hmm.  Seth, does nsImageFrame::SourceRectToDest need to take orientation into account?

Comment 3

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

Investigating now.

Comment 4

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

(In reply to Boris Zbarsky [:bz] from comment #2)
> Hmm.  Seth, does nsImageFrame::SourceRectToDest need to take orientation
> into account?

After giving this a quick look, it seems like the *real* problem is that the invalidation rect sent for the FRAME_UPDATE (aka OnDataAvailable) event does not respect the orientation. (There may be a second problem, but I think this is the primary thing.)

This should be fairly easy to fix in this case, because the decoder has the information available to do it right. This might also be an issue for other kinds of ImageWrappers that the decoder can't know about.

Comment 5

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

This is another bug that is potentially related to bug 942004, so I'll go ahead and make that connection explicit.

Patching incoming shortly.

Comment 6

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

Try job for the incoming patch here:

https://tbpl.mozilla.org/?tree=Try&rev=93c5944b1604

Comment 7

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

Attachment: (Part 1) - Add invalidation rect support to ImageWrappers, and use it in nsImageFrame

So it turns out that there are two problems here.

Problem one is the "stairstep" effect visible in Ted's screenshots. This is cause by the invalidation rect sent by the FRAME_UPDATE events being in the wrong coordinate system. (The invalidation rect is always in the space of the underlying 'real' image that's being decoded, but we need it to be in the space of the OrientedImage that wraps it.)

More elegant solutions to this that allow ImageWrappers to transform all events are possible, but since this seems to be only the situation where we have a problem in practice, I've gone for a targeted patch that adds a new method to the imgIContainer interface, allowing the caller to transform an invalidation rect in "decode space" into the corresponding "image space" rect. I've implemented this method everywhere that needs it implemented. (We don't actually use it for ClippedImage, but the implementation is trivial and if we ever start using ClippedImage this way it'd be preferable if it "just worked".)

There's only one caller that needs to use this method, nsImageFrame, since everyone else that handles FRAME_UPDATE events just invalidates themselves entirely. (Which is always safe.)

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8441114 [details] [diff] [review]
(Part 1) - Add invalidation rect support to ImageWrappers, and use it in nsImageFrame

>+native TempRefImgIContainer(already_AddRefed<imgIContainer>);

Seems to be unused?

sr=me

Comment 9

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

(In reply to Boris Zbarsky [:bz] from comment #8)
> >+native TempRefImgIContainer(already_AddRefed<imgIContainer>);
> 
> Seems to be unused?

So it is! Will remove from the final patch. Thanks for the quick review!

Comment 10

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

Attachment: (Part 2) - Update ImageDocument size from layout during the loading process

I mentioned in comment 7 that there were two problems, and this patch fixes the second one. The issue is that nsImageDocument cannot rely on its nsImageFrame being constructed when it gets the SIZE_AVAILABLE event, and right now we don't even try: we wait to call UpdateSizeFromLayout (which is where the size change from 'image-orientation' gets applied) until the document load event. That means that for slow-loading images, the image will be presented with the wrong size (and hence the wrong aspect ratio) until it is loaded completely.

This patch makes us try much harder to get the updated size from layout. First of all, we call UpdateSizeFromLayout in the SIZE_AVAILABLE event handler, which often succeeds. In addition, every time we get a FRAME_UPDATE event, we try again, because if we're getting FRAME_UPDATE events then that means the user is staring at a slow-loading image, and we should try as hard as possible to present it correctly. This will almost certainly succeed very quickly even if we failed in the SIZE_AVAILABLE event handler. After it succeeds the first time, later calls are redundant, but also aren't very costly as we aren't actually changing the size and so we bail out early.

This patch, combined with part 1, makes us display images with the correct orientation *and* aspect ratio, even while loading.

Comment 11

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

Comment on attachment 8441125 [details] [diff] [review]
(Part 2) - Update ImageDocument size from layout during the loading process

As far as I see the change to Notify may lead to
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsContentUtils.cpp?rev=9a5636240f6c#3619 to fire, since
the Notify is called with nsAutoScriptBlocker on stack and
UpdateSizeFromLayout may end up calling stuff that causes mutation events to fire
(which means any random script may run).

And ImageDocument::OnStartContainer seems to use scriptrunner for other
stuff, so you should use script runner also for updating size.
See http://hg.mozilla.org/mozilla-central/rev/7e93778b4b14

Comment 12

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

Thanks for the review. Will upload a new version of the patch shortly.

Comment 13

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

Attachment: (Part 1) - Add invalidation rect support to ImageWrappers, and use it in nsImageFrame

Address review comments and remove a wrong assertion.

Comment 14

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

Attachment: (Part 2) - Update ImageDocument size from layout during the loading process

Addressed review comments by running UpdateSizeFromLayout asynchronously.

Comment 15

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

The updated patch looks good locally. Try job here:

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

Comment 16

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

Comment on attachment 8442469 [details] [diff] [review]
(Part 2) - Update ImageDocument size from layout during the loading process


>+  if (aType == imgINotificationObserver::FRAME_UPDATE) {
>+    // The image is decoding slowly enough that we're getting individual frame
>+    // updates. Try to get an updated image size from layout so the user doesn't
>+    // potentially stare at a distorted image while it loads.
>+    // (We need to do this asynchronously since we're within an nsAutoScriptBlocker.)
>+    nsCOMPtr<nsIRunnable> runnable =
>+      NS_NewRunnableMethod(this, &ImageDocument::UpdateSizeFromLayout);
>+    nsContentUtils::AddScriptRunner(runnable);
>+  }
Could you explain what guarantees that layout has been updated at this point?
Why would GetIntrinsicSize return something new in UpdateSizeFromLayout.
Does InvalidateFrame call in nsImageFrame::OnDataAvailable end up updating intrinsic size or what?

Comment 17

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

(In reply to Olli Pettay [:smaug] from comment #16)
> Could you explain what guarantees that layout has been updated at this point?

Nothing guarantees it. Everything is async here.

> Why would GetIntrinsicSize return something new in UpdateSizeFromLayout.
> Does InvalidateFrame call in nsImageFrame::OnDataAvailable end up updating
> intrinsic size or what?

It can happen in two main ways:

(1) We can end up rebuilding the nsImageFrame. The imgIContainer will replay its notifications to the newly rebuilt nsImageFrame, so it will get SIZE_AVAILABLE again, but by then style information generally is available.

This is actually the case that we end up hitting right now that makes this patch work. The nsImageFrame gets rebuilt because the character set changes, and at that time style information is available, so we need the new (correct) intrinsic size value.

(2) We can update the intrinsic size in nsImageFrame::DoSetStyleContext. This will cover both cases where the style information wasn't ready yet, and cases where it got changed dynamically by JS.

This case doesn't work yet, but I'm going to upload a patch shortly to fix it in bug 997010. I originally had that code included in the patches for this bug, but I separated it out after I learned that there was another bug about that issue and that that code isn't actually needed to fix this bug.

If you'd prefer, we can make bug 997010 block this bug.

Comment 18

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

(In reply to Seth Fowler [:seth] from comment #17)
> and at that time style information is available, so we need the new

That should have been "so we get the new"

Comment 19

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

(For now I'm actually going to set the dependency the other way, since otherwise I'll have to rebase this patch.)

Comment 20

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

So why isn't handling SIZE_AVAILABLE enough? Or is FRAME_UPDATE just some random state when we try
whether we might have new intrinsic size?

Comment 21

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

(In reply to Olli Pettay [:smaug] from comment #20)
> So why isn't handling SIZE_AVAILABLE enough?

Because style information is not always available then.

Imagine we have a fairly small image that we're able to decode in one spin of the decoder. Then the following event interleavings are possible:

1. (style available) (SIZE_AVAILABLE event) (document load event)
2. (SIZE_AVAILABLE) (style available) (document load event)

I ignore the issue of when the nsImageFrame is constructed or reconstructed, which is another source of complexity.

For case 1, UpdateSizeFromLayout will succeed in the SIZE_AVAILABLE event handler, which is ideal, since that means there's no change we'll paint the image with the wrong aspect ratio.

For case 2, UpdateSizeFromLayout will fail until the document load event handler. This is less ideal since it's possible that we paint before that arrives, but at least it ensures that once the image is completely loaded, we'll paint with the right aspect ratio. Since we decoded this image in one spin, the user almost certainly won't notice any kind of problem.

Now image we have a much larger image coming down over a slow connection. This is exactly the situation that we're dealing with in this bug. In this case, it could take many spins of the decoder to completely decode the image. Here are a few of the many possible event interleavings:

3. (style available) (SIZE_AVAILABLE event) (FRAME_UPDATE event) (FRAME_UPDATE event) ... (document load event)
4. (SIZE_AVAILABLE event) (style available) (FRAME_UPDATE event) (FRAME_UPDATE event) ... (document load event)
5. (SIZE_AVAILABLE event) (FRAME_UPDATE event) (style available) (FRAME_UPDATE event) ... (document load event)
6. (SIZE_AVAILABLE event) (FRAME_UPDATE event) (FRAME_UPDATE event) ... (style available) (document load event)

Case 3 is just like case 1, above. UpdateSizeFromLayout will succeed in the SIZE_AVAILABLE event handler.

In case 4 and 5, UpdateSizeFromLayout will fail in the SIZE_AVAILABLE event handler, but it will succeed in one of the FRAME_UPDATE event handlers. FRAME_UPDATE is *not* chosen randomly. I tried to explain this in the source comments in the patch, but we get FRAME_UPDATE events precisely the decoder needs more than one spin to decode the image. Those are the cases where an image slowly appears on the screen as more of it is downloaded and decoded. On a slow connection, the user could be staring at this slowly appearing image for *minutes*. It's critical that we call UpdateSizeFromLayout to ensure that we get the correct size as soon as style information is available, or else the user could be staring for minutes at an image that is displayed with the wrong aspect ratio.

There is an unbounded number of cases like cases 4 and 5. Although very unlikely, it's theoretically possible that style information is not available during any of the FRAME_UPDATE event handlers. Even in this situation, then, we could end up in case 6, which is very much like case 2: we don't have style information available until the last possible moment, in the document load event handler. We're guaranteed to have it there because the load event won't fire until all stylesheets are loaded. So UpdateSizeFromLayout should always succeed in the document load event handler, guaranteeing that we get the correct size sooner or later.

Hopefully this made things clear. FRAME_UPDATE is not chosen randomly, and calling UpdateSizeFromLayout in the FRAME_UPDATE handler is *critical* to this patch.

Comment 22

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

Comment on attachment 8442469 [details] [diff] [review]
(Part 2) - Update ImageDocument size from layout during the loading process

Review of attachment 8442469 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/html/document/src/ImageDocument.cpp
@@ +522,5 @@
>  
>  nsresult
>  ImageDocument::OnStartContainer(imgIRequest* aRequest, imgIContainer* aImage)
>  {
> +  // Styles may not yet been applied. Default to the image's intrinsic size.

Should be "may not yet have been". Will fix in the final version of the patch.

Comment 23

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

So in case of 
5. (SIZE_AVAILABLE event) (FRAME_UPDATE event) (style available) (FRAME_UPDATE event) ... (document load event)

What happens if we have a refreshdriver tick doing painting right after (style available) ?

Comment 24

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

Comment on attachment 8442469 [details] [diff] [review]
(Part 2) - Update ImageDocument size from layout during the loading process

OK, so this doesn't really guarantee UpdateSizeFromLayout is called at right times. As discussed on IRC, there are other ways to do it, and ImageFrame
probably needs to notify ImageDoc when its state changes.

Comment 25

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

We discussed this at length on IRC and decided that it'd be cleaner to have some way to detect image-orientation changes directly. At the moment my proposal is to do this using a custom event. I'm unsetting the review flag until I get a chance to rework the patch with that in mind.

Comment 26

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

So after more thought about this and discussion with others, I've concluded that there is a better solution. The existing implementation of ImageDocument relies upon absolute positioning and manually setting the image's width and height to achieve centering and shrink-to-fit. With flexbox (which permits us to center both horizontally and vertically with simple CSS) and viewport units (which make implementing shrink-to-fit very easy) we actually don't need this level of complexity at the C++ level anymore.

I'm going to file a new bug (blocking this one) which will move more of the implementation of the ImageDocument into CSS and out of C++.

Comment 27

[Daniel Holbert [:dholbert]]

[marking as NEW instead of UNCONFIRMED, per IRC]

Comment 29

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

I'm going to change the dependencies between this bug, bug 997010, and bug 1029285. It turns out that we have to wait for a bug fix in the flexbox code before bug 1029285 can proceed. However, this bug and bug 997010 are still useful independently.

Landing the patch in this bug will resolve the invalidation rect issue that causes a stairstep effect when images with an EXIF orientation applied load on slow connections.

Landing bug 997010 will make us recognize dynamic changes in the |image-orientation| CSS property.

There will still be a race condition that could cause images to be displayed with the wrong aspect ratio until they're completely loaded. This will eventually be resolved by bug 1029285, when it's ready.

Comment 30

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

Pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/8aec56ad356a

Comment 31

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/8aec56ad356a