996398 - Use fastSeek API in Gaia video app

Status: RESOLVED FIXED

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

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

I implemented HTMLMediaElement.fastSeek() in bug 778077. We already use fastSeek in the Gaia Browser App when seeking in <video> elements. This makes seeking faster, as we'll seek to keyframes, and won't decode to reach the precise seek target.

When using the touch controls it's not possible to seek precisely anyway, so I don't think sacrificing seek precision here is a big deal.

We can also use fsatSeek in thumbnailing. I saw the time taken to thumbnail a video decrease by 10x on the Tarako when using fastSeek.

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Pull request

Pull Request:

Use HTMLMediaElement.fastSeek() for seeking and thumbnailing in Gaia Video App.

Note: To test this patch, you'll want to ensure you also have the Gecko patch for bug 993753 in your Gecko, else seeking won't work properly.

Comment 2

[Dominic Kuo [:dkuo]]

Thanks Chris, I have filed bug 996410 for music, we should also do it for music app.

Comment 3

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Adding perf, [tarako] because this could be a nice backlog item for tarako.  It would be nice to have faster thumbnailing.

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from comment #3)
> Adding perf, [tarako] because this could be a nice backlog item for tarako. 
> It would be nice to have faster thumbnailing.

Note my patch here requires bug 778077 to be present in Gecko for it to work. Bug 778077 has only landed in Gecko 31 (B2G 1.5), so we'd need to back port bug 778077 and a couple of other bugs if we were to ship this in B2G v1.3t.

Comment 5

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

Comment on attachment 8406600 [details] [review]
Pull request

Thanks for this patch. The responsiveness looks better than before.

There is lint error in travis. Please check it before landing. Thanks.

Comment 6

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

Comment on attachment 8406600 [details] [review]
Pull request

Oh! Sorry. I forgot one thing. We also need to patch "view.js" file where also uses currentTime to change the position. The position is at: https://github.com/mozilla-b2g/gaia/blob/master/apps/video/js/view.js#L436

So, I clear the review flag.

Comment 7

[Ivan Tsay (:ITsay)]

Hi John,

Since this one blocks one of partner's request on 2.0 FL, please help to finish the patch and see if it works as expected. Per our discussion, it seems that there may have the issue on webM format when adapting the new API. If so, please also provide your finding and we can check with Chris what's going on based on your finding. Thanks you very much!

Comment 8

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

After retesting, the webm is slow to response in Inari with build 20140520040203 without this patch. So, it's not related to fastSeek. I can provide a patch which includes view.js.

Comment 9

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

Attachment: use fastSeek api in slider dragging and thumbnail creation

Russ,

Please review this patch. Most of the code are made by Chris. I add the one in |view.js|.

Comment 10

[Ivan Tsay (:ITsay)]

Remove Tarako from title since this is not device specific bug.

Comment 11

[Ivan Tsay (:ITsay)]

Nominate this bug to 2.0? since this one blocks the bug 984576

Comment 12

[Russ Nicoletti [:russn]]

Comment on attachment 8429881 [details] [review]
use fastSeek api in slider dragging and thumbnail creation

I believe there is an issue with the changes, so r-. My comments/questions are in the PR.

Comment 13

[Chris Pearce [:cpearce (Not reading bugmail)]]

I commented in the PR. I don't think the issues raised are a problem.

Comment 14

[Russ Nicoletti [:russn]]

Thanks for the explanations. I updated the PR with the one concern that I still have.

Comment 15

[bhavana bajaj [:bajaj]]

clearing the blocking flag, refer to the explanation in https://bugzilla.mozilla.org/show_bug.cgi?id=984576#c46

Comment 16

[Ivan Tsay (:ITsay)]

Hi Russ,

Since John is on PTO for a week this week, mind if you can help on this bug by addressing your concern on the patch and provide another one? We are approaching the 2.0 feature Landing deadline in 4 days and this is one of the feature needed by our partner.

Comment 17

[Russ Nicoletti [:russn]]

No problem, I will take care of it.

Comment 18

[Hema Koka [:hema]]

