Seeking to start of MP4 halts playback

VERIFIED FIXED in Firefox 42

Status

()

defect
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: pehrsons, Assigned: jya)

Tracking

Trunk
mozilla44
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 wontfix, firefox42 fixed, firefox43 fixed, firefox44 verified)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

5 years ago
On Mac OS X 10.10, Yosemite.

To reproduce:
1. Open http://newton.messe.de/video_online/53208172dc14b_120314_6_telenor_connexion-03.mp4
2. After the video has played for a bit, drag the seek position marker to the very start of the video and release it anywhere.

Expected result:
Playout continues from where the marker was dropped.

Actual result:
Playout stops and never recovers.
Further seeking, pausing, playing doesn't help.
Ready state is stuck at 1 (HAVE_METADATA).
Reloading the video starts it over again.
(Also, first video frame is all green.. looks good in Chrome, QuickTime. Related?)
Component: Audio/Video → Audio/Video: Playback
Works on Windows. Broken on Mac OSX.
Assignee

Comment 2

4 years ago
The issue appears to be that seeking is slow when you just drag the slider back to the beginning as it will cause the MDSM to seek many times along the way.

Seeks aren't cancelled and it waits for the previous seek to be completed until it goes to the beginning.
So depending on how quickly or slowly you drag the slider ; you may queue dozens of seeks.

Bug 1159343 helped but obviously not enough.

JW, this is a much easier test case than B2G to reproduce the problem !
Flags: needinfo?(jwwang)
See Also: → 1159343
Assignee

Comment 3

4 years ago
Seems that the JS GUI sends dozen of seek and is itself waiting for the seek to complete (I can't explain otherwise why we would see so many seek being happening serially ; the MDSM can't queue more than one seek at a time).
Assignee

Comment 4

4 years ago
Posted patch Abort pending seeks. (obsolete) — Splinter Review
Attachment #8666582 - Flags: review?(jwwang)
Assignee

Updated

4 years ago
Flags: needinfo?(jwwang)
Assignee

Updated

4 years ago
Assignee: nobody → jyavenard
Assignee

Comment 5

4 years ago
Abort even earlier if possible
Attachment #8666586 - Flags: review?(jwwang)
Assignee

Updated

4 years ago
Attachment #8666582 - Attachment is obsolete: true
Attachment #8666582 - Flags: review?(jwwang)
Comment on attachment 8666586 [details] [diff] [review]
Abort pending seeks.

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

That is a problem of our promise mechanics. When you disconnect a promise, the job is not cancelled but the result is ignore. We end up with having hundreds of tasks waiting in the queue. This is getting worse when we seek to unbuffered ranges because it is slow to re-create HTTP channels. I wonder if there are better tools to solve this problem. Maybe a priority queue to enable out-of-order execution of tasks or cancellable tasks?

::: dom/media/MediaFormatReader.cpp
@@ +1410,5 @@
>    mPendingSeekTime = mOriginalSeekTime;
>  
>    nsRefPtr<SeekPromise> p = mSeekPromise.Ensure(__func__);
>  
> +  RefPtr<nsIRunnable> task(

nsRefPtr

@@ +1426,5 @@
> +    return;
> +  }
> +  // Reset any pending demuxer's seek.
> +  mAudio.mSeekRequest.DisconnectIfExists();
> +  mVideo.mSeekRequest.DisconnectIfExists();

It is tricky we have to do this again despite having done that in ResetDecode(). Worth a more elaborate comment.
Attachment #8666586 - Flags: review?(jwwang) → review+
Assignee

Comment 7

