1106963 - Gracefully handle a switch to system clock after decoded media stream gets destroyed

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Andreas Pehrson [:pehrsons]]

See bug 1106698.

There's a risk that we use a clock other than the system clock - like the mediastream equivalent - just to switch back later - for instance during shutdown. Time then appears to go backwards if the system clock is behind the other clock we used.

Comment 1

[Andreas Pehrson [:pehrsons]]

Attachment: Do not revert to using system clock after having used decoded stream time

This works fine for the decoded stream time.

I don't think we can do the same thing for the audio clock though since audio can end and we should then be using the system clock instead.

Comment 2

[JW Wang [:jwwang] [:jw_wang]]

The title is not correct. For the file case, it is possible to play out all audio frames while there are still video frames remaining. We will switch back to the system clock since all audio frames are played out. However, once a media element is captured, there is no returning back and we shouldn't switch clock.

Comment 3

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8531476 [details] [diff] [review]
Do not revert to using system clock after having used decoded stream time

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

::: dom/media/MediaDecoderStateMachine.cpp
@@ +2818,5 @@
> +  // If we have recently begun streaming our output, get a hold on the
> +  // decoded stream listener so we don't risk having to switch back to
> +  // the system clock during shutdown. See bug 1106698.
> +  if (mDecoder->GetDecodedStream() && !mDecodedStreamClockListener) {
> +    mDecodedStreamClockListener = mDecoder->GetDecodedStream()->mListener;

It worries me that the stream listener could be become staled when MediaDecoder::RecreateDecodedStream is called. I think we should do something like MediaDecoderStateMachine::ResyncAudioClock to re-sync the stream clock before the stream is destroyed.

Comment 4

[Andreas Pehrson [:pehrsons]]

Attachment: Resync media stream clock before destroying decoded stream

How about this?

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d7362bcad0fe

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/161ccc4bb2bb

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/161ccc4bb2bb

Comment 7

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8532358 [details] [diff] [review]
Resync media stream clock before destroying decoded stream

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: Blocking uplift of bug 879717, see bug 879717 comment 200 for its approval request comment.
[Describe test coverage new/current, TBPL]: Landed on m-c in 37. Covered by all media tests capturing an element with mozCaptureStream().
[Risks and why]: low. Only affects user-facing media time of media elements captured by mozCaptureStream().
[String/UUID change made/needed]: None

Comment 8

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8532358 [details] [diff] [review]
Resync media stream clock before destroying decoded stream

> Only affects user-facing media time of media elements captured by
> mozCaptureStream().
Actually, could you explain what is the impact for Firefox itself?

Comment 9

[Andreas Pehrson [:pehrsons]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #8)
> Comment on attachment 8532358 [details] [diff] [review]
> Resync media stream clock before destroying decoded stream
> 
> > Only affects user-facing media time of media elements captured by
> > mozCaptureStream().
> Actually, could you explain what is the impact for Firefox itself?

First of all, regarding this bug on its own: On pages with a media element that is captured with mozCaptureStream() (should be few), the clock of the media element might appear to be wrong after seeking.

Second, and the big thing if you ask me, is bug 879717 which is blocked by us here. In short, without 879717 we don't follow the spec on media elements with streams (typically getUserMedia, WebRTC), but you can find more info on bug 879717 itself.

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8532358 [details] [diff] [review]
Resync media stream clock before destroying decoded stream

Sorry but same rational as in bug 879717.

Comment 11

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8532358 [details] [diff] [review]
Resync media stream clock before destroying decoded stream

Sorry, I have to request beta approval again. We're seeing failures in automation in beta (bug 1124404) after bug 1113600 got uplifted, but this bug is also needed.

Without this bug the clock for a captured media element can go backwards, though mostly during shutdown. Bug 1113600 changed the behavior by checking that the clock also in the captured cases doesn't go backwards. We could either uplift this bug or revert the check change done in 1113600.

I propose we uplift this bug for a proper fix. Also see comment 7 for the previous request.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/cdffc642c9b9