Closed Bug 1135170 Opened 5 years ago Closed 5 years ago

Simplify seeking pipeline

Categories

(Core :: Audio/Video, defect, P2)

x86
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(14 files, 2 obsolete files)

4.23 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
4.70 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
2.51 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
8.60 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
7.04 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
1.09 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
649 bytes, patch
karlt
: review+
Details | Diff | Splinter Review
23.71 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
4.62 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
3.52 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
39.30 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
4.30 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
3.89 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
4.94 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
The current setup is exceedingly complex, with lots of interdependencies between stages. I have some ideas on how to make the whole process a lot less complicated, which should hopefully make it at lot less racey.
Depends on: 1135424
No longer depends on: 1135424
Blocks: 1130237
Blocks: 1130239
Depends on: 1135785
(In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect things) from comment #3)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=67dbed53e83d

This was missing a patch I wanted to include. Canceled.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5dbf76881944
Blocks: 1136873
I traced this back to something 2011 or earlier and then gave up. Given that we're
doing an exact microsecond comparison here this is almost certainly dead code in
every case except for the one where the media is paused and JS does
|el.currentTime = el.currentTime|. And in that case, I think running through the
regular seek machinery is probably fine.
Attachment #8569452 - Flags: review?(cpearce)
I can't see any reason why this should be necessary, and cursory archaeology
suggests that this too is a hand-me-down from previous threading models.
Attachment #8569454 - Flags: review?(cpearce)
The model we're moving towards is one where the MDSM can just disconnect all of
its promises, send a ResetDecode down the pipe, and start doing something
unrelated.
Attachment #8569455 - Flags: review?(cpearce)
The ogg reader makes two adjustments to the seek time - the first is to clamp it
between start and end time, which MDSM already does. The second is to subtract
SEEK_OPUS_PREROLL from the target. If we wanted to, we could return this as the
resolve value in the seek promise and handle the update in the MDSM. But I think
DropVideoUpToSeekTarget should actually handle this just fine.
Attachment #8569456 - Flags: review?(cpearce)
Attachment #8569458 - Flags: review?(cpearce)
Comment on attachment 8569458 [details] [diff] [review]
Part 7 - Mark previously-failing WPT as passing. v1

Convention is to remove the subtest blocks from .ini files when the subtest passes and remove the whole .ini file when the whole test is passing, as it will be here.

Automated updates from upstream rewrite the .ini files, so if convention is followed now, there is less noise later.
All consumers of MediaCacheStream::GetCachedRanges do this except this one.
Attachment #8571450 - Flags: review?(cpearce)
Priority: -- → P2
I'll pass the review here to Matt Woodrow, it's been waiting in my queue for too long.
Blocks: 1139206
Attachment #8569451 - Flags: review?(cpearce) → review?(matt.woodrow)
Attachment #8569452 - Flags: review?(cpearce) → review?(matt.woodrow)
Attachment #8569454 - Flags: review?(cpearce) → review?(matt.woodrow)
Attachment #8569455 - Flags: review?(cpearce) → review?(matt.woodrow)
Attachment #8569456 - Flags: review?(cpearce) → review?(matt.woodrow)
Attachment #8569457 - Flags: review?(cpearce) → review?(matt.woodrow)
Attachment #8571450 - Flags: review?(cpearce) → review?(matt.woodrow)
Attachment #8569458 - Attachment is obsolete: true
Attachment #8569458 - Flags: review?(cpearce)
Attachment #8572259 - Flags: review?(karlt)
Attachment #8571450 - Flags: review?(matt.woodrow) → review+
Attachment #8569451 - Flags: review?(matt.woodrow) → review+
Attachment #8569452 - Flags: review?(matt.woodrow) → review+
Attachment #8569454 - Flags: review?(matt.woodrow) → review+
I rebased these patches on top of sotaro's in bug 1128357, which are going to
land first.
Attachment #8569455 - Attachment is obsolete: true
Attachment #8569455 - Flags: review?(matt.woodrow)
Attachment #8572293 - Flags: review?(matt.woodrow)
Comment on attachment 8572293 [details] [diff] [review]
Part 4 - Streamline seek initiation logic and abolish manual seek cancels and retries. v1

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

::: dom/media/MediaDecoderReader.h
@@ -171,5 @@
> -  // promise might have already dispatched a resolution or an error code before
> -  // the cancel arrived.
> -  //
> -  // Must be called on the decode task queue.
> -  virtual void CancelSeek() { };

We should update the documentation for ResetDecode to say that it cancels any outstanding seek promises.
Attachment #8572293 - Flags: review?(matt.woodrow) → review+
Attachment #8569456 - Flags: review?(matt.woodrow) → review+
Attachment #8569457 - Flags: review?(matt.woodrow) → review+
I still don't love the naming of mSeekTarget/mCurrentSeekTarget/mQueuedSeekTarget.