Putting this into sprint 3 milestone and assigning to Russ to help wrap up (as John is OOO this week)

Comment 19

[Russ Nicoletti [:russn]]

Attachment: PR request -- https://github.com/mozilla-b2g/gaia/pull/19955

I made minor changes to the original PR, including using fastSeek for the forward/rewind functionality.

Comment 20

[David Flanagan [:djf]]

Comment on attachment 8433680 [details] [review]
PR request -- https://github.com/mozilla-b2g/gaia/pull/19955

r- because the patch changes the behavior of the app in a couple of ways. See github.

Comment 21

[Russ Nicoletti [:russn]]

Comment on attachment 8433680 [details] [review]
PR request -- https://github.com/mozilla-b2g/gaia/pull/19955

Review comments have been addressed. Please review again.

Comment 22

[David Flanagan [:djf]]

Comment on attachment 8433680 [details] [review]
PR request -- https://github.com/mozilla-b2g/gaia/pull/19955

Looks good.

Comment 23

[Russ Nicoletti [:russn]]

Comment on attachment 8433680 [details] [review]
PR request -- https://github.com/mozilla-b2g/gaia/pull/19955

Sorry to do this to you but I implemented the behavior of pausing the video at the end when the user uses the forward button to seek to the end, so r? again.

Three files affected: video.js, forward_rewind_controller.js, and video_forward_rewind_controller_test.js

Comment 24

[David Flanagan [:djf]]

Comment on attachment 8433680 [details] [review]
PR request -- https://github.com/mozilla-b2g/gaia/pull/19955

r- because I think there is an easier way and because isSeekedToEnd doesn't get cleared if the user uses the slider bar. See my comments on github for details.

Comment 25

[Russ Nicoletti [:russn]]

Comment on attachment 8433680 [details] [review]
PR request -- https://github.com/mozilla-b2g/gaia/pull/19955

The latest review comments are now addressed. I'm calling pause() in ForwardRewindController when video seeks past the end and since we still get an ended event when paused and seeking to the end, playerEnded tests whether player is paused (in addition to whether slider is being dragged) to determine if it should continue to move video to beginning.

Comment 26

[David Flanagan [:djf]]

Comment on attachment 8433680 [details] [review]
PR request -- https://github.com/mozilla-b2g/gaia/pull/19955

Russ,

I had a few nits for you on github, but was pretty happy with the patch.

Until I noticed another issue that is basically not your fault.  It means r- and probably pushing this out of the 2.0 release.

Here's the main comment from Github. (See the github comments for the full version)

If I adb push test_media/Movies /storage/sdcard0/Movies using my Flame, and then open the Elephant's-dream movie, I find that if I'm somewhere between ~30 and ~50s in the movie and I press the forward button, it will jump 10s forward and then skip back to 43s. Always. So pressing and holding the forward arrow does not allow me to skip through this movie.

It is a webm movie, and perhaps it does not have enough key frames to make fastSeek() work as expected.

In any case, if fastSeek() is this unreliable we can't use it unless we have backup (like wait for the seeked event and if we didn't get close enough to the target fall back on currentTime.) I'd say that this is a serious enough bug that it should cause us to push this feature out until 2.1 

Chris: any idea what is going on here? Is this expected behavior for fastSeek()?  Is the movie test_media/Movies/elephants-dream.webm a really unusual one?  Is it normal to find movies that have key frames more than 10s apart? What algorithm would you recommend for skipping through a movie at approximately 10s intervals if fastSeek() can't be counted on to be accurate enough? Do we just not use it at all, or do we try it and fall back on currentTime?

Comment 27

[David Flanagan [:djf]]

Removing target-milestone and feature-b2g flags since it looks like we can't complete this for the 2.0 release.  This is a minor performance issue but not really a user-visible change.

Needinfo Hema and Sri so they know about this change.

Comment 28

[Chris Pearce [:cpearce (Not reading bugmail)]]

> Chris: any idea what is going on here? 

mkvinfo -v tells me that the cue points (keyframes) for elephants-dream.web are:

