1181290 - [Aries][Gallery] Images draw in segments from right to left displaying placeholder color briefly and flashing

Status: VERIFIED FIXED

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

Description

[Oliver Nelson [:oliverthor]]

Description:
When a user is accessing images in large view from their gallery (by tapping), they will observe that the images have a splintered load-in and display a placeholder color while the image settles. This does not occur when the user is swiping between images, only when they are accessing from thumbnail view of gallery's contents. 
The load itself appears similar to the swipe behavior observed in the Camera app preview pictures: bug 1178510

Repro Steps:
1) Update an Aries to 20150707033152
2) Open the Camera app
3) Take 5 pictures
4) Return to homescreen, open gallery
5) Tap a picture to view large
6) Observe draw

Actual:
Draw appears from right to left and flashes; placeholder color is observed on segments when flashing

Expected:
Image draws at once; no flashing


Environmental Variables:
----------------------
Device: Aries 2.5
Build ID: 20150707033152
Gaia: e6506d68e01489b57616f8b74e8773f938ce62b3
Gecko: e271ef4c49ae
Gonk: 2916e2368074b5383c80bf5a0fba3fc83ba310bd
Version: 42.0a1 (2.5)
Firmware Version: D5803_23.1.A.1.28_NCB.ftf
User Agent: Mozilla/5.0 (Mobile; rv:42.0) Gecko/42.0 Firefox/42.0

**********************

Issue DOES NOT REPRO on master for flame devices
Device: Flame 2.5
BuildID: 20150707010204
Gaia: e6506d68e01489b57616f8b74e8773f938ce62b3
Gecko: e7e69cc8c07b
Gonk: a4f6f31d1fe213ac935ca8ede7d05e47324101a4
Version: 42.0a1 (2.5) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:42.0) Gecko/42.0 Firefox/42.0
----------------------

Repro frequency: 5/5
See attached: 
video- https://youtu.be/KUgFM5mDwDI
logcat

Comment 1

[Peter Bylenga [:PBylenga]]

NI Component owner to take a look, is this a dupe of bug 1178510?

Comment 2

[No-Jun Park [:njpark]]

hmm, I think it might be a graphics issue.  kats, could you take a look at this one?

And I think bug 1178510 is definitely a dupe of this one.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

It might be a graphics issue but my preliminary investigation didn't point to any of the usual suspects. The image isn't being animated in; it's just plopped into place and seems to draw in chunks. Seth, could this be a result of decode-on-draw or something?

Comment 5

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

My best guess is that this is just a result of the image not being decoded already. Nothing to do with decode-on-draw, necessarily. We won't keep non-visible images in memory, regardless of decode-on-draw.

Probably this is a result of bug 1163215, which made us use <img> elements instead of CSS background images for the Gallery app. <img> elements draw in progressively instead of "popping" in once they're completely decoded, like CSS background images do.

We switched to using <img> elements because currently memory used by CSS background images cannot be freed when the images scroll out of view, resulting in severe memory-related issues in the Gallery app. I want to fix that issue at the platform level for 2.5, but it will take a while; it's a pretty massive change.

I'm going to speculatively add blocking bugs.

Comment 6

[Hema Koka [:hema]]

Regression and noticeable flashing...

Comment 8

[David Flanagan [:djf]]

Seth: could this be happening because we're still using the #-moz-sample-size media fragment and not just relying on gecko to downsample the image for us?

If so, then fixing 1140619 might fix this.

Otherwise, the issue is that the camera app takes ridiculously large 20mp images, but does not include a screen-sized EXIF preview. In these circumstances, we have to decode the full image, and that involves processing megabytes of data. So of course it is slow.

I'd suggest that the best solution here would be to change the default photo resolution in the camera app so that it takes smaller photos by default. Andrew: is this something you're willing to consider?

If we have to, we could revert the gallery back to the old days when we created preview images for large photos that did not have suitable EXIF previews. Given that gecko can downsample on decode, we shouldn't have to do that, but I suppose we could put it back in if needed.

More likely, though, would be to play some kind of CSS trick and fade the photo in to try to disguise the image decoding delay.

Comment 9

[David Flanagan [:djf]]

Clearing the regression keyword. I'm pretty sure that no code has actually broken here: this bug appears because we've quadrupled the size of the photos we take.

Comment 10

[David Flanagan [:djf]]

