Closed
Bug 1139206
Opened 9 years ago
Closed 9 years ago
Fix up AudioOffloadPlayer to interact sanely with MediaDecoder seeking
Categories
(Core :: Audio/Video, defect)
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: bholley, Assigned: sotaro)
References
Details
Attachments
(1 file, 4 obsolete files)
22.58 KB,
patch
|
sotaro
:
review+
|
Details | Diff | Splinter Review |
Currently, the omx AudioOffloadPlayer fires MediaDecoder::SeekingStopped runnables when the MediaDecoder never invoked Seek on the MDSM. This breaks the promise-based abstraction in my refactoring of the seeking pipeline in bug 1135170. I hacked around it for now, but we should figure out a good solution going forward. I'll write more here once bug 1135170 lands.
Reporter | ||
Comment 1•9 years ago
|
||
Any thoughts on this Blake? In a nutshell, seeking the MDSM is now promise-based, so we should find a way to rework the omx code to interact with it properly.
Flags: needinfo?(bwu)
Comment 2•9 years ago
|
||
Currently I don't have good ideas. ni sotaro as well.
Flags: needinfo?(bwu) → needinfo?(sotaro.ikeda.g)
Assignee | ||
Comment 3•9 years ago
|
||
I checked how seek works when AudioOffloadPlayer exists. The call sequence was the following. To handle actual seek, there is no actual thread context change except dispatching events. MediaOmxCommonDecoder::ChangeState(PLAY_STATE_SEEKING) ->MediaDecoder::ChangeState(PLAY_STATE_SEEKING) ->MediaOmxCommonDecoder::ApplyStateToStateMachine(PLAY_STATE_SEEKING) ->// Do nothing when AudioOffloadPlayer exists. ->AudioOffloadPlayer::ChangeState(PLAY_STATE_SEEKING) ->AudioOffloadPlayer::SeekTo(seekTimeUs, true) ->// Dispatch MediaDecoder::SeekingStarted ->// Flush Audio sink ->// Dispatch MediaDecoder::SimulateSeekResolvedForAudioOffload //run scheduler MediaDecoder::SimulateSeekResolvedForAudioOffload() ->MediaDecoder::OnSeekResolvedInternal()
Flags: needinfo?(sotaro.ikeda.g)
Assignee | ||
Comment 4•9 years ago
|
||
By adding nsRefPtr<MediaDecoder::SeekPromise> Seek() method to AudioOffloadPlayer, MediaDecoder's seek seems to work properly.
Assignee | ||
Comment 5•9 years ago
|
||
FYI: I create a diagram related to AudioOffloadPlayer. https://github.com/sotaroikeda/firefox-diagrams/raw/master/media/dom_media_omx_AudioOffloadPlayer_FirefoxOS_2_2.pdf
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → sotaro.ikeda.g
Assignee | ||
Comment 6•9 years ago
|
||
(In reply to Sotaro Ikeda [:sotaro] from comment #3) > I checked how seek works when AudioOffloadPlayer exists. > > The call sequence was the following. To handle actual seek, there is no > actual thread context change except dispatching events. > It was incorrect, during playback, the event is dispatched from AudioTrack callback thread.
Assignee | ||
Comment 7•9 years ago
|
||
Reporter | ||
Comment 8•9 years ago
|
||
Comment on attachment 8584658 [details] [diff] [review] patch - Update AudioOffloadPlayer seek Review of attachment 8584658 [details] [diff] [review]: ----------------------------------------------------------------- Awesome, thanks for jumping on this Sotaro! ::: dom/media/MediaDecoder.h @@ +809,5 @@ > > void OnSeekResolved(SeekResolveValue aVal) > { > mSeekRequest.Complete(); > OnSeekResolvedInternal(aVal.mAtEnd, aVal.mEventVisibility); Can you merge OnSeekResolved and OnSeekResolvedInternal now that this is fixed?
Assignee | ||
Comment 9•9 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #8) > > Can you merge OnSeekResolved and OnSeekResolvedInternal now that this is > fixed? Yes, I am going to update the patch. Then I am going to check if it works correctly on flame device.
Assignee | ||
Comment 10•9 years ago
|
||
Attachment #8584658 -
Attachment is obsolete: true
Assignee | ||
Comment 11•9 years ago
|
||
Fix seek during pause problem.
Attachment #8584862 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8585593 -
Flags: review?(bobbyholley)
Reporter | ||
Comment 12•9 years ago
|
||
Comment on attachment 8585593 [details] [diff] [review] patch - Update AudioOffloadPlayer seek Review of attachment 8585593 [details] [diff] [review]: ----------------------------------------------------------------- MediaDecoder stuff looks great, but I don't know enough about the omx/ stuff to say too much there. Flagging Blake to review the bulk of the patch.
Attachment #8585593 -
Flags: review?(bwu)
Attachment #8585593 -
Flags: review?(bobbyholley)
Attachment #8585593 -
Flags: review+
Comment 13•9 years ago
|
||
Comment on attachment 8585593 [details] [diff] [review] patch - Update AudioOffloadPlayer seek Review of attachment 8585593 [details] [diff] [review]: ----------------------------------------------------------------- I don't know omx stuff better than sotaro :) This patch looks good to me! Just some nits. ::: dom/media/omx/AudioOffloadPlayer.cpp @@ +465,5 @@ > + // We do not reset mSeekTarget here. > + if (!mSeekPromise.IsEmpty()) { > + MediaDecoder::SeekResolveValue val(mReachedEOS, mSeekTarget.mEventVisibility); > + mSeekPromise.Resolve(val, __func__); > + } This seems to do the same thing in NotifyAudioEOS(). Would it be better to put it to be in a function which is called here and NotifyAudioEOS()? @@ +575,5 @@ > kKeyTime, &mPositionTimeMediaUs)); > } > > + if (mSeekTarget.IsValid() && seekTimeUs == mSeekTarget.mTime) { > + MOZ_ASSERT(mSeekTarget.IsValid()); This assertion check is not helpful here. ::: dom/media/omx/MediaOmxCommonDecoder.cpp @@ +208,5 @@ > + case PLAY_STATE_SEEKING: > + mSeekRequest.Begin(mAudioOffloadPlayer->Seek(mRequestedSeekTarget) > + ->RefableThen(NS_GetCurrentThread(), __func__, static_cast<MediaDecoder*>(this), > + &MediaDecoder::OnSeekResolved, &MediaDecoder::OnSeekRejected)); > + mRequestedSeekTarget.Reset(); could call ResetSeekTime() instead
Attachment #8585593 -
Flags: review?(bwu) → review+
Assignee | ||
Comment 14•9 years ago
|
||
Comment on attachment 8585593 [details] [diff] [review] patch - Update AudioOffloadPlayer seek Review of attachment 8585593 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/omx/AudioOffloadPlayer.cpp @@ +465,5 @@ > + // We do not reset mSeekTarget here. > + if (!mSeekPromise.IsEmpty()) { > + MediaDecoder::SeekResolveValue val(mReachedEOS, mSeekTarget.mEventVisibility); > + mSeekPromise.Resolve(val, __func__); > + } NotifyAudioTearDown() is different from NotifyAudioEOS(). It needs to be different. The following is snip from android. enum cb_event_t { CB_EVENT_FILL_BUFFER, // Request to write more data to buffer. CB_EVENT_STREAM_END, // Sent after all the buffers queued in AF and HW are played // back (after stop is called) CB_EVENT_TEAR_DOWN // The AudioTrack was invalidated due to use case change: // Need to re-evaluate offloading options }; http://androidxref.com/5.1.0_r1/xref/frameworks/av/include/media/MediaPlayerInterface.h#78 @@ +575,5 @@ > kKeyTime, &mPositionTimeMediaUs)); > } > > + if (mSeekTarget.IsValid() && seekTimeUs == mSeekTarget.mTime) { > + MOZ_ASSERT(mSeekTarget.IsValid()); Yup, the check is not necessary. ::: dom/media/omx/MediaOmxCommonDecoder.cpp @@ +208,5 @@ > + case PLAY_STATE_SEEKING: > + mSeekRequest.Begin(mAudioOffloadPlayer->Seek(mRequestedSeekTarget) > + ->RefableThen(NS_GetCurrentThread(), __func__, static_cast<MediaDecoder*>(this), > + &MediaDecoder::OnSeekResolved, &MediaDecoder::OnSeekRejected)); > + mRequestedSeekTarget.Reset(); The code intentionally mimic the code of MediaDecoder::ApplyStateToStateMachine(). Then I am going to keep mRequestedSeekTarget.Reset(). https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaDecoder.cpp#1319
Assignee | ||
Comment 15•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f5a169f0476
Comment 16•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1f5a169f0476
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Assignee | ||
Comment 17•9 years ago
|
||
This causes bug 1150716 :-(
Assignee | ||
Comment 18•9 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #16) > https://hg.mozilla.org/mozilla-central/rev/1f5a169f0476 Ryan, can this be backed out?
Flags: needinfo?(ryanvm)
Comment 19•9 years ago
|
||
https://wiki.mozilla.org/Sheriffing/How:To:Backouts
Flags: needinfo?(ryanvm)
Assignee | ||
Comment 20•9 years ago
|
||
Thanks for the link.
Assignee | ||
Comment 21•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/df682adfd132
Assignee | ||
Updated•9 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I backed it out of Mozilla central after testing to make sure that the backout worked: https://hg.mozilla.org/mozilla-central/rev/70a113676b21
This makes me wonder, do we need only to backout of inbound and then wait for the merge? or do we need to worry about backout for both? I guess I should have asked that question before backing out.
Flags: needinfo?(ryanvm)
Comment 24•9 years ago
|
||
Normally we would have just let it merge to m-c from inbound or backed out directly from m-c in the first place. Kind of moot now, though. No real harm in what you did besides polluting the hg history.
Assignee | ||
Comment 25•9 years ago
|
||
unbitrot.
Attachment #8585593 -
Attachment is obsolete: true
Attachment #8592947 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Attachment #8592947 -
Attachment is patch: true
Attachment #8592947 -
Attachment mime type: text/x-patch → text/plain
Assignee | ||
Comment 26•9 years ago
|
||
Fix regression. I misunderstood the following part was still called only during seek :-( ------------------------------ NotifyPositionChanged(); // need to adjust the mStartPosUs for offload decoding since parser // might not be able to get the exact seek time requested. mStartPosUs = mPositionTimeMediaUs; AUDIO_OFFLOAD_LOG(PR_LOG_DEBUG, ("Adjust seek time to: %.2f", mStartPosUs / 1E6)); }
Attachment #8592947 -
Attachment is obsolete: true
Attachment #8593026 -
Flags: review+
Assignee | ||
Comment 27•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b49fbd3ad1b1
Comment 28•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b49fbd3ad1b1
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in
before you can comment on or make changes to this bug.
Description
•