Closed Bug 984576 Opened 7 years ago Closed 6 years ago

Lag observed when seeking to a new position while playing video files

Categories

(Core :: Audio/Video, defect)

30 Branch
ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
2.0 S4 (20june)
feature-b2g 2.0
Tracking Status
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: bhargavg1, Assigned: rnicoletti)

References

Details

(Whiteboard: [caf priority: p3][CR 623423])

Observe that it takes time for the playback to start when seeking to a new position while doing video playback
blocking-b2g: --- → 1.4?
What device? What build ID? What Android OS version?
Build IDs which I tested on

GAIA:66fb457e892af3463bb4fbd1461c7e67865c416e
GECKO:0bacffad36f65a27929ca85164792750c695c3cb

Android OS: Kit-Kat

Device: 8x26 

I guess we can see that it takes time to start playing again, quite easily seen on most of the builds
Blocks: gonk-kk
What is the acceptable time?
Flags: needinfo?(bhargavg1)
CJ.

Please review
Flags: needinfo?(cku)
Component: Gaia::Video → Video/Audio
Product: Firefox OS → Core
Version: unspecified → 30 Branch
(In reply to Preeti Raghunath(:Preeti) from comment #3)
> What is the acceptable time?

Dont think we have a bench mark for the timing, except for that to be smooth.
Flags: needinfo?(bhargavg1)
Currently Gecko does accurate seek. That is, if you seek to 1.23242 seconds into a video, we'll seek to the preceding keyframe, and then decode the media forward until we reach exactly 1.23242 in the video and audio streams. We seek to the closest audio sample. The delay that you're seeing may be the time we're spending decoding forward from the preceding keyframe to the seek target.

I'm working on implementing HTMLMediaElement.fastSeek() in bug 778077.
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#dom-media-fastseek

When using fastSeek() we will skip the "decode forward to the seek target" step. We should probably use this in the video controls instead of precise seek, since it's pretty much impossible to give precise inputs via touch controls anyway. This should reduce the delay when seeking.
Depends on: 778077
(In reply to Chris Pearce (:cpearce) from comment #6)
> step. We should probably use this in the video controls instead of precise
> seek, since it's pretty much impossible to give precise inputs via touch
> controls anyway. This should reduce the delay when seeking.

There are other players that do this, and it is fine until some joker makes a video file that goes minutes between keyframes. Then it is very frustrating.
Bhargav,

Can we please have a video of the same? Lets get a visual on KK device and JB device to understand the lag here?

What was the time it took in 1.3?
Flags: needinfo?(bhargavg1)
(In reply to Timothy B. Terriberry (:derf) from comment #7)
> (In reply to Chris Pearce (:cpearce) from comment #6)
> > step. We should probably use this in the video controls instead of precise
> > seek, since it's pretty much impossible to give precise inputs via touch
> > controls anyway. This should reduce the delay when seeking.
> 
> There are other players that do this, and it is fine until some joker makes
> a video file that goes minutes between keyframes. Then it is very
> frustrating.

Just tell gmaxwell to stop doing that.  ;)

To a certain degree, we've already lost when playing videos with large keyframe distances, as if we're doing exact seek on low end hardware, the seek will take ages, and that's frustrating too.

I'm not suggesting we use fastSeek in video controls on desktop, only mobile, where the scrubber bar is only a few times wider than the finger pressing it.
Hi Bruce,
Is this issue relative to your work?
Flags: needinfo?(cku) → needinfo?(brsun)
(In reply to C.J. Ku[:CJKu] from comment #10)
> Hi Bruce,
> Is this issue relative to your work?

Hi CJ,
Not so relevant. Changing the codec from sync to async doesn't help on this issue. Users still need to wait necessary video data to be downloaded before they can see the video frame.
Flags: needinfo?(brsun)
Please provide benchmark numbers for 1.3. It will be easy to compare against.
CJ

Is this reproducible on the QRD?
Flags: needinfo?(cku)
Sorry, have no QRD in team. 

Preeti, I think Eric Chou's team is more suitable to give you an answer on this bug, since they are working on B2G media playback.

NI Eric here to let him get notice on this bug.
Flags: needinfo?(cku) → needinfo?(echou)
bhargav, can you please renominate this once you have the additional information requested. At this time we cannot make a blocking decision here with the info at hand ?
blocking-b2g: 1.4? → ---
Flags: needinfo?(bhargavg1)
Whiteboard: [CR 623423]
IIRC, Bug 973408 is recent big change that could affect to seek.
:bajaj -- can you please not remove the 1.4? nom when we are actively working on the bug. Bhargav is working on providing this info and we also have daily sync-up to track it. So, no need to remove the nom here. Thanks.
blocking-b2g: --- → 1.4?
Adding back the ni on Bhargav.
Flags: needinfo?(bhargavg1)
Remove echou's ni since he is aware of this issue already.

bhargavg1:
May I know what use case you are referring to?
 - 1. Approximate seek for speed: In some scenarios, users just want to see the video frame as quick as possible after seeking, and probably they don't care very much about whether the displayed video is the same video frame at the seek position or not. For example, users just want to skip some period of video content that they are not interested in. In this case, probably the nearest individual frame would be a good choice to be played from.
 - 2. Frame-accurate seek: In some scenarios, users want to see the exact video frame at the seek position. For example, users might want to step the video frame-by-frame. Since some referenced video frames are needed to be decoded in advance before the exact frame can be decoded, users might experience some lead time before the exact frame can be displayed.

Please kindly share what scenario you are testing and what kind of lag is acceptable.
Flags: needinfo?(echou)
(In reply to Preeti Raghunath(:Preeti) from comment #8)
> Bhargav,
> 
> Can we please have a video of the same? Lets get a visual on KK device and
> JB device to understand the lag here?
> 
> What was the time it took in 1.3?

Shared clip at, https://drive.google.com/file/d/0B0zTAnPOpx-xV0VwdGtBRjk3VG8/edit?usp=sharing
Flags: needinfo?(bhargavg1)
(In reply to bhargavg1 from comment #20)
> (In reply to Preeti Raghunath(:Preeti) from comment #8)
> > Bhargav,
> > 
> > Can we please have a video of the same? Lets get a visual on KK device and
> > JB device to understand the lag here?
> > 
> > What was the time it took in 1.3?
> 
> Shared clip at,
> https://drive.google.com/file/d/0B0zTAnPOpx-xV0VwdGtBRjk3VG8/edit?usp=sharing

Can you attach or provide to the video you used to reproduce this bug?
(In reply to Jason Smith [:jsmith] from comment #21)
> (In reply to bhargavg1 from comment #20)
> > (In reply to Preeti Raghunath(:Preeti) from comment #8)
> > > Bhargav,
> > > 
> > > Can we please have a video of the same? Lets get a visual on KK device and
> > > JB device to understand the lag here?
> > > 
> > > What was the time it took in 1.3?
> > 
> > Shared clip at,
> > https://drive.google.com/file/d/0B0zTAnPOpx-xV0VwdGtBRjk3VG8/edit?usp=sharing
> 
> Can you attach or provide to the video you used to reproduce this bug?

provide a link*
Once Bug 778077 has landed (should be up for review in a the next few working days), we can test changing to using that in the video app and default controls, and should that make average seek times faster, though less precise.
(In reply to Bruce Sun [:brsun] from comment #19)
> Remove echou's ni since he is aware of this issue already.
> 
> bhargavg1:
> May I know what use case you are referring to?
>  - 1. Approximate seek for speed: In some scenarios, users just want to see
> the video frame as quick as possible after seeking, and probably they don't
> care very much about whether the displayed video is the same video frame at
> the seek position or not. For example, users just want to skip some period
> of video content that they are not interested in. In this case, probably the
> nearest individual frame would be a good choice to be played from.
>  - 2. Frame-accurate seek: In some scenarios, users want to see the exact
> video frame at the seek position. For example, users might want to step the
> video frame-by-frame. Since some referenced video frames are needed to be
> decoded in advance before the exact frame can be decoded, users might
> experience some lead time before the exact frame can be displayed.
> 
> Please kindly share what scenario you are testing and what kind of lag is
> acceptable.

This is more from UX perspective, I see moving to the nearest position should be fine
CJ

please review the video and check if we need anything else from QC.
Flags: needinfo?(cku)
Hi Preeti,

(In reply to Preeti Raghunath(:Preeti) from comment #25)
> CJ
> 
> please review the video and check if we need anything else from QC.

Please see comment 14. CJ's team takes media recording bugs, not media playback bugs. Please ni? me directly next time you have device-specific media playback issues. Thank you.

Per comment 23 & 24, I think Chris' fastSeek API can fulfill the reporter's requirement. I suggest that we should keep this bug open, once bug 778077 has landed, we could use this bug to track the implementation of Video app, which can improve its seeking time by using fastSeek(). (Not exactly sure if this is the best answer, ni? Bruce again to double confirm)
Flags: needinfo?(cku) → needinfo?(brsun)
From comment 24, it seems this bug is referring to the similar problem that fastseek api is going to solve. Probably the necessary modification could be done in gecko without modifying gonk, but I am not very sure.

cpearce: do you have any plan on gonk regarding to fastweek api implementation?
Flags: needinfo?(brsun) → needinfo?(cpearce)
Eric,

Do we need anything from QC? We would like to understand if we can have a patch 3/27?
Flags: needinfo?(echou)
(In reply to Bruce Sun [:brsun] from comment #27)
> From comment 24, it seems this bug is referring to the similar problem that
> fastseek api is going to solve. 

Yes.

> Probably the necessary modification could be
> done in gecko without modifying gonk, but I am not very sure.

Yes, we should only need to modify gecko.


> cpearce: do you have any plan on gonk regarding to fastweek api
> implementation?

I will implement this in gecko for Gecko 31/B2G 1.5 in bug 778077, and then we can consider backporting if desired.
Flags: needinfo?(cpearce)
Is there any particular reason that we should consider to have this bug as a 1.4 blocker?
(In reply to Preeti Raghunath(:Preeti) from comment #28)
> Eric,
> 
> Do we need anything from QC? 

Please see comment 26. All we need to do with this bug are (1) fix bug 778077, which Chris has been already dealing with, and (2) let Video app use fastSeek() API.

> We would like to understand if we can have a patch 3/27?

No. AFAIK the best solution should be waiting for bug 778077 landing on 1.5, then we can decide if we're going to backport to 1.4. Moreover, I don't even think this should be a 1.4 blocker since we don't even have a benchmark to define how 'fast' the seek should be from UX perspective.
Flags: needinfo?(echou)
Marvin - Can you make a product call on what release this should block, if any? This would require implementing a new feature (see the dependency here), so if we take this into 1.4, then we're going to add a feature after FC.
Flags: needinfo?(mkhoo)
(In reply to Eric Chou [:ericchou] [:echou] from comment #31)
> (In reply to Preeti Raghunath(:Preeti) from comment #28)
> > Eric,
> > 
> > Do we need anything from QC? 
> 
> Please see comment 26. All we need to do with this bug are (1) fix bug
> 778077, which Chris has been already dealing with, and (2) let Video app use
> fastSeek() API.
> 
> > We would like to understand if we can have a patch 3/27?
> 
> No. AFAIK the best solution should be waiting for bug 778077 landing on 1.5,
> then we can decide if we're going to backport to 1.4. Moreover, I don't even
> think this should be a 1.4 blocker since we don't even have a benchmark to
> define how 'fast' the seek should be from UX perspective.

Thanks Eric. Will provide the same feedback to QC.
Flags: needinfo?(ikumar)
Thanks for feedback, I have communicated it to test teams and will wait for their feedback on the same.
Hi Jason, this improvement is planned in 1.5, consider 1.4 is under stabilization, adding feature work or pull in may bring concerns / work from various team. this should not be blocker, improvement will be made in 1.5, backporting to 1.4 is an option.
Flags: needinfo?(mkhoo)
blocking-b2g: 1.4? → 1.5?
As per test team's feedback it's not a good UX to see pauses in 30fps playback. I am moving it to CS blocking to see if we can backport this fix and at least fix this by CS timeframe.
Flags: needinfo?(ikumar)
(In reply to Marvin Khoo [:Marvin_Khoo] from comment #35)
> Hi Jason, this improvement is planned in 1.5, consider 1.4 is under
> stabilization, adding feature work or pull in may bring concerns / work from
> various team. this should not be blocker, improvement will be made in 1.5,
> backporting to 1.4 is an option.

I don't think it makes sense to consider backporting a feature - that poses regression risk to the product, which is going to risk making the product stability worse. We can certainly get a risk assessment here though if need be.

(In reply to Inder from comment #36)
> As per test team's feedback it's not a good UX to see pauses in 30fps
> playback. I am moving it to CS blocking to see if we can backport this fix
> and at least fix this by CS timeframe.

Something that isn't great UX doesn't necessarily make a blocker. In order to block here, we really need a clarification of why this is absolutely needed for 1.4 specifically. What partner use cases does this block for 1.4? Are there partner apps requiring this? Product is currently against blocking on this, so I suggest following up with the product team here to clarify what the 1.4 specific need is here.
Jason - I kept it in CS blocking because it was suggested that backporting is an option and so we could track the work. If we are not planning to do it at all for v1.4 then I guess we can remove the blocker. The use case is simply to not see pauses during seek in 30fps playback. The request was from test team's perspective where we try to make sure the UX is similar to other OSes on the same chipset.
From an IRC discussion with Chris Pearce:

There is some regression risk to taking the required dependencies here. It's changing the logic for determining a playback position after a seek. Chris wants to bake this first for a few weeks before we consider uplifting it. The implementation does make the product nicer overall. The risk is that playback position could be wrong after seeking, which is an unharmful thing to UX if there does end up being a bug in there. Fail case is - user seeks to time T, Video App reports ending up at time X, user can still play and playback still ends when end of media is reached. We normally don't uplift features, so we need a strong case on why this is required on 1.4.
The ask is for 1.5. Will be tracked to block 1.5 QC FC
No longer blocks: gonk-kk
Chris - With the dependent bug fixed, is this bug fixed as well?
Flags: needinfo?(cpearce)
(In reply to Jason Smith [:jsmith] from comment #41)
> Chris - With the dependent bug fixed, is this bug fixed as well?

For <video> in the Browser App yes. For video in the Video App no, bug 996398 needs to land to improve that. I haven't had a chance to address the review comments on that, someone else is welcome to take it off me if they wish.
Flags: needinfo?(cpearce)
Depends on: 996398
ni? to Ivan as he knows who to take look into.
nominate 2.1? as CAF requests for 2.1.
blocking-b2g: 2.0? → 2.1?
Flags: needinfo?(itsay)
(In reply to Wesley Huang [:wesley_huang] from comment #43)
> ni? to Ivan as he knows who to take look into.
> nominate 2.1? as CAF requests for 2.1.
Sorry, request is on 2.0
blocking-b2g: 2.1? → 2.0?
This one is more like a meta bug for lag issue. Need to finish the dependent bug 996398. Already find the engineer to work on bug 996398 and evaluate it to see if we can get it done in 2.0 FL time frame. Will update in the bug 996398.
Flags: needinfo?(itsay)
Clearing the blocking flag as this does not technically block ship the release and rather setting the feature-b2g:2.0 as this is targeted by 2.0 FL.
blocking-b2g: 2.0? → ---
feature-b2g: --- → 2.0
Ivan, please assign a dev owner for if this is still targeted for v2.0 FL.  Also, if this is a meta bug to track Lag issue, please change the summary title to include [Meta].
Assignee: nobody → itsay
Whiteboard: [CR 623423] → [caf priority: p3][CR 623423]
Hi Hema,

Mind if you can take this bug and help to monitor the feature on video app to use new fask seek API (). As far as I understand, once the bug 996398 is landed, this one is resolved as well.
Flags: needinfo?(hkoka)
(In reply to Ivan Tsay (:ITsay) from comment #48)
> Hi Hema,
> 
> Mind if you can take this bug and help to monitor the feature on video app
> to use new fask seek API (). As far as I understand, once the bug 996398 is
> landed, this one is resolved as well.

Russ has agreed to address the review comments for bug 996398 and try to wrap it up before FL since John is out. Once that is done, we need to ensure that it indeed fixes the lag (If it does not, Eric's team is probably the best to take this up). 

I will assign this to Russ for now, but leave it under Audio/Video component so it is under Eric's radar. 



Thanks
Hema
Assignee: itsay → rnicoletti
Flags: needinfo?(hkoka)
(In reply to Hema Koka [:hema] from comment #49)
> (In reply to Ivan Tsay (:ITsay) from comment #48)
> > Hi Hema,
> > 
> > Mind if you can take this bug and help to monitor the feature on video app
> > to use new fask seek API (). As far as I understand, once the bug 996398 is
> > landed, this one is resolved as well.
> 
> Russ has agreed to address the review comments for bug 996398 and try to
> wrap it up before FL since John is out. Once that is done, we need to ensure
> that it indeed fixes the lag (If it does not, Eric's team is probably the
> best to take this up). 
> 
> I will assign this to Russ for now, but leave it under Audio/Video component
> so it is under Eric's radar. 

Thanks and agree. Please ni? me if the lag is not resolved after fixing the dependent bug 996398.
Depends on: 1022913
From my testing with the 996398 change, there is no lag when using the slider to seek within the video.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S4 (20june)
You need to log in before you can comment on or make changes to this bug.