I have been hoping that this would be resolved (or mitigated sufficiently) by the work in bug 1206206. But it now looks like that will not be landing in 2.5.

I believe that there is not a bug here, but that is just how much time it takes to decode and display the giant images from Aries. The camera on that device does not produce usable EXIF preview images, so we have to decode the full image.

In order to change this, we'd have to modify the Gallery metadata parser to create a screen-sized preview image for each photo the user takes. We used to do this in the past, and I think we still do it for large PNG images. So the change won't be too difficult.  It is a tradeoff: it will make scanning slower, and take up more space on the user's sd card, but viewing photos in the gallery will be more responsive.

Comment 11

[Punam Dahiya [:pdahiya]]

As discussed over IRC, assigning myself to look into this issue.

Comment 12

[GitHub Autolander Bot]

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

Comment 13

[Punam Dahiya [:pdahiya]]

Comment on attachment 8669959 [details] [review]
[gaia] punamdahiya:Bug1181290 > mozilla-b2g:master

Hi David

Attaching patch that creates preview for large jpeg files. I have kept the file size check to 2.5 MB as down-sampling jpegs > 2.5 MB for preview shows flash when viewed in fullscreen. Please review. Thanks!

Comment 14

[David Flanagan [:djf]]

Comment on attachment 8669959 [details] [review]
[gaia] punamdahiya:Bug1181290 > mozilla-b2g:master

Please base the size test on the actual image size, since we already know it. The file size is usually a good proxy for jpeg image size, but it can vary a lot depending on the subject of a photo.  We've got the actual pixel size of the image already, so let's use that.

Comment 15

[Punam Dahiya [:pdahiya]]

(In reply to David Flanagan [:djf] from comment #14)
> Comment on attachment 8669959 [details] [review]
> [gaia] punamdahiya:Bug1181290 > mozilla-b2g:master
> 
> Please base the size test on the actual image size, since we already know
> it. The file size is usually a good proxy for jpeg image size, but it can
> vary a lot depending on the subject of a photo.  We've got the actual pixel
> size of the image already, so let's use that.

makes sense, updated patch to use image size in pixels. Please review.

Comment 16

[David Flanagan [:djf]]

Comment on attachment 8669959 [details] [review]
[gaia] punamdahiya:Bug1181290 > mozilla-b2g:master

Looks good to me

Comment 17

[Punam Dahiya [:pdahiya]]

While testing this fix for camera Bug 1188286, here are my findings

a) Camera preview doesn't use preview images stored in sdcard. Reason being metadataparser is kicked in when gallery app is opened. Camera preview uses media_frame to check for valid preview and if no valid preview is available (Line 272) uses downsampled image as preview.

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

So flashing is still seen while viewing image in camera preview.

b) On debugging, we do have a valid preview generated by Aries camera however it's not bigenough and has width and height of 160 x120 because of which media_frame fails to use it and executes flow inside nopreview();

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

Andrew, David: Is there any way to customize this preview size so that camera app generate valid sizes preview.
I see we have config inside camera app, is this something that can be configured by device.

https://github.com/mozilla-b2g/gaia/blob/master/apps/camera/js/config/config.js#L14

Thanks!

Comment 18

[Punam Dahiya [:pdahiya]]

Synched up with Andrew on IRC and 160x 120 is the largest size preview Aries can produce. Landing attached patch that fixes this issue for gallery and moving the discussion to resolve this issue for camera in bug 1188286.

Comment 19

[Punam Dahiya [:pdahiya]]

Patch landed on master

https://github.com/mozilla-b2g/gaia/commit/cbf099c0858cc03172df87977c17f26f12e69fe2

Comment 20

[Jessica]

Attachment: verified_Aries KK v2.5.3gp

According to the STR of Comment 0, this bug has been verified as pass on latest Aries KK v2.5.

Actual results: Image draws at once and no flashing.
See attachment: verified_Aries KK v2.5.3gp
Reproduce rate: 0/6


Device: Aries KK v2.5 (Pass)
Build ID               20151010002512
Gaia Revision          74b0d4b17f39d238a7997800bd9363d3c60f20c3
Gaia Date              2015-10-09 19:27:39
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/d1bb0de19476541cd517ab14017e7fedbd9f13e3
Gecko Version          44.0a1
Device Name            aries
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.worker.20151009.234251
Firmware Date          Fri Oct  9 23:42:59 UTC 2015
Bootloader             s1