1068986 - [gallery][camera] fix image decoding problems in MediaFrame

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Gallery, defect)

Description

[David Flanagan [:djf]]

In gallery and camera, we use MediaFrame (shared/js/media/media_frame.js) for displaying photos.  When the photos are large and do not have decent EXIF previews, it takes a while for the image to decode and the visuals while this is happening are not pleasing.

There are two specific issues:

1) When gecko needs to display a large image in an img element, it decodes part of the image synchronously, and then defers the rest of the decode and performs it synchronously. MediaFrame uses an img element, and for large images, this two-pass decode is visible to the user: part of the image appears, then there is a very noticeable pause and then the rest of the image appears. It is a bad effect, and has been reported as a bug a number of times.

2) In the Gallery (but not camera) we attempt to have images decoded and ready to be displayed when the user swipes left or right. This used to work. But Gecko has gotten smart enough to not decode img elements when they are not visible, so now, when images don't have an EXIF preview, there is a noticeable delay before they appear when we swipe left or right. This is a regression and has also been reported as a bug by partners.

The solution to both of these problems may be to switch from using <img> to background-image.  background-images apparently won't display until full decoded, which should prevent the two-phase decode issue. And gecko isn't smart enough to prevent the decode of off-screen background images, so this change should solve issue 2 as well.

It is unclear how difficult it will be to convert MediaFrame to use background-image, but it seems like something we should try to do.

Comment 1

[David Flanagan [:djf]]

Here's a transcript of a really helpful IRC conversation with some of the #gfx team that should help with this bug.

