Leo Reporter
	Description • 12 years ago

1. Title : Limitation of having more than one instance of hardware decoder for video player
2. Precondition : video player is going to be launched for first time
3. Tester's Action : Selecting a video file while the video application is retrieving thumbnail for the first time when it starts
4. Detailed Symptom (ENG.) : As there is limitation in our hardware, we can't have more than one instance of hardware decoder a time. So, H/W codec returns error when we try to play a video file while the video application is retrieving thumbnail for the first time after bootup.
5. Gaia Revision # :   " a78ebf426840b5ef08c0cc3e437ad30aba3e2528 "
6. Expected : video player should prevent showing video files list until it retrieves all thumbnail information of video files.
7.Reproducibility: Yes
                1)Frequency Rate : 100%
8.Comparison Results : 
                1)Model Comparing : 
9. Attached files: 
                1)Log : 
                2)Test Contents : 
                3)Video file :

	Lukas Blakk [:lsblakk] use ?needinfo
	Comment 2 • 12 years ago

QA can you confirm if this is device specific?

	cajbir (:cajbir)
	Comment 3 • 12 years ago

The last time we came up with this it was worked around in Gaia by making sure only one video was active at a time. Is that not sufficient?

	Alex Keybl [:akeybl]
	Comment 4 • 12 years ago

