Video app fails to list videos where it can't generate a thumbnail

RESOLVED FIXED

Status

Firefox OS
Gaia::Video
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: cajbir, Assigned: gduan)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

(Reporter)

Description

5 years ago
Steps to reproduce requires hardware that does not support capturing thumbnails from H.264 videos (eg. Nexus S):

1) Make sure you have mp4 files on the sdcard, either by adb'ing them there or recording a video with the camera.
2) Go to video app.

Expected result:

1) mp4 video appears in list

Actual result:

1) mp4 video does not appear

Reason:

If the hardware does not support capturing thumbnails from the hardware decoder then the video is not listed at all. This is because 'canvas.drawImage' throws an error at this point. If we capture and ignore it we can still list the video but with a blank thumbnail.
(Reporter)

Updated

5 years ago
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
(Reporter)

Comment 1

5 years ago
Created attachment 746196 [details] [diff] [review]
Ignore thumbnail generation error
Attachment #746196 - Flags: review?(dflanagan)
Comment on attachment 746196 [details] [diff] [review]
Ignore thumbnail generation error

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

If we can't capture a thumbnail for the video, then we won't be able to play it, either, and I think the best solution is to just ignore it.

The code looks fine, but I'm giving an r- because I think the current behavior (ignoring non-playable videos) is the right thing to do. Argue with me if I'm wrong about this. I'd be reluctant to change it, though, without an explicit directive from UX to list video files that have no thumbnail and cannot be played!
Attachment #746196 - Flags: review?(dflanagan) → review-
(Reporter)

Comment 3

5 years ago
(In reply to David Flanagan [:djf] from comment #2)
> Comment on attachment 746196 [details] [diff] [review]
> Ignore thumbnail generation error
> 
> Review of attachment 746196 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> If we can't capture a thumbnail for the video, then we won't be able to play
> it, either, and I think the best solution is to just ignore it.
> 

That's not correct. H.264 videos play fine on the Nexus S but thumbnails can't be taken from them. This is due to the driver not allowing us to get direct access to the pixel data vs the zero copy method the playback uses to display the video.

The situation at the moment is you can record videos on the Nexus S but they don't appear anywhere because the thumbnail won't display - but they play fine if you just handle the thumbnail error as in the patch.

Other hardware has similar issues - some variations of the Samsung Galaxy S2 have the same problem. We will also have the problem with videos that have the "don't allow access to the pixels" bits set (eg. DRM when we support that).
(Reporter)

Comment 4

5 years ago
See bug 866028 where a production device is having similar issues (can't produce thumbnail but can play video).
Chris,

Thanks for explaining this issue...

The fact that there are videos that cannot be copied into a canvas has implications for the canvas web standard which seems to assume that videos can always be copied, I think.

I think we need UX help here, so setting needinfo on Casey: Casey what should the video app do for videos that can be played but for which we can not obtain a thumbnail?  

Chris: does your patch distinguish between thumbnail failures that indicate an invalid, unplayable, video and those that just can't have a thumbnail created?
Flags: needinfo?(kyee)
Flags: needinfo?(chris.double)
(Reporter)

Comment 6

5 years ago
(In reply to David Flanagan [:djf] from comment #5)
> Chris: does your patch distinguish between thumbnail failures that indicate
> an invalid, unplayable, video and those that just can't have a thumbnail
> created?
Yes. If the video is unplayable we won't have been able to load, then seek to the frame to create the thumbnail. The patch only handles the case where the thumbnail itself cannot be generated.
Flags: needinfo?(chris.double)

Comment 7

5 years ago
We will need some kind of generic stand-in thumbnail for videos.

Flagging Patryk for visual design support.
Flags: needinfo?(kyee) → needinfo?(padamczyk)
Peter La is working on thumbnails for media. He can provide the needed graphics.
Flags: needinfo?(padamczyk) → needinfo?(pla)

Comment 9

5 years ago
Created attachment 764930 [details]
Video Thumbanail Screenshots + Exported Graphic

Attached is the exported thumbnail graphic - let me know if you need a 2x resolution version - along with a few screenshots to show it in possible scenarios.
Flags: needinfo?(pla)
Created attachment 8349930 [details]
PR to master

Hi John,
could you kindly check this patch? It should display a Thumbnail.png once we cannot get preview image.

Hi Chris,
could you kindly check if it's workable?
Attachment #8349930 - Flags: review?(johu)
Attachment #8349930 - Flags: feedback?(chris.double)
Hi Peter,
could you provide @1.5x image?
Flags: needinfo?(pla)
Comment on attachment 8349930 [details]
PR to master

The patch looks good. But it breaks a unit test. And we need to include 1.5x image in this patch.

I don't have a device to reproduce this issue. So, I reset the review flag and wait for chris's feedback. Once he f+, please put review flag to me. Thanks.
Attachment #8349930 - Flags: review?(johu)
(Reporter)

Comment 13

5 years ago
Comment on attachment 8349930 [details]
PR to master

Looks good to me but I'm unable to test on a device.
Attachment #8349930 - Flags: feedback?(chris.double) → feedback+

Comment 14

5 years ago
Created attachment 8357832 [details]
Updated Thumbnail Icons @ 1x, 1.5x and 2x Sizes

Hi George,

I've separated the icon from the background so that it's more flexible.  This means you'll have to render a rectangle for the background separately @ the specified colour (#333333).

Let me know if you need anything else?
Flags: needinfo?(pla)
Comment on attachment 8349930 [details]
PR to master

Hi John,
please kindly check it again. Thanks.
Attachment #8349930 - Flags: review?(johu)
Comment on attachment 8349930 [details]
PR to master

r- because the background-size: 100% is wrong when we use this alternative thumbnail for videos which cannot generate thumbnail. We should use its 1x size.

Suggest to create another css class for this case and use it to override the original background image size and color.
Attachment #8349930 - Flags: review?(johu) → review-
Comment on attachment 8349930 [details]
PR to master

Thanks for commenting, patch is updated. Please check it again.
Thanks.
Attachment #8349930 - Flags: review- → review?(johu)
Comment on attachment 8349930 [details]
PR to master

Looks good to me. Please wait for travis result before landing code.
Attachment #8349930 - Flags: review?(johu) → review+
Assignee: nobody → gduan
Thanks,
Master: https://github.com/mozilla-b2g/gaia/commit/14d091833eafd2ea6c11ccb53444e9756df587e6
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
I've noticed this commit during my merge today because the PNG files it introduces in the app are (almost) uncompressed. Since we landed bug 959659 we're trying to ensure that all our assets are properly compressed as it saves storage, memory and CPU time when decoding the images.

You can use the script under tools/png_recompress.sh in your gaia clone to re-compress assets before committing them. As an example I've run it on these new files and it yields a 20-30 reduction in size depending on the files, here's the sample invocation/output:

$ ./tools/png_recompress.sh -v apps/video/style/images/default_thumbnail.png \
                               apps/video/style/images/default_thumbnail@1.5x.png \
                               apps/video/style/images/default_thumbnail@2x.png

apps/video/style/images/default_thumbnail.png 154295 530 0.003
apps/video/style/images/default_thumbnail@1.5x.png 154524 675 0.004
apps/video/style/images/default_thumbnail@2x.png 154778 874 0.005

Number of files processed: 3
Total size of the files prior to recompression: 463597
Total size of the files after recompression: 2079
Compression ratio: 0.004

Note that the script requires the 'optipng' and 'advancecomp' programs to be installed to work properly.
Gabriele,

Thanks for that. I file a bug, bug 960845, about this and will apply that asap.
You need to log in before you can comment on or make changes to this bug.