Closed
Bug 1106963
Opened 10 years ago
Closed 10 years ago
Gracefully handle a switch to system clock after decoded media stream gets destroyed
Categories
(Core :: Audio/Video, defect)
Core
Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla37
People
(Reporter: pehrsons, Assigned: pehrsons)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
roc
:
review+
jwwang
:
feedback+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Comment 1•10 years ago
|
||
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.
Attachment #8531476 -
Flags: feedback?(jwwang)
Comment 2•10 years ago
|
||
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.
Assignee | ||
Updated•10 years ago
|
Summary: Don't switch back to system clock once other clock has been chosen in MediaDecoderStateMachine → Don't switch back to system clock after a MediaElement has been captured
Comment 3•10 years ago
|
||
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.
Attachment #8531476 -
Flags: feedback?(jwwang)
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8531476 -
Attachment is obsolete: true
Attachment #8532358 -
Flags: feedback?(jwwang)
Updated•10 years ago
|
Attachment #8532358 -
Flags: feedback?(jwwang) → feedback+
Assignee | ||
Updated•10 years ago
|
Attachment #8532358 -
Flags: review?(roc)
Assignee | ||
Updated•10 years ago
|
Summary: Don't switch back to system clock after a MediaElement has been captured → Gracefully handle a switch to system clock after decoded media stream gets destroyed
Attachment #8532358 -
Flags: review?(roc) → review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 5•10 years ago
|
||
Keywords: checkin-needed
Comment 6•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Assignee | ||
Comment 7•10 years ago
|
||
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
Attachment #8532358 -
Flags: approval-mozilla-beta?
Assignee | ||
Updated•10 years ago
|
status-firefox36:
--- → affected
status-firefox37:
--- → fixed
Updated•10 years ago
|
Attachment #8532358 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 8•10 years ago
|
||
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?
Attachment #8532358 -
Flags: approval-mozilla-beta+ → approval-mozilla-beta?
Assignee | ||
Comment 9•10 years ago
|
||
(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.
Updated•10 years ago
|
Attachment #8532358 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•10 years ago
|
Attachment #8532358 -
Flags: approval-mozilla-beta+ → approval-mozilla-beta?
Comment 10•10 years ago
|
||
Comment on attachment 8532358 [details] [diff] [review]
Resync media stream clock before destroying decoded stream
Sorry but same rational as in bug 879717.
Attachment #8532358 -
Flags: approval-mozilla-beta? → approval-mozilla-beta-
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Comment 11•10 years ago
|
||
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.
Attachment #8532358 -
Flags: approval-mozilla-beta- → approval-mozilla-beta?
Updated•10 years ago
|
Attachment #8532358 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•10 years ago
|
Comment 12•10 years ago
|
||
Updated•10 years ago
|
Flags: qe-verify-
You need to log in
before you can comment on or make changes to this bug.
Description
•