Maybe mPendingSeekTarget and mSeekRequestTarget (to match mSeekRequest)?
(In reply to Matt Woodrow (:mattwoodrow) from comment #18)
> I still don't love the naming of
> mSeekTarget/mCurrentSeekTarget/mQueuedSeekTarget.
> 
> Maybe mPendingSeekTarget and mSeekRequestTarget (to match mSeekRequest)?

We think alike! That change comes in the one of the patches that I haven't uploaded yet. :-)
Attachment #8572259 - Flags: review?(karlt) → review+
This has two implications:
* We no longer need to pipe mQueuedSeekTarget through MDSM::Seek to get the
  appropriate clamping.
* MDSM::Seek doesn't _need_ to be called on the main thread anymore.
Attachment #8572801 - Flags: review?(matt.woodrow)
This means that we can get rid of the code to recheck state after dropping the
monitor. We'll remove the other monitor drop from this method in a subsequent
patch.
Attachment #8572802 - Flags: review?(matt.woodrow)
Note that this patch needed heavy rebasing over sotaro's patch in bug 1128357.
Attachment #8572803 - Flags: review?(matt.woodrow)
Blocks: 1138528
Blocks: 1139121
Attachment #8572801 - Flags: review?(matt.woodrow) → review+
Attachment #8572802 - Flags: review?(matt.woodrow) → review+
Attachment #8572804 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8572803 [details] [diff] [review]
Part 10 - Rewrite the MediaDecoder-to-MediaDecoderStateMachine interface to be Promise-based. v1

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

Awesome, huge improvement!

::: dom/media/MediaDecoderStateMachine.cpp
@@ +1472,5 @@
>        } else {
> +        mQueuedSeek.mTarget = SeekTarget(mCurrentFrameTime,
> +                                         SeekTarget::Accurate,
> +                                         MediaDecoderEventVisibility::Suppressed);
> +        // XXXbholley - Nobody is listening to this promise. Do we need to pas it

pass
Attachment #8572803 - Flags: review?(matt.woodrow) → review+
The use of play() and pause() in the test is hugely problematic for short video
files and slow/laggy platforms. In particular, if playback has ended by
the time that we fire the 'seeked' event listener, then the ensuing play() will
put us back into seeking mode (seeking to 0), making the test fail.
Attachment #8573463 - Flags: review?(matt.woodrow)
This test is fundamentally racey - it loads very short video files (some less
than 1s), plays them, waits for timeupdate events to try to find just the right
moment to seek, performs a seek, and then checks various pieces of
playback-dependent state (while playing).

The specific issue I ran into was that the video would sometimes finish playing
before the 'seeked' event handler fired, which means that readyState is
HAVE_CURRENT_DATA (per spec). I could fiddle with the test a bit to handle this
case, but I think we're doing a disservice to ourselves by having it in the tree.
Attachment #8573464 - Flags: review?(matt.woodrow)
Depends on: 1093980
(In reply to Bobby Holley (:bholley) from comment #26)
I think the test still makes sense where it check if we get HAVE_FUTURE_DATA when seeking within buffered data though it doesn't really impact the user experience. I would prefer to disable it for now and fix it when are have time.
(In reply to JW Wang [:jwwang] from comment #27)
> (In reply to Bobby Holley (:bholley) from comment #26)
> I think the test still makes sense where it check if we get HAVE_FUTURE_DATA
> when seeking within buffered data though it doesn't really impact the user
> experience. I would prefer to disable it for now and fix it when are have
> time.

The problem is that doing this test while play()ing is inherently racey, and we don't (as far as I know) have a reliable way to force a non-MSE media element to preload a large buffered range without playing.
This is very green modulo win7 opt mochitest-3 orange: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c9f890db27d

I disabled the orange tests in bug 1140675, given that they were failing like crazy without my patches too.

There are two tests fixes yet-to-be-reviewed in this bug, but given that they're test-only and this is a huge change, I'm landing them rpending=mattwoodrow. I'm happy to make any necessary changes retroactively, and will hold myself to it.

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=cb5ec48a3391
(Note that those are periodic builds, not per-push builds.  And they appear to be using gcc 4.8; not sure how many other builds use 4.8.)
The bustage is reproducable locally by disabling EME; I guess those device builds disable EME.
Attachment #8573464 - Flags: review?(matt.woodrow) → review+
Attachment #8573463 - Flags: review?(matt.woodrow) → review+
I uplifted the seek-to-currentTime.html XFAIL removal to 38. Jean-Yves seeking changes seem to have fixed that one.

https://hg.mozilla.org/releases/mozilla-aurora/rev/586471431570
Duplicate of this bug: 1132851
You need to log in before you can comment on or make changes to this bug.