4 years ago
(In reply to JW Wang [:jwwang] from comment #6)
> Comment on attachment 8666586 [details] [diff] [review]
> Abort pending seeks.
> 
> Review of attachment 8666586 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> That is a problem of our promise mechanics. When you disconnect a promise,
> the job is not cancelled but the result is ignore. We end up with having
> hundreds of tasks waiting in the queue. This is getting worse when we seek
> to unbuffered ranges because it is slow to re-create HTTP channels. I wonder
> if there are better tools to solve this problem. Maybe a priority queue to
> enable out-of-order execution of tasks or cancellable tasks?

Well, we have our flushable taskqueue but this has caused issues on its own and the whole idea behind promises was to remove that task queue.
bholley may have some ideas there.

Having said that, by requeuing a task it achieves the same result.
The MDSM fired tasks such as:
MediaFormatReader::ResetDecode()
MediaFormatReader::Seek()
MediaFormatReader::ResetDecode()
MediaFormatReader::Seek()
...
MediaFormatReader::ResetDecode()
MediaFormatReader::Seek()

But re-queuing the AttemptSeek means that only one seek will be acted upon and all remaining pending tasks to AttemptSeek() will immediately abort.

Dragging the slide from the end to the 0:00 taking about 4s to slide the cursor (so doing it rather slowly) I could seek hundreds of seek being acted upon and it took 4:34s for playback to restart.

With this patch it, only 3 seeks were actually acted on and it took less than 10s for playback to resume.

> 
> ::: dom/media/MediaFormatReader.cpp
> @@ +1410,5 @@
> >    mPendingSeekTime = mOriginalSeekTime;
> >  
> >    nsRefPtr<SeekPromise> p = mSeekPromise.Ensure(__func__);
> >  
> > +  RefPtr<nsIRunnable> task(
> 
> nsRefPtr

All nsIRunnable are stored in a RefPtr , also nsRefPtr is going I believe (can't remember the bug #).

> It is tricky we have to do this again despite having done that in
> ResetDecode(). Worth a more elaborate comment.

Yeah, I will explain the rationale behind it.
Assignee

Comment 8

4 years ago
ni myself as this would need to be uplifted to beta (to fix all the related bugs duplicating this one)
Flags: needinfo?(jyavenard)
Assignee

Updated

4 years ago
No longer blocks: 1208953
https://hg.mozilla.org/mozilla-central/rev/33cdf7fca531
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Assignee

Comment 11

4 years ago
Comment on attachment 8666586 [details] [diff] [review]
Abort pending seeks.

Approval Request Comment
[Feature/regressing bug #]: 1156708
[User impact if declined]: Extremely long seek time if user drag the time slider rather than clicking into the location you want to seek to.
[Describe test coverage new/current, TreeHerder]: Manual tests, plenty of media mochitests, in central
[Risks and why]: None, we just requeue the seek which thanks to task queues property means only the last one will be acted upon.
[String/UUID change made/needed]: None
Flags: needinfo?(jyavenard)
Attachment #8666586 - Flags: approval-mozilla-beta?
Attachment #8666586 - Flags: approval-mozilla-aurora?
Assignee

Comment 12

4 years ago
Note that this issue has been there for a long time, but had slipped under the radar.
Comment on attachment 8666586 [details] [diff] [review]
Abort pending seeks.

Ok, let's take it. Should be in 42 beta 3.
Attachment #8666586 - Flags: approval-mozilla-beta?
Attachment #8666586 - Flags: approval-mozilla-beta+
Attachment #8666586 - Flags: approval-mozilla-aurora?
Attachment #8666586 - Flags: approval-mozilla-aurora+
Blocks: 1163667
QA Whiteboard: [good first verify]

Comment 16

4 years ago
This Bug's fix is verified in latest Firefox 44.0a2 Aurora

As we clicked anywhere in seekbar the video plays in any position as we clicked. No issues in the Bug.(https://bugzilla.mozilla.org/show_bug.cgi?id=1089586) 

Build Id:  	20151029045227
User Agent: Mozilla/5.0 (X11; Linux i686; rv:44.0) Gecko/20100101 Firefox/44.0

[testday-20151030]
Status: RESOLVED → VERIFIED

Comment 17

4 years ago
I simple couldn't reproduce the bug in Firefox 40 , Firefox 41, Firefox 38 in Mac OS X 10.10. Can anyone tell me in which build the bug can be reproduce, so I can verify the fix.
Flags: needinfo?(ajones)
(In reply to Ashickur Rahman from comment #17)
> I simple couldn't reproduce the bug in Firefox 40 , Firefox 41, Firefox 38
> in Mac OS X 10.10. Can anyone tell me in which build the bug can be
> reproduce, so I can verify the fix.

It should be reproducible in 38 through to 40. It will be easier to reproduce with a slow Internet connection.
Flags: needinfo?(ajones)
You need to log in before you can comment on or make changes to this bug.