|+ Cues at 3441
| + Cue point at 3447
|  + Cue time: 0.000s at 3449
|  + Cue track positions at 3452
|   + Cue track: 1 at 3454
|   + Cue cluster position: 3937 at 3457
| + Cue point at 3461
|  + Cue time: 7.924s at 3463
|  + Cue track positions at 3467
|   + Cue track: 1 at 3469
|   + Cue cluster position: 3937 at 3472
|   + Cue block number: 677 at 3476
| + Cue point at 3481
|  + Cue time: 10.010s at 3483
|  + Cue track positions at 3487
|   + Cue track: 1 at 3489
|   + Cue cluster position: 3937 at 3492
|   + Cue block number: 837 at 3496
| + Cue point at 3501
|  + Cue time: 14.431s at 3503
|  + Cue track positions at 3507
|   + Cue track: 1 at 3509
|   + Cue cluster position: 3937 at 3512
|   + Cue block number: 1187 at 3516
| + Cue point at 3521
|  + Cue time: 16.433s at 3523
|  + Cue track positions at 3527
|   + Cue track: 1 at 3529
|   + Cue cluster position: 3937 at 3532
|   + Cue block number: 1333 at 3536
| + Cue point at 3541
|  + Cue time: 17.225s at 3543
|  + Cue track positions at 3547
|   + Cue track: 1 at 3549
|   + Cue cluster position: 3937 at 3552
|   + Cue block number: 1404 at 3556
| + Cue point at 3561
|  + Cue time: 18.852s at 3563
|  + Cue track positions at 3567
|   + Cue track: 1 at 3569
|   + Cue cluster position: 3937 at 3572
|   + Cue block number: 1516 at 3576
| + Cue point at 3581
|  + Cue time: 23.106s at 3583
|  + Cue track positions at 3587
|   + Cue track: 1 at 3589
|   + Cue cluster position: 3937 at 3592
|   + Cue block number: 1848 at 3596
| + Cue point at 3601
|  + Cue time: 27.068s at 3603
|  + Cue track positions at 3607
|   + Cue track: 1 at 3609
|   + Cue cluster position: 3937 at 3612
|   + Cue block number: 2174 at 3616
| + Cue point at 3621
|  + Cue time: 31.239s at 3623
|  + Cue track positions at 3627
|   + Cue track: 1 at 3629
|   + Cue cluster position: 2203027 at 3632
|   + Cue block number: 1 at 3637
| + Cue point at 3641
|  + Cue time: 38.496s at 3643
|  + Cue track positions at 3647
|   + Cue track: 1 at 3649
|   + Cue cluster position: 2203027 at 3652
|   + Cue block number: 572 at 3657
| + Cue point at 3662
|  + Cue time: 43.168s at 3664
|  + Cue track positions at 3668
|   + Cue track: 1 at 3670
|   + Cue cluster position: 2203027 at 3673
|   + Cue block number: 943 at 3678
| + Cue point at 3683
|  + Cue time: 61.853s at 3685
|  + Cue track positions at 3689
|   + Cue track: 1 at 3691
|   + Cue cluster position: 4338266 at 3694
|   + Cue block number: 1 at 3699
| + Cue point at 3703
|  + Cue time: 65.356s at 3705
|  + Cue track positions at 3709
|   + Cue track: 1 at 3711
|   + Cue cluster position: 4338266 at 3714
|   + Cue block number: 330 at 3719
| + Cue point at 3724
|  + Cue time: 69.944s at 3726
|  + Cue track positions at 3731
|   + Cue track: 1 at 3733
|   + Cue cluster position: 4338266 at 3736
|   + Cue block number: 848 at 3741
| + Cue point at 3746
|  + Cue time: 72.739s at 3748
|  + Cue track positions at 3753
|   + Cue track: 1 at 3755
|   + Cue cluster position: 4338266 at 3758
|   + Cue block number: 1081 at 3763
| + Cue point at 3768
|  + Cue time: 81.915s at 3770
|  + Cue track positions at 3775
|   + Cue track: 1 at 3777
|   + Cue cluster position: 4338266 at 3780
|   + Cue block number: 2032 at 3785
| + Cue point at 3790
|  + Cue time: 87.629s at 3792
|  + Cue track positions at 3797
|   + Cue track: 1 at 3799
|   + Cue cluster position: 4338266 at 3802
|   + Cue block number: 2482 at 3807
| + Cue point at 3812
|  + Cue time: 91.633s at 3814
|  + Cue track positions at 3819
|   + Cue track: 1 at 3821
|   + Cue cluster position: 4338266 at 3824
|   + Cue block number: 2751 at 3829
| + Cue point at 3834
|  + Cue time: 93.259s at 3836
|  + Cue track positions at 3841
|   + Cue track: 1 at 3843
|   + Cue cluster position: 6685373 at 3846
|   + Cue block number: 1 at 3851
| + Cue point at 3855
|  + Cue time: 95.261s at 3857
|  + Cue track positions at 3862
|   + Cue track: 1 at 3864
|   + Cue cluster position: 6685373 at 3867
|   + Cue block number: 174 at 3872
| + Cue point at 3876
|  + Cue time: 95.804s at 3878
|  + Cue track positions at 3883
|   + Cue track: 1 at 3885
|   + Cue cluster position: 6685373 at 3888
|   + Cue block number: 211 at 3893
| + Cue point at 3897
|  + Cue time: 97.806s at 3899
|  + Cue track positions at 3904
|   + Cue track: 1 at 3906
|   + Cue cluster position: 6685373 at 3909
|   + Cue block number: 350 at 3914
| + Cue point at 3919
|  + Cue time: 98.556s at 3921
|  + Cue track positions at 3926
|   + Cue track: 1 at 3928
|   + Cue cluster position: 6685373 at 3931
|   + Cue block number: 413 at 3936
| + Cue point at 3941
|  + Cue time: 106.689s at 3943
|  + Cue track positions at 3948
|   + Cue track: 1 at 3950
|   + Cue cluster position: 6685373 at 3953
|   + Cue block number: 1197 at 3958
| + Cue point at 3963
|  + Cue time: 111.402s at 3965
|  + Cue track positions at 3970
|   + Cue track: 1 at 3972
|   + Cue cluster position: 6685373 at 3975
|   + Cue block number: 1699 at 3980

