1163215 - Make MediaFrame use <img> instead of background-image

Status: RESOLVED FIXED

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

Description

[David Flanagan [:djf]]

The MediaFrame class used to use <img> elements. Then we changed to use background-image because gecko was better at decoding those while offscreen.

Now, gecko should be smart enough to decode img elements when they are near the screen, and we're having memory issues possibly related to background images not being released soon enough.

So it is time to switch back to <img> elements in shared/js/media/media_frame.js.

Seth Fowler pointed out that we can still use a background-image on an <img> element to make the transition from preview to full-size image nice and smooth.  He even prepared a trial patch at: https://github.com/sethfowler/gaia/commit/d44816c000886cb549f08d02c18a5d8a169bbb81

So this bug is to either just land Seth's patch or to do something similar to that.

See also bug 1140619 for further discussion of why this is going to be necessary in the future anyway.

And see bug 1161734 for a 2.2+ bug that we're hoping this change will help with.

Comment 1

[David Flanagan [:djf]]

Seth will investigate whether this bug blocks 2.2+ bug 1161734. If it does, then we'll have to uplift this.

Comment 2

[GitHub Autolander Bot]

Attachment: [gaia] davidflanagan:bug1163215 > mozilla-b2g:master

Comment 3

[David Flanagan [:djf]]

Comment on attachment 8604338 [details] [review]
[gaia] davidflanagan:bug1163215 > mozilla-b2g:master

Punam: please review this as soon as you can. It will probably be needed to resolve blocker bug 1161734. We're switching MediaFrame back to using <img> instead of background-image. As gecko evolves, the best way to display large images keeps changing.  Note that we're using background-image on a <img> element now, but only when the user zooms in. This gives us a way to start zooming with the preview image and then swap in the full-size image when it is ready.

Seth: please review from the platform standpoint. Does this look like it will help gecko better manage its image memory?  Note that I did not use your version of the patch here in part because yours is based on code that has already apparently removed the #-moz-samplesize media fragments, and I assume we're going to continue to need #-moz-samplesize on 2.2.

Comment 4

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

(In reply to David Flanagan [:djf] from comment #3)
> Seth: please review from the platform standpoint. Does this look like it
> will help gecko better manage its image memory?  Note that I did not use
> your version of the patch here in part because yours is based on code that
> has already apparently removed the #-moz-samplesize media fragments, and I
> assume we're going to continue to need #-moz-samplesize on 2.2.

Yeah, that's correct. We are nearing the point where we won't need #-moz-samplesize on master, but that stuff won't get backported to 2.2.

Comment 5

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

Comment on attachment 8604338 [details] [review]
[gaia] davidflanagan:bug1163215 > mozilla-b2g:master

r- only because it doesn't look like you're ever actually passing |bg| to |_displayImage|, as I mentioned on github.

Comment 6

[David Flanagan [:djf]]

Comment on attachment 8604338 [details] [review]
[gaia] davidflanagan:bug1163215 > mozilla-b2g:master

Resetting the r? flag. See line 448 for the _displayImage() call that uses the new argument.

I'll update the typo in the comment before landing, but don't want to push a new commit now because it will probably restart the test run.

Comment 7

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

Comment on attachment 8604338 [details] [review]
[gaia] davidflanagan:bug1163215 > mozilla-b2g:master

I did indeed just miss it. Thanks for pointing that out, David!

Comment 8

[Punam Dahiya [:pdahiya]]

Comment on attachment 8604338 [details] [review]
[gaia] davidflanagan:bug1163215 > mozilla-b2g:master

Hi David

The patch looks good and has my r+. Few nits noted in github about updating comments and with that patch looks good to land on master.

Another nit,  I noticed while testing the patch on nexus-4 is a subtle flicker when the image first loads in full-screen mode after clicking thumbnail which is not seen on master when using background-image.

Thanks!

Comment 9

[David Flanagan [:djf]]

Comment on attachment 8604338 [details] [review]
[gaia] davidflanagan:bug1163215 > mozilla-b2g:master

Punam,

I've updated the patch with the comment edits you pointed out, and have also made bigger changes:

1) the flickering was the broken image icon being displayed. I now set the opacity to 0 by default, then set it to 1 when I get a load event. I added CSS to gallery.css to make the opacity transition animated which gives a nice effect, I think.