djf: jrmuizel: hi. A couple of image questions for you related to gallery, if you don't mind...
[2:49pm] jrmuizel: djf: go for it
[2:50pm] djf: jrmuizel: 1) when we decode full-size photos for display on the screen, gecko often renders part of the image, then pauses, then renders the rest. Its a janky effect and people keep filing bugs about it. Any idea why that happens or how I could avoid it?
[2:51pm] jrmuizel: djf: we do a sync decode for the first part and then do async decode with the rest
[2:51pm] djf: 2) now that gecko is smart enough to not decode offscreen images, I need a way to force it to decode offscreen images. In gallery, I need to be able to swipe an image in from offscreen and have it ready to display. But I've got a regression now where it takes a whlie to display becasue it is not decoded yet (I think). Any tricks for forcing an image to decode even when offscreen?
[2:51pm] jrmuizel: djf: this way we're not blocking the main thread as long
[2:51pm] sotaro left the chat room. (Input/output error)
[2:51pm] jrmuizel: djf: tn is probably a better person to ask about that
[2:52pm] jrmuizel: djf: you could try putting it in a will-change layer
[2:52pm] jrmuizel: djf: intuitively that's what should work, if it doesn't it might be worth fixing
[2:53pm] djf: jrmuizel: will-change:transform?
[2:54pm] jrmuizel: djf: yes
[2:54pm] jrmuizel: djf: I assume you're moving the images around with transform?
[2:54pm] djf: jrmuizel: so the two-phase decode is an intentional thing and not a bug. I'm thinking that I could avoid it by creating a small canvas and doing a drawImage() to it to force a sync decode and then show the image.  I'm assuming that will block the main thread, but would fix the visual bug.
[2:54pm] jet joined the chat room.
[2:54pm] jrmuizel: djf: please don't do that
[2:55pm] jrmuizel: djf: we can probably do something about the two-phase decode
[2:55pm] djf: jrmuizel: yes, I assume I'm using a transform, but it has been a long time. Is gecko smart enough to not decode an image that is offscreen because of a transform: translate?
[2:55pm] tn: djf, thats the main difference between <img> and background images. <img> display partially available images
[2:56pm] jgilbert left the chat room. (Ping timeout)
[2:56pm] tn: djf, if the image is fixed pos and just below the viewport i think we'd decode it (because we decode thing just outside the viewport)
[2:56pm] djf: tn: ah, so if I switched to using background-image I could solve my partial display bug and probably also the offscreen decode bug?
[2:56pm] djf: tn: currently it is off to the side because the gallery does sideways swipes
[2:56pm] jrmuizel: djf: can you use a div with background:url() and set will-change on the ones that are to the right and left of the currently displayed image
[2:57pm] juanb joined the chat room.
[2:57pm] tn: djf, background images also don't have any of the smarts to not decode not visible things 
[2:57pm] jrmuizel: and the currently displayed image of course
[2:57pm] milan: djf: just keep an eye on memory usage, whether you're using will-change or otherwise increasing the amount of decoded images...
[2:57pm] milan left the chat room. (Quit: )
[2:58pm] jrmuizel: djf: it sounds like background: image will cause us to not use image layers anymore
[2:58pm] jrmuizel: djf: which is not good
[2:58pm] tn: djf, the "visible" part of decoding visible images isn't super smart. if the image is within the viewport of the page (even if it's covered by a thousand opaque things) it's considered visible. likewise if we have to draw it for any reason
[2:58pm] juanb left the chat room. (Ping timeout)
[2:59pm] jrmuizel: djf: or maybe they will
[2:59pm] djf: tn: so if I put it right below the visible part of the screen I can force it to decode, then move it to the side right when I want to swipe it in, maybe?
[2:59pm] tn: background images can be layerized, there is code for it, not sure how often it is activated
[3:00pm] mstange: any time a non-clipped, non-repeating image is alone in its layer
[3:00pm] jrmuizel: djf: so it might work well then
[3:00pm] tn: djf, i think that would work. assuming the page has some amount of possible up/down scrolling
[3:00pm] djf: mstange: what does "non-clipped" mean?  If the user has zoomed in on a photo, the div with background-image applied will be bigger than the screen
[3:01pm] kamidphish joined the chat room.
[3:01pm] mstange: djf: it's about whether the image fits into the background area of the element
[3:01pm] djf: tn: no, no up/down scrolling in gallery at all.
[3:01pm] kats is now known as kats|away.
[3:01pm] mstange: djf: if the element is too small or the image is moved outside using background-position, then it's clipped
[3:01pm] BenWa: With my patch we should try harder to to build image layers
[3:02pm] mstange: djf: if the whole element is zoomed so that the background area is also bigger than the screen, you should still get an image layer
[3:02pm] mstange: djf: it's definitely worth testing, though
[3:02pm] djf: mstange: okay,  I think I can do that.
[3:02pm] BenWa: if the image is scaled up to fit the background we might not layerize that
[3:03pm] BenWa: Right now the code is fairly restricted ATM
[3:03pm] djf: mstange: if I do this, what am I looking for? I need to confirm in the layer tree that I'm getting an image layer instead of something else?
[3:03pm] BenWa: you’d want to test that if you plan on using that
[3:03pm] mstange: djf: exactly, you'll get an image layer instead of a thebes layer
[3:03pm] djf: And what are the consequences if it is not an image layer? Worse frame rate when panning the image?
[3:04pm] mstange: djf: if panning the image only changes a transform, then panning should be just as fast
[3:04pm] mstange: djf: but the initial paint of the image into the thebeslayer will be slower if you don't have an image layer
[3:04pm] mstange: djf: because resizing images on the CPU is slower than on the GPU in the compositor
[3:05pm] tn: djf, that probably won't work then. could you just put a (1x1) copy of the images on the visible page and put it behind something opaque? gross but would work
[3:05pm] gal left the chat room. (Quit: gal)
[3:06pm] djf: tn: I'll try that if this background-image approach doesn't solve the problem.
[3:06pm] pcwalton left the chat room. (Quit: pcwalton)
[3:06pm] jcarpente joined the chat room.
[3:07pm] tn: djf, okay, background images are going to have another set of problems, in that we are fairly dumb about decoding them
[3:07pm] jcarpenter left the chat room. (Ping timeout)
[3:07pm] jcarpenter joined the chat room.
[3:08pm] jcarpente left the chat room. (Ping timeout)
[3:08pm] djf: tn: I thought we were dumb in the right way, in my case, since they always decode, and since they don't display until fully decoded
[3:08pm] juanb joined the chat room.
[3:08pm] djf: tn: I'm never going to have more than 3 at a time, and they will be smallish images, either using an EXIF preview or a #-moz-samplesize downsampled image.
[3:08pm] tn: djf, yes, but that might cause images you don't want to be decoded to get decoded too, increasing mem usage potential. something to be aware of
[3:09pm] tn: djf, okay, that should work

Comment 2

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

Hi David -

Just wonder do you have a ETA of this modification? As you already know some partner's issues are blocked by this one, so it would be nice if they can have a rough idea about when this modification might be done such that they can plan ahead accordingly.

Thanks 

Vance

Comment 3

[David Flanagan [:djf]]

Vance,