(In reply to Chris Double (:doublec) from comment #3)
> The last time we came up with this it was worked around in Gaia by making
> sure only one video was active at a time. Is that not sufficient?

What bug was that? Can you reassign to whoever did this previously?

	cajbir (:cajbir)
	Comment 5 • 12 years ago

I'm not sure what bug it was as it was a Gaia bug, managed in Github, which I wasn't following. Since this bug seems to be in the Gaia video player I'll assign to the last person who worked on that.

	Naoki Hirata :nhirata (please use needinfo instead of cc)
	Comment 6 • 12 years ago

Wouldn't that become an issue if : https://bugzilla.mozilla.org/show_bug.cgi?id=849768 occurs?  because then one could hit the home button and try to watch more videos.  Should the current video stop and the newly selected video play in that instance?  Also what occurs when you have a youtube video from browser app that gets played?  I might be over thinking things...

	Naoki Hirata :nhirata (please use needinfo instead of cc)
	Comment 7 • 12 years ago

I don't believe this is hardware dependent.

Gecko  http://hg.mozilla.org/releases/mozilla-b2g18/rev/da523063aa7b
Gaia   a845be046c5d3cb077e3c78f963ca5c079e7ab3d
BuildID 20130404070202
Version 18.0
Unagi

I think it was tested on a different device than the unagi when originally reported?

	cajbir (:cajbir)
	Comment 8 • 12 years ago

At the time this issue (that of only one hardware decoder instance working) it was decided (by cjones IIRC) that the gaia workaround of clearing out the currently playing instance before starting another was suitable. At that time (and it still is for 1.0 I think) the browser couldn't play H.264 videos using the hardware decoder due to permission issues. 

It's unfortunate that this occurs on qualcomm devices. The issue does not exist on other non-qualcomm devices I've used b2g on (referring to the 'is it hardware dependent' question).

	cajbir (:cajbir)
	Comment 9 • 12 years ago

As per comment 4, reassigning to the person who worked on the Gaia workaround previously in bug 837236.

	David Flanagan [:djf] Assignee
	Comment 10 • 12 years ago

The reporter is absolutely right about the diagnosis of this bug. The suggested workaround is bad UX, however, and I think we can do better.

The camera app saves a poster image for each video it records, but for all other videos, we need to generate a poster and thumbanil, and we do that by playing the video in an off-screen <video> element. Our hardware allows only one h.264 video to play at a time. Free video formats like ogg have software decoders, so we're okay there, too. But for h.264 video other than the .3gp files from the camera, we cannot scan metadata (i.e. generate poster images) and play at the same time.

The obvious work around is to not allow the user to play videos until scanning is done. The simplest fix would to just do an alert() if the user taps on a video while scanning is happening. We'll need new strings to display in the dialog. A fix like this would take an hour or so, and just about anyone should be able to do it.

We might be able to do better by taking the poster/thumbnail image generation responsbility away from the metadata parser function passed to the mediadb library and handling the image generation separately. That is, we could display video filenames without thumbnails as soon as we locate the file. Then we'd generate thumbnails during idle time. If the user taps on a video to play it we stop generating the images and play the video. When the video stops playing, we resume generating images.  Note that the app has to be open (the active app) in order for us to generate images, because the video, camera, and gallery apps cooperate by only using the hardware video decoder while they are the active app. 

This is trickier and would require more testing, but seems like the right way to go. I'm guessing it would be day's worth of work. I'm probably the right one to make this fix, though Dale or Dominic might be candidates, too.

I'm likely to be tied up with keyboard auto-correct and word suggestion work for the rest of this week, so if anyone wants to steal the bug that is okay with me.

	Casey Yee
	Comment 11 • 12 years ago

From a UX perspective, if we can avoid having to block the user from playing videos that would be much preferred.

	Dylan Oliver [:doliver]
	Comment 12 • 11 years ago

David, can you provide current status of this issue?

	David Flanagan [:djf] Assignee
	Comment 13 • 11 years ago

I'm doing final testing on the patch right now. I'll attach it tonight And hope that Dale or Dominic will be able to review tomorrow.

	David Flanagan [:djf] Assignee
	Comment 14 • 11 years ago

This patch fixes the bug where the video app could try to play a video at the same time as it was parsing metadata. But we can only have one video tag (with h.263 video) active at a time so the metadata parsing would fail.

See the new file metadata.js: the comments at the top explain the situation well.
The old metadata parsing code from video.js has been moved to this new file, refactored, and a new queuing mechanism has been added that is independent of the mediadb.

As part of this, I've moved the MediaDB initialization code to another new file, db.js. I needed to change enough of that code that it just seemed simplest to put it in a separate file.

In the process of doing this, I also found and fixed a lot of other minor Video app bugs.  I don't know if any of these have been filed, but I've fixed these things:

- Startup time is faster because the enumeration code batches its results rather than creating and inserting a new thumbnail each time the enumeration returns an item. This makes a really big difference if you have lots (50+) videos.

- The scanning progress bar was not working. It works now, but it is now displayed during metadata parsing, rather than scanning, since it is the metadata parsing that takes up most of the time.

- The thumbnails were being generated at 160x160 and then made to fit a 210x120 space. They didn't look good being resized like that. Now they're generated and saved at the right size.

- There were still two calls to mozCancelFullScreen() in the code. They're gone now.

- When we deleted a file from the video player, we were staying in the player mode, and continued to display the deleted video. Now we return to the thumbnails list after deleting a file.

- When tapping on a thumbnail to display it, there was no feedback. Now there is a :active style to show that something is going to happen if you release your finger.

- There was an off-by-one bug where sometimes (after a new video was inserted or an old one deleted, maybe?) tapping on a thumbnail would play the video above it. That's fixed now.

- we no longer update the metadata of videos that are viewed while handling a pick request.

- If we started playing a video and then used the back button to return to the thumbnail list without waiting for the video to finish and without pausing it, we would never unlock the screen, and it would not blank. That's fixed, I think.

None of these assorted fixes took much code to fix, but you'll see the various changes scattered around the app, and I'm listing the bugs here so you'll know why I was making those unrelated changes.

	Dominic Kuo [:dkuo]
	Comment 15 • 11 years ago

Comment on attachment 745030 [details]
link to patch on github

David,

I think this patch looks really nice! Although this is a big patch that modified lots of code, but I think there is no obvious defect of the new parsing logic, also plenty of minor bugs are fixed and even the startup time is faster!, so it's worthy to make these changes and let's land it.

	David Flanagan [:djf] Assignee
	Comment 16 • 11 years ago

Landed on master: https://github.com/mozilla-b2g/gaia/commit/cf932ff85b56f46c55fa89be46d6a1168075aab4

	David Flanagan [:djf] Assignee
	Comment 17 • 11 years ago

Uplifted to v1-train: https://github.com/mozilla-b2g/gaia/commit/d5f7b2d3f7acc04c35abda8b001262d8912f9dfe

	David Flanagan [:djf] Assignee
	Comment 18 • 11 years ago

Setting QA wanted because this is kind of a tricky bug to verify and because (as you can see from comment 14) it was a fairly extensive patch.  Please be on the lookout for regressions in the Video app, and, if time allows, give it a little extra testing love.

In order to verify the main bug that this fixes, you'll probably want to copy a bunch of videos (20 or more so you have lots of scanning time) and then launch the app. You'll want to verify that you can play videos while metadata scanning is happening.  (Actually what you're verifying is that scanning stops when videos are played).  Metadata scanning also stops if you return to the homescreen or launch other apps and should resume when you return to the video app.

	Jason Smith [:jsmith]
	Comment 19 • 11 years ago

Switching keywords to verifyme as this is a verification request.

Marcia - Can you help address the testing request in comment 18?

	Marcia Knous [:marcia]
	Comment 20 • 11 years ago

Using this build on Inari:

Gecko  http://hg.mozilla.org/releases/mozilla-b2g18/rev/884ad1bbe24e
Gaia   1bac83700810f27e00a937e34e7c865da02e0215
BuildID 20130507070204
Version 18.0

I loaded a fair number of videos onto my SD card yesterday. All videos were various lengths, some were movies with over 1 hour of playing time.

While videos are loading, I am having a difficult time selecting another video to play. On this device in the beginning, I couldn't select any videos. At some point I was able to, but it takes me at least three times pressing the video for it to start playing. It looks as if a threshold was reached where I could play one while scanning was going on, but it definitely did not happen right at the beginning when the scanning started.

I have a 32 GB card installed, of which videos are taking up 3.5 GB. There is still 24.5 GB space left according to Media Storage.

	Sotaro Ikeda [:sotaro]
	Comment 21 • 11 years ago

djf, is it possible to uplift the fix to tef? The problem happens also on v1.0.1.

	David Flanagan [:djf] Assignee
	Comment 23 • 11 years ago

It should be possible to uplift this, but see comment 14: the patch was a wide-ranging one and fixed a number of bugs in addition to the major one called out in the description.  (It improves startup performance and also improves the appearance of the thumbnails).

I'm happy to try to uplift it if we get approval.  Or, if it seems risky, I can try to just uplift the parts relevant to the hardware contention issue.

870071 was nominated for tef, and it has been closed as a duplicate of this one, so I'm requesting tef? on this one now.  I'm not sure if that works since this has been closed, however.

	David Flanagan [:djf] Assignee
	Comment 24 • 11 years ago

Changing the status-b2g18-1.0.1 flag to ? in case anyone is monitoring that and can grant permission for uplift.

	Sotaro Ikeda [:sotaro]
	Comment 25 • 11 years ago

djf, thanks to nominating to tef. I think it is necessary to uplift to tef, even by the risk in comment 14. This bug should be fixed also in v1.0.1. I prefer uplift almost all changes to v1.0.1. To share code between v1.1 is lower risk in total, I think. I hope the bug get tef+.

	Daniel Coloma:dcoloma
	Comment 26 • 11 years ago

(In reply to Sotaro Ikeda [:sotaro] from comment #25)
> djf, thanks to nominating to tef. I think it is necessary to uplift to tef,
> even by the risk in comment 14. This bug should be fixed also in v1.0.1. I
> prefer uplift almost all changes to v1.0.1. To share code between v1.1 is
> lower risk in total, I think. I hope the bug get tef+.

Sotaro, this bug has morphed since it was initial created. Can you provide a short description on how this affects end-users?

	Sotaro Ikeda [:sotaro]
	Comment 27 • 11 years ago

The bug still handles same problem as comment #0. User failed to start video playback during thumbnail generation.

>4. Detailed Symptom (ENG.) : As there is limitation in our hardware, we can't have >more than one instance of hardware decoder a time. So, H/W codec returns error when >we try to play a video file while the video application is retrieving thumbnail for ?the first time after bootup

	James Zhang (Spreadtrum)
	Comment 28 • 11 years ago

Hi Sotaro, can you see bug 866028 patch?
I think that using software codec to generate thumbnail and hardware codec to playback maybe another way to resolve this issue.

	Alex Keybl [:akeybl]
	Comment 29 • 11 years ago

STR as we understand them:

1) Load a bunch of videos on the SD card, or take a number of videos without opening the Video app
2) Open the video app
3) Try to play a video

Video won't play until thumbnailing is complete.

Risk here doesn't warrant fixing this one-off user scenario.

	Parul Mathur
	Comment 30 • 11 years ago

Created test case in MozTrap https://moztrap.mozilla.org/manage/case/8791/

Executed test case and verified fixed on:
Device: Leo phone
Build Identifier: 20130621070212
Update channel: leo/1.1/nightly
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/a34f6d62cb05
Gaia: cca61224e6df8e9f7e450d77cb6a8cf91029be641371823447
Git commit info: 2013-06-21 14:04:07
OS version: 1.1.0.0-prerelease