2) I've changed MediaFrame to use only a single image element instead of creating a new one each time displayImage() is called. I hope that this will allow Gecko to discard old images more quickly than it would with the old code. This change is relatively large but might have a substantial impact on memory usage.

I've just noticed that there were some python test failures, probably due to the change of a div to an img, and I may have to push another version of the patch to make those work.  But you should be able to review these changes now.

Comment 10

[Punam Dahiya [:pdahiya]]

Comment on attachment 8604338 [details] [review]
[gaia] davidflanagan:bug1163215 > mozilla-b2g:master

Hi David

The updated patch looks good and has my r+. I see one python tests that's failing, with that fixed it should be good to land. Thanks!

Comment 11

[David Flanagan [:djf]]

Comment on attachment 8604338 [details] [review]
[gaia] davidflanagan:bug1163215 > mozilla-b2g:master

No-Jun: I'd like your review of the python test changes, please. I think I'm going to have to uplift this to 2.2, so there is some urgency about getting it landed.

Comment 12

[No-Jun Park [:njpark]]

gallery tests ran locally, and they passed, and the changes themselves on the script looks okay to me as well.

Comment 13

[David Flanagan [:djf]]

Tests are finally green, and I've got all the reviews I need.

Comment 14

[David Flanagan [:djf]]

Seth: do you want this uplifted to 2.2 as part of your work on bug 1161734?

Comment 15

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/85bcb789753f9a185d6a05abb088f8f3440810d0

Comment 16

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

(In reply to David Flanagan [:djf] from comment #14)
> Seth: do you want this uplifted to 2.2 as part of your work on bug 1161734?

I think that's a good idea, yeah.

Comment 17

[David Flanagan [:djf]]

Comment on attachment 8604338 [details] [review]
[gaia] davidflanagan:bug1163215 > mozilla-b2g:master

Josh,

Seth thinks that uplifting this patch will help resolve the 2.2+ blocker bug 1161734.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): not a regression; this is just an adjustment to gaia to make it follow the changing fast paths in gecko.

[User impact] if declined: gallery app may use more memory and be less responsive.

[Testing completed]: locally

[Risk to taking this patch] (and alternatives if risky): this is not a trivial patch, but it is not a really complex one either. We're going to need to do lots of QA testing on the gallery app after bug 1161734 is resolved, and that should help us find any regressions introduced here.  Having said that, I don't think that this patch will introduce regressions.

[String changes made]: none

Comment 18

[Josh Cheng [:josh]]

Comment on attachment 8604338 [details] [review]
[gaia] davidflanagan:bug1163215 > mozilla-b2g:master

Hi Kevin

FYR about this patch related to bug 1161734.
Please help for regression test on Gallery app after this patch and bug 1161734 fix.

Thanks!

Comment 19

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/2784bc4f5caf83744a246557d7eeb37b5c7f394e

Comment 20

[KTucker [:KTucker][Inactive 3/4/2016]]

Adding qawanted per Comment 18.

Comment 21

[KTucker [:KTucker][Inactive 3/4/2016]]

No flashing thumbnails were observed when hundreds of pictures are present on the device but now there are large section of pictures(black areas) that appear to be missing on the Flame 2.2. 

Environmental Variables:
Device: Flame 2.2 (Full Flash)(KK)(319mb)
BuildID: 20150519002500
Gaia: 732acec6f37d13ccea6b0ddc48904a53a2970894
Gecko: 1389e6b8c065
Gonk: bd9cb3af2a0354577a6903917bc826489050b40d
Version: 37.0 (2.2) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0