The fix I've described here is still speculative so I'm not sure it will solve the problem. And until I start working on it, I'm not sure how much work will be involved. So if I tried to give an ETA at this point, I'd just be making something up.

I assume that if I can fix this, our partner will want to uplift it, but since neither of the bugs that depend on this one are marked as blockers I'm not sure how high the priority for this work is.

Comment 4

[David Flanagan [:djf]]

I've spent some time working on this bug and have a WIP patch. It seems to be working quite well, and the trickiest part (zooming in) was much easier than I feared. If I work on this tomorrow, I think I can have a patch ready for review by the end of the day on the 19th.

Comment 5

[David Flanagan [:djf]]

Attachment: link to patch on github

This patch seems to be working, and I think it is ready for review.

There's a chance that when I look at it in the morning I'll find bugs or simplifications that I'd like to make, but I'm hoping that it is just done. There are still some comments to myself in the code right now, and I'll need to remove them, but they might actually help with your review.

Justin: this isn't marked as a blocker, but the TAM for the Madai project thinks it will become one if we don't get it fixed soon, so there is some urgency on this. I'm asking for your review since this is code you'll probably want to be familiar with for your work on Gallery. But if you're swamped with camera bugs, I could probably get Jim or Punam to take the review instead.

What I've done here is convert MediaFrame to use background-image instead of an <img> element. There were two goals here:

1) to prevent the jerky partial decode, pause, full decode display that we get with <img>.

2) To allow gecko to decode the images for offscreen media frames so that they are ready to display when the user swipes them in. This used to work for <img> elements but then gecko got smarter and stoped decoding the offscreen img elements.

In addition to these two benefits I started out going for, this switch turns out to have a couple of surprise benefits:

3) it becomes much easier to transition from a preview image to the full size image becasue the CSS background-image property allows us to specify multiple images. To transition to the full-size image, I just put the full url in front of the preview url in the background-image delcaration. That overlaid image remains transparent until it is decoded and then it hides the old one. It seems to work beautifully 

4) Because the full-size image just overlays the preview image when it is ready, the app feels a lot more responsive when loading full images. The old code depended on a timer to hide the preview and show the fullsize image. Now we just get the full image as fast as gecko and display it, and it is a noticeable change.

I'm happy that this patch removes more code than it adds. And a lot of the new lines are comments. But this still isn't a trivial patch and I hope you're able to read the code carefully to look for errors.  (There is lots of refactoring I'd like to do on MediaFrame, but I tried hard to avoid doing it in this patch.)

I also hope to put together a testing plan and a set of test images (big, small, jpeg, png, preview, no preview, exif rotation, etc.) to really exercise this new version of MediaFrame carefully.

Comment 6

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

(In reply to David Flanagan [:djf] from comment #5)
> Created attachment 8492035 [details] [review]
> link to patch on github
> 
> This patch seems to be working, and I think it is ready for review.
> 
> There's a chance that when I look at it in the morning I'll find bugs or
> simplifications that I'd like to make, but I'm hoping that it is just done.
> There are still some comments to myself in the code right now, and I'll need
> to remove them, but they might actually help with your review.
> 
> Justin: this isn't marked as a blocker, but the TAM for the Madai project
> thinks it will become one if we don't get it fixed soon, so there is some
> urgency on this. I'm asking for your review since this is code you'll
> probably want to be familiar with for your work on Gallery. But if you're
> swamped with camera bugs, I could probably get Jim or Punam to take the
> review instead.
> 
> What I've done here is convert MediaFrame to use background-image instead of
> an <img> element. There were two goals here:
> 
> 1) to prevent the jerky partial decode, pause, full decode display that we
> get with <img>.
> 
> 2) To allow gecko to decode the images for offscreen media frames so that
> they are ready to display when the user swipes them in. This used to work
> for <img> elements but then gecko got smarter and stoped decoding the
> offscreen img elements.
> 
> In addition to these two benefits I started out going for, this switch turns
> out to have a couple of surprise benefits:
> 
> 3) it becomes much easier to transition from a preview image to the full
> size image becasue the CSS background-image property allows us to specify
> multiple images. To transition to the full-size image, I just put the full
> url in front of the preview url in the background-image delcaration. That
> overlaid image remains transparent until it is decoded and then it hides the
> old one. It seems to work beautifully 
> 
> 4) Because the full-size image just overlays the preview image when it is
> ready, the app feels a lot more responsive when loading full images. The old
> code depended on a timer to hide the preview and show the fullsize image.
> Now we just get the full image as fast as gecko and display it, and it is a
> noticeable change.
> 
> I'm happy that this patch removes more code than it adds. And a lot of the
> new lines are comments. But this still isn't a trivial patch and I hope
> you're able to read the code carefully to look for errors.  (There is lots
> of refactoring I'd like to do on MediaFrame, but I tried hard to avoid doing
> it in this patch.)
> 
> I also hope to put together a testing plan and a set of test images (big,
> small, jpeg, png, preview, no preview, exif rotation, etc.) to really
> exercise this new version of MediaFrame carefully.