So you can see there's no keyframes between 43.168s and 61.853s.

There is no way to control how any arbitrary media on the web encodes its keyframes. We can control the keyframe distribution in the files we encode, but not the files other people encode.

So I recommend you only use fastSeek() for seeking with touch controls, as the input with a touch progress bar is inprecise enough that the user probably won't realise they're not seeking precisely.


> Is this expected behavior for
> fastSeek()?

Yes.

> Is the movie test_media/Movies/elephants-dream.webm a really
> unusual one?

I don't think so. I imagine movies encoded for the web won't have keyframe intervals too large. The tool ffmpeg2theora, which is commonly encodes to Ogg videos, uses a keyframe interval of 2 seconds. I don't know what the defaults for other encoding tools are.

> Is it normal to find movies that have key frames more than 10s
> apart?

We don't have any hard data on this. My intuition says "no", but I have no data to back that up.

> What algorithm would you recommend for skipping through a movie at
> approximately 10s intervals if fastSeek() can't be counted on to be accurate
> enough?

I recommend you use an accurate v.currentTime seek for seeking when accuracy is important (as in this case) and fastSeek() when it's not (seeking the using the main progress bar in touch controls).

Comment 29

[bhavana bajaj [:bajaj]]

(In reply to David Flanagan [:djf] from comment #27)
> Removing target-milestone and feature-b2g flags since it looks like we can't
> complete this for the 2.0 release.  This is a minor performance issue but
> not really a user-visible change.
> 
> Needinfo Hema and Sri so they know about this change.

Please note, we have committed to fix Bug 984576 to CAF, which this bug blocks, so we will have to uplift this in the next few days to resolve the issue if it missed the 2.0 boat before merge.

Comment 30

[Russ Nicoletti [:russn]]

David, what is your opinion of using fastSeek for the slider and currentTime for forward/rewind -- as cpearce recommends (comment #28)? I believe this approach works well because when using the slider the user won't expect precise seeking but will appreciate the performance. Using currentTime for the forward/rewind buttons will give us the seeking precision even if it has issues with some videos. I believe the lack of key frames in some videos will cause us problems regardless of which approach we use but my testing has shown that using currentTime in this case is the better approach.

Comment 31

[David Flanagan [:djf]]

Russ: what you describe seems reasonable in the context of this bug, but I worry that it won't actually satisfy CAF in bug 984576, so I'd like to try doing a little more.

It seems to me that the fundamental issue here is that fastSeek() can in some cases (when videos have sparse keyframes) go in the wrong direction. I've filed bug 1022913 about that.  If I can create a shim that uses fastSeek but falls back on currentTime to ensure that it seeks in the right direction, then I'd prefer to use your original patch that uses fastSeek everywhere, plus a shim I'll create that works around bug 1022913.

So let's leave the current pull request that uses fast seek everywhere.  If you've got a version that uses fastSeek only for the slider, then attach that, too, please.

I'll try to create the fastSeek() shim, and we'll land one or the other this afternoon.  Sound good?

Comment 32

[David Flanagan [:djf]]

It turns out that maybe I don't need to create a fastSeek() shim because Chris Pearce already has a work-in-progress patch in bug 1022913 ready for testing.

Reviewing the status of the current pull request, I think there are still a couple of minor changes we need to make before we can land it. Chris's a patch in bug 1022913 will solve the major issue that caused an r- on this patch.  I think we can land before 1022913 lands.

But there is still the issue of the synchronous use of the asynchronously updated currentTime property. Rather than updating the displayed time when the user clicks the forward or backward buttons we should instead wait for the next 'seeked' or 'timeupdate' event do update the time, so we don't change it more than necessary.

At this point it doesn't look like this fix will land before the 2.0 branch happens tonight. But we should be able to get the fix in tomorrow.

Russ: if you're able to fix those minor comments this afternoon, I can review and land tonight. Otherwise, I'll take the bug tomorrow and try to land it and uplift.

Comment 33

[Russ Nicoletti [:russn]]

I tried making the change you suggested regarding. I found that currentTime in the 'seeked' handler (I used updateVideoControlSlider) while handling the first 'seeked' event after making the fastSeek call is the same as 'seekTime'. However, the very next time updateVideoControlSlider is called, currentTime changes to a couple of seconds less than seekTime. I'm not sure what's going on. I'm not going to put any more effort into this right now. I'm going to work on porting the 1002593 patch to 1.4 -- there is no single 1.3t patch that can be uplifted to 1.4; all the related 1.3t changes were rolled up into the 1002593 patch to master.

Comment 34

[David Flanagan [:djf]]

We didn't get this landed by the 2.0 FL deadline because we were de-railed by bug 1022913 (and because Russ was too busy with bug 1002593.)  Taking this myself now and will try to finish it up.

Comment 35

[David Flanagan [:djf]]

Attachment: a new version of the PR built on top of Russ's patch

Comment 36

[Russ Nicoletti [:russn]]

Comment on attachment 8438002 [details] [review]
a new version of the PR built on top of Russ's patch

Looks good. One nit regarding the comment in forward_rewind_controller.js, seekVideo.

Comment 37

[David Flanagan [:djf]]

Okay, once Travis runs, I think we'll be able to land this.

Requesting 2.0+ since this was a feature we commited to for 2.0.

Comment 38

[David Flanagan [:djf]]

Landed to master: https://github.com/mozilla-b2g/gaia/commit/bfc48b690164b89dededb8ccf74f4a4f3ae02977

Comment 39

[David Flanagan [:djf]]

uplifted to v2.0: https://github.com/mozilla-b2g/gaia/commit/856970e46643317e322d5d72840490a42c3282de

Comment 40

[Hema Koka [:hema]]

(In reply to David Flanagan [:djf] from comment #39)
> uplifted to v2.0:
> https://github.com/mozilla-b2g/gaia/commit/
> 856970e46643317e322d5d72840490a42c3282de

Thanks David!