1188286 - [Camera]When you browse photos in preview mode in Camera, device screen is Blank for several seconds when switching to next photo.

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Camera, defect, P3)

Description

[Annabelle]

Attachment: logcat_2318.txt

[1.Description]:
[Arise v2.5][Flame v2.2 & master] [Camera]When you browse photos in preview mode in Camera, device screen is Blank for several seconds when switching to next photo.
See attachment: Arise_2.5.3gp & logcat_ 2318.txt

[2.Testing Steps]: 
1. Open the Camera app. 
2. Take multiple photos. 
3. Select the Preview icon. 
4. Swipe through the photos left and right. 

[3.Expected Result]: 
4.The photos change with a smooth transition animation.

[4.Actual Result]: 
4.The device will be blank for several seconds when switching to next photo.

[5.Reproduction build]: 

Device: Aries KK 2.5(Affected)
Build ID               20150728035120
Gaia Revision          14e32276025b0310d3e89027320cf4b2a24cedfb
Gaia Date              2015-07-27 16:43:18
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/33dc8a83cfc0
Gecko Version          42.0a1
Device Name            aries
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.worker.20150728.033538
Firmware Date          Tue Jul 28 03:35:46 UTC 2015
Bootloader             s1

Device: Flame KK 2.2(Affected)
Build ID               20150727152503
Gaia Revision          e1e6317f17a840b19af9dbb25f5a771d8d9fa161
Gaia Date              2015-07-15 21:05:11
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/9d103025178f
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150727.184844
Firmware Date          Mon Jul 27 18:48:55 EDT 2015
Firmware Version       v18D v4
Bootloader             L1TC000118D0

Device: Flame KK 2.5(Affected)
Build ID               20150727150208
Gaia Revision          14e32276025b0310d3e89027320cf4b2a24cedfb
Gaia Date              2015-07-27 16:43:18
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/c0a3d6f72a63
Gecko Version          42.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150727.183206
Firmware Date          Mon Jul 27 18:32:18 EDT 2015
Firmware Version       v18D v4
Bootloader             L1TC000118D0

[6.Reproduction Frequency]: 
Always Recurrence,5/5

[7.TCID]: 
14279

[8. Note] The photos change with a smooth transition animation in Gallery.

Comment 1

[Annabelle]

Attachment: Arise_2.5.3gp

Comment 2

[No-Jun Park [:njpark]]

[Blocking Requested - why for this release]:

The preview app does transition slowly, and there is a noticeable gap between each picture refresh.  This happens in Flame too, but in Aries it looks worse because part of the image is shown before pausing a bit and redraws completely.  nominating for 2.5.

Comment 3

[Hema Koka [:hema]]

Bad user experience, blocking

Comment 4

[Hema Koka [:hema]]

Diego, please investigate

Comment 5

[Andrew Osmond [:aosmond] (he/him)]

Bug 1181290 is similar. The photos are really large, take a long time to be drawn.

Comment 6

[Punam Dahiya [:pdahiya]]

See comment https://bugzilla.mozilla.org/show_bug.cgi?id=1181290#c17 updated while investigating this issue for camera preview.

Comment 7

[Punam Dahiya [:pdahiya]]

The issue here is not a big enough EXIF preview (160 x 120), because of which we are generating preview by downsampling full size images. Downsampled images shows flash while loading on Aries because of large size of image.

I propose to scale small EXIF preview from Aries to screen size with smooth animations and start to load downsampled preview in background and when its loaded, use the downsampled preview image to remove the pixel effect. This should reduce the grey flash seen due to lag of loading downsampled image.

Including djf for his input on proposed fix. Setting NI for Diego if it's ok to assign bug to myself. Thanks!

Comment 8

[David Flanagan [:djf]]

Punam,

Here are my concerns with your proposal:

- the screen is so large and the EXIF preview is so small that I think the upsampled image might look worse than a blank screen.  I haven't tested this, though.

- Camera uses MediaFrame, and I don't think that MediaFrame can do what you're proposing without significant (and therefore risky) changes. If you're going to try it, I think I'd ask you to make your change outside of MediaFrame, not inside of it.  (Maybe I'm missing something and MediaFrame can handle this easily. Let me know if I'm wrong.)

- In your comment you write "and when its loaded"...  Note though that the load event on an image just tells you when the file has been transferred over the network. We don't have any way to tell when an image is actually decoded.  In the past I've had to use a setTimeout() to guess.  In the future (when we remove #moz-samplesize) I'll be using a technique in MediaFrame where I display a small image with CSS background-image on an <img> element.  That background image gets displayed until the full image is loaded. This cool hack was Seth's idea, and I've verified that it works, so perhaps you could do that with the EXIF image if you break the MediaFrame abstraction and set a background image on the MediaFrame <img> element

Note that the camera preview gallery already has some kind of fade out/fade in code to disguise the ugliness of the transition between images.  Another approach you might be able to take here is to play around with that to disguise the issue. That would be the simplest and least risky change.

Note that Gallery doesn't (didn't) have this problem as badly becasuse it uses three media frames and images can start decoding while offscreen. That felt too heavyweight for the preview gallery in camera, so I went with one media frame when I wrote that code. So another approach would be to to use more MediaFrame elements, maybe with Justin's gaia-carousel web component. But that seems like a pretty heavyweight change to make after the FL date.

Comment 9

[Punam Dahiya [:pdahiya]]

(In reply to David Flanagan [:djf] from comment #8)
> Punam,
> 
> Here are my concerns with your proposal:
> 
> - the screen is so large and the EXIF preview is so small that I think the
> upsampled image might look worse than a blank screen.  I haven't tested
> this, though.
> 
> - Camera uses MediaFrame, and I don't think that MediaFrame can do what
> you're proposing without significant (and therefore risky) changes. If
> you're going to try it, I think I'd ask you to make your change outside of
> MediaFrame, not inside of it.  (Maybe I'm missing something and MediaFrame
> can handle this easily. Let me know if I'm wrong.)
> 
> - In your comment you write "and when its loaded"...  Note though that the
> load event on an image just tells you when the file has been transferred
> over the network. We don't have any way to tell when an image is actually
> decoded.  In the past I've had to use a setTimeout() to guess.  In the
> future (when we remove #moz-samplesize) I'll be using a technique in
> MediaFrame where I display a small image with CSS background-image on an
> <img> element.  That background image gets displayed until the full image is
> loaded. This cool hack was Seth's idea, and I've verified that it works, so
> perhaps you could do that with the EXIF image if you break the MediaFrame
> abstraction and set a background image on the MediaFrame <img> element
> 
 _displayImage in MediaFrame takes background URL and do similar
https://github.com/mozilla-b2g/gaia/blob/master/shared/js/media/media_frame.js#L367

I was hoping to pass EXIF preview url to _ displayImage. Since code to generate downsampled image is triggered inside noPreview, we can change noPreview to accept a bgURL  and pass it to _displayImage 

https://github.com/mozilla-b2g/gaia/blob/master/shared/js/media/media_frame.js#L289

I agree media_frame is shared API and should have minimal and well tested changes. I can do a quick patch and attach it for feedback and risk analysis. 

> Note that the camera preview gallery already has some kind of fade out/fade
> in code to disguise the ugliness of the transition between images.  Another
> approach you might be able to take here is to play around with that to
> disguise the issue. That would be the simplest and least risky change.
> 
> Note that Gallery doesn't (didn't) have this problem as badly becasuse it
> uses three media frames and images can start decoding while offscreen. That
> felt too heavyweight for the preview gallery in camera, so I went with one
> media frame when I wrote that code. So another approach would be to to use
> more MediaFrame elements, maybe with Justin's gaia-carousel web component.
> But that seems like a pretty heavyweight change to make after the FL date.

Comment 10

[GitHub Autolander Bot]

Attachment: [gaia] punamdahiya:Bug1188286 > mozilla-b2g:master

Comment 12

[Punam Dahiya [:pdahiya]]

Comment on attachment 8670949 [details] [review]
[gaia] punamdahiya:Bug1188286 > mozilla-b2g:master

Hi David
Attaching patch for your feedback. I tested and the patch fixes the similar issue reported in contacts and messaging app (Bug 1191223). Thanks!

Comment 13

[David Flanagan [:djf]]

Comment on attachment 8670949 [details] [review]
[gaia] punamdahiya:Bug1188286 > mozilla-b2g:master

Punam,

I'm excited that you were able to do this so quickly. I thought the patch you were proposing would be much harder.

How well does it work? Do the tiny Aries EXIF previews look better than nothing?

I've left suggestions on github, but basically I think this is great and it should be an easy review once you finish it up.

Comment 14

[Punam Dahiya [:pdahiya]]

(In reply to David Flanagan [:djf] from comment #13)
> Comment on attachment 8670949 [details] [review]
> [gaia] punamdahiya:Bug1188286 > mozilla-b2g:master
> 
> Punam,
> 
> I'm excited that you were able to do this so quickly. I thought the patch
> you were proposing would be much harder.
> 
> How well does it work? Do the tiny Aries EXIF previews look better than
> nothing?
> 
> I've left suggestions on github, but basically I think this is great and it
> should be an easy review once you finish it up.

Thanks David, I have updated patch with your feedback. Tiny Aries EXIF preview definitely looks better than grey screen. Tiny preview is loaded for a fraction of seconds before switching to downsampled preview image.

I have tested attached patch for camera preview and camera pick activity (Bug 1191223). For camera preview while swiping fast between images, I am seeing momentarily grey sometime. Please review and provide your input.

Comment 15

[Punam Dahiya [:pdahiya]]

Comment on attachment 8670949 [details] [review]
[gaia] punamdahiya:Bug1188286 > mozilla-b2g:master

Removing review flag to fix unit tests.

Comment 16

[Punam Dahiya [:pdahiya]]

Comment on attachment 8670949 [details] [review]
[gaia] punamdahiya:Bug1188286 > mozilla-b2g:master

Unit tests are passing. Setting review flag now. Thanks

Comment 17

[David Flanagan [:djf]]

Attachment: more patch to try

Comment 18

[Punam Dahiya [:pdahiya]]

(In reply to David Flanagan [:djf] from comment #17)
> Created attachment 8672047 [details]
> more patch to try

Thanks David, will try it and update bug.

Comment 19

[David Flanagan [:djf]]

Comment on attachment 8670949 [details] [review]
[gaia] punamdahiya:Bug1188286 > mozilla-b2g:master

Punam,

Your patch works better than I imagined it could. Thanks for actually doing it and making it work.

I see what you mean about swiping sideways still sometimes showing the image decoding. I think that is a race between the background image displaying and the big image loading and beginning to decode or something. I think I fixed it with the patch I just attached.

r+ if you incorporate my patch and test carefully. Be sure to verify that your patch (and mine) do not break anything in Gallery or break anything in the camera app on flame.

Finally, think about tweaking the FADE_IN_DURATION constant in preview_gallery.js  It looks okay to me with much smaller values, but I also like how it look with the current 500ms. I'm not sure what to do here, but consider reducing that number

Comment 20

[Punam Dahiya [:pdahiya]]

(In reply to David Flanagan [:djf] from comment #19)
> Comment on attachment 8670949 [details] [review]
> [gaia] punamdahiya:Bug1188286 > mozilla-b2g:master
> 
> Punam,
> 
> Your patch works better than I imagined it could. Thanks for actually doing
> it and making it work.
> 
> I see what you mean about swiping sideways still sometimes showing the image
> decoding. I think that is a race between the background image displaying and
> the big image loading and beginning to decode or something. I think I fixed
> it with the patch I just attached.
> 
Thanks David. I verified and it works!
> r+ if you incorporate my patch and test carefully. Be sure to verify that
> your patch (and mine) do not break anything in Gallery or break anything in
> the camera app on flame.
> 

verified and carrying r+ on the updated patch. Will land after tree herder is done running tests.
> Finally, think about tweaking the FADE_IN_DURATION constant in
> preview_gallery.js  It looks okay to me with much smaller values, but I also
> like how it look with the current 500ms. I'm not sure what to do here, but
> consider reducing that number

I agree there's a very slight improvement on reducing FADE_IN_DURATION, for now updating it to 450. IMO on bringing it to lower values shows a sudden change when swapping between bg and preview image.

Comment 21

[Punam Dahiya [:pdahiya]]

Patch landed on master

https://github.com/mozilla-b2g/gaia/commit/7f5325649e416ab1737f855473b038105072a7c8

Comment 22

[Carrie.Yang Leave from Mozilla]

This bug has been verified as pass on latest build of Flame KK v2.5 and Aries KK v2.5 by the STR in comment 0.
Actual results: When swiping fast between images for camera preview, It shows momentarily grey 
sometime.
See attachment: Verified_Aries_v2.5.3gp.
Reproduce rate: 0/10

Device: Flame KK v2.5 (Pass)
Build ID               20151015150343
Gaia Revision          8ea9029190af2ffeb04dcd97b323738125e31a0e
Gaia Date              2015-10-15 14:30:30
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/d374d16cbb251c9dac5af69f8e186e821ce82fe2
Gecko Version          44.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20151015.183044
Firmware Date          Thu Oct 15 18:30:54 EDT 2015
Firmware Version       v18D v4
Bootloader             L1TC000118D0

Device: Aries KK v2.5 (Pass)
Build ID               20151015193337
Gaia Revision          8ea9029190af2ffeb04dcd97b323738125e31a0e
Gaia Date              2015-10-15 14:30:30
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/d374d16cbb251c9dac5af69f8e186e821ce82fe2
Gecko Version          44.0a1
Device Name            aries
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.worker.20151015.185317
Firmware Date          Thu Oct 15 18:53:25 UTC 2015
Bootloader             s1

Comment 23

[Carrie.Yang Leave from Mozilla]

Attachment: Verified_Aries_v2.5.3gp