Thanks David, will also ask our partner to test the patch thoroughly as well

Thanks

Vance

Comment 7

[Justin D'Arcangelo [:justindarc]]

David,
Thanks for posting that IRC transcript here! VERY informative! Going to start reviewing your patch now.

-Justin

Comment 8

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8492035 [details] [review]
link to patch on github

As discussed on IRC, this looks good! Tested in both Camera and Gallery as well as with externally-loaded large images w/o EXIF previews and I see a noticeable improvement. Thanks again for the research with the help of #gfx!

Comment 9

[David Flanagan [:djf]]

I've got r+ on this, but I still want to make some time to test more carefully before landing.

I've filed bug 1070207 to create a set of test images we can use for manual testing of gallery, and that will help with testing of this bug.  I also want to verify that MediaFrame continues to work with videos.

And I want to verify that the changes to media frame do not affect the camera preview code or the gallery open activity. I think those are the only two uses of MediaFrame other than gallery.

Comment 10

[David Flanagan [:djf]]

Turns out that by modifying MediaFrame so that it no longer exposes imageblob, I've broken the share button in gallery... I need to fix that and also check that the other exported properties I removed are not in use.

Comment 11

[David Flanagan [:djf]]

I've updated the PR to fix the broken share button in Gallery. It was a simple 2-line fix to restore the imageblob property on MediaFrame, so it reduces the overall size of this patch.

Comment 12

[David Flanagan [:djf]]

If I run this patch on the images 17-33 from bug 1070207, it is clear that swiping through those large jpegs is smoother with this patch than without it.  Still not perfect, but none of the jaggy decoding artifacts that can be seen without the patch.

Comment 13

[David Flanagan [:djf]]

Landed on master: https://github.com/mozilla-b2g/gaia/commit/5baa82f4abae9ce0d10c92a9ecee8fbc4da74e98

Comment 14

[David Flanagan [:djf]]

Comment on attachment 8492035 [details] [review]
link to patch on github

Vance, the TAM for the Madai project reports that the two dependent bugs may become 2.0 blockers for Madai, so I'm requesting uplift of this bug, on the assumption that it will help resolve those bugs.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): At some point after the Gallery app was written, gecko got smarter about not decoding images while they were offscreen, and that may have regressed this. Also, Gecko now does a two-stage image decode. That may also be a change in the platform, or maybe we're just now using much bigger images (because with #-moz-samplesize we can) and we're now seeing that behavior. In any case, this but addresses visual regressions but I can't pin those regressions down.


[User impact] if declined: swiping through images in the gallery (and also the camera preview) will not be as smooth as it could be when those images do not have suitable EXIF previews. 

[Testing completed]: Yes, manually. I landed bug 1070207 so I'd have good tests to try this out against.

[Risk to taking this patch] (and alternatives if risky): This patch changes the way that Gallery and Camera display images from using an HTML <img> element to using the CSS background-image property. It is a non-trivial change, but it is not a horrendously complicated one, either. Because this code is mostly about visual appearance it is not covered by automated tests. But it has been manually tested fairly thoroughly. If I was doing the triage, I think I would recommend that we uplift to 2.1, where the patch will get more testing than it will get on master. For 2.0, perhaps our Madai partner can uplift it into their own build. Or we can uplift if it is stable on 2.1?

[String changes made]: none.

Comment 15

[bhavana bajaj [:bajaj]]

(In reply to David Flanagan [:djf] from comment #14)
> Comment on attachment 8492035 [details] [review]
> link to patch on github
> 
> Vance, the TAM for the Madai project reports that the two dependent bugs may
> become 2.0 blockers for Madai, so I'm requesting uplift of this bug, on the
> assumption that it will help resolve those bugs.

Vance, can you please help confirm with the partner that the partner here helps fix the two blockers ? I would be more comfortable uplifting once this is verified.
> 
> [Approval Request Comment]
> [Bug caused by] (feature/regressing bug #): At some point after the Gallery
> app was written, gecko got smarter about not decoding images while they were
> offscreen, and that may have regressed this. Also, Gecko now does a
> two-stage image decode. That may also be a change in the platform, or maybe
> we're just now using much bigger images (because with #-moz-samplesize we
> can) and we're now seeing that behavior. In any case, this but addresses
> visual regressions but I can't pin those regressions down.
> 
> 
> [User impact] if declined: swiping through images in the gallery (and also
> the camera preview) will not be as smooth as it could be when those images
> do not have suitable EXIF previews. 
> 
> [Testing completed]: Yes, manually. I landed bug 1070207 so I'd have good
> tests to try this out against.
> 
> [Risk to taking this patch] (and alternatives if risky): This patch changes
> the way that Gallery and Camera display images from using an HTML <img>
> element to using the CSS background-image property. It is a non-trivial
> change, but it is not a horrendously complicated one, either. Because this
> code is mostly about visual appearance it is not covered by automated tests.
> But it has been manually tested fairly thoroughly. If I was doing the
> triage, I think I would recommend that we uplift to 2.1, where the patch
> will get more testing than it will get on master. For 2.0, perhaps our Madai
> partner can uplift it into their own build. Or we can uplift if it is stable
> on 2.1?
> 
> [String changes made]: none.

Comment 16

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

Partner confirmed that the patch did solve this issue, please approve to land on 2.0 branch

Thanks

Comment 17

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/2b4d4027d56c89ea0bba1239278bfdf1e03beda9

Comment 18

[bhavana bajaj [:bajaj]]

Comment on attachment 8492035 [details] [review]
link to patch on github

Given the two blocking bugs raised by partner and as they have also verified the fixes per Vance, approving this to land on 2.0

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for v2.0 uplift.

Comment 20

[David Flanagan [:djf]]

Attachment: PR for 2.0

Ryan: here's a PR for the 2.0 branch. 

I'd like to make sure the python integration tests pass before you uplift this:

https://tbpl.mozilla.org/?rev=ac25e52a01508bccc1968b6af2d24d8c39086302&tree=Gaia-Try

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Trees are currently closed. I'll check back later as part of my regular uplift routine.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/9725d188a733a4aeebcfcf4c52d28e1ad8a2ba6f

Comment 23

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

(In reply to David Flanagan [:djf] from comment #20)
> Created attachment 8498439 [details] [review]
> PR for 2.0
> 
> Ryan: here's a PR for the 2.0 branch. 
> 
> I'd like to make sure the python integration tests pass before you uplift
> this:
> 
> https://tbpl.mozilla.org/
> ?rev=ac25e52a01508bccc1968b6af2d24d8c39086302&tree=Gaia-Try

Hi David -

Our partner also want to cherry-pick this one to Dolphin(As you know Dolphin just launch in Bangladesh, ), it it possible the patch can be applied to Dolphin?

Comment 24

[Kai-Zhen Li [:seinlin][:kli]]

https://github.com/mozilla-b2g/gaia/commit/494a604e3fd8597c39c7baf9cc6311f757d1135e

Comment 25

[David Flanagan [:djf]]

(In reply to Vance Chen [:vchen][[e-mail]] from comment #23)
> 
> Our partner also want to cherry-pick this one to Dolphin(As you know Dolphin
> just launch in Bangladesh, ), it it possible the patch can be applied to
> Dolphin?

I think the patch will work for Dolphin but have not tried it on the 1.4 branch. It is possible that the version of gecko used in Dolphin has different performance characteristics, so even if the patch applies cleanly, I can't guarantee that it will perform the same way on that branch.

If our partners do cherry-pick this patch, they should plan to test very carefully. Here's what I wrote about the risk level in the uplift request:

[Risk to taking this patch] (and alternatives if risky): This patch changes the way that Gallery and Camera display images from using an HTML <img> element to using the CSS background-image property. It is a non-trivial change, but it is not a horrendously complicated one, either. Because this code is mostly about visual appearance it is not covered by automated tests. But it has been manually tested fairly thoroughly. If I was doing the triage, I think I would recommend that we uplift to 2.1, where the patch will get more testing than it will get on master. For 2.0, perhaps our Madai partner can uplift it into their own build. Or we can uplift if it is stable on 2.1?