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

Status: RESOLVED FIXED

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

Description

[cajbir (:cajbir)]

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.

Comment 1

[cajbir (:cajbir)]

Attachment: Ignore thumbnail generation error

Comment 2

[David Flanagan [:djf]]

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!

Comment 3

[cajbir (:cajbir)]

(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).

Comment 4

[cajbir (:cajbir)]

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

Comment 5

[David Flanagan [:djf]]

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?

Comment 6

[cajbir (:cajbir)]

(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.

Comment 7

[Casey Yee]

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

Flagging Patryk for visual design support.

Comment 8

[Patryk Adamczyk [:patryk] UX]

Peter La is working on thumbnails for media. He can provide the needed graphics.

Comment 9

[Peter La]

Attachment: 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.

Comment 10

[(Inactive after June) George Duan [:gduan] [:喬智]]

Attachment: 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?

Comment 11

[(Inactive after June) George Duan [:gduan] [:喬智]]

Hi Peter,
could you provide @1.5x image?

Comment 12

[John Hu [:johnhu][:johu][:醬糊小弟]]

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.

Comment 13

[cajbir (:cajbir)]

Comment on attachment 8349930 [details]
PR to master

Looks good to me but I'm unable to test on a device.

Comment 14

[Peter La]

Attachment: 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?

Comment 15

[(Inactive after June) George Duan [:gduan] [:喬智]]

Comment on attachment 8349930 [details]
PR to master

Hi John,
please kindly check it again. Thanks.

Comment 16

[John Hu [:johnhu][: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.

Comment 17

[(Inactive after June) George Duan [:gduan] [:喬智]]

Comment on attachment 8349930 [details]
PR to master

Thanks for commenting, patch is updated. Please check it again.
Thanks.

Comment 18

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 8349930 [details]
PR to master

Looks good to me. Please wait for travis result before landing code.

Comment 19

[(Inactive after June) George Duan [:gduan] [:喬智]]

Thanks,
Master: https://github.com/mozilla-b2g/gaia/commit/14d091833eafd2ea6c11ccb53444e9756df587e6

Comment 20

[Gabriele Svelto [:gsvelto] (PTO)]

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/[e-mail] \
                               apps/video/style/images/[e-mail]

apps/video/style/images/default_thumbnail.png 154295 530 0.003
apps/video/style/images/[e-mail] 154524 675 0.004
apps/video/style/images/[e-mail] 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.

Comment 21

[John Hu [:johnhu][:johu][:醬糊小弟]]

Gabriele,

Thanks for that. I file a bug, bug 960845, about this and will apply that asap.