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)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: bholley, Assigned: sotaro)

References

Details

Attachments

(1 file, 4 obsolete files)

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.
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)
Currently I don't have good ideas.
ni sotaro as well.
Flags: needinfo?(bwu) → needinfo?(sotaro.ikeda.g)
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)
By adding nsRefPtr<MediaDecoder::SeekPromise> Seek() method to AudioOffloadPlayer, MediaDecoder's seek seems to work properly.
Assignee: nobody → sotaro.ikeda.g
(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.
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?
(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.
Attachment #8584658 - Attachment is obsolete: true
Fix seek during pause problem.
Attachment #8584862 - Attachment is obsolete: true
Attachment #8585593 - Flags: review?(bobbyholley)
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 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+
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
https://hg.mozilla.org/mozilla-central/rev/1f5a169f0476
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
This causes bug 1150716 :-(
(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)
Thanks for the link.
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)
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.
Flags: needinfo?(ryanvm)
Target Milestone: mozilla40 → ---
unbitrot.
Attachment #8585593 - Attachment is obsolete: true
Attachment #8592947 - Flags: review+
Attachment #8592947 - Attachment is patch: true
Attachment #8592947 - Attachment mime type: text/x-patch → text/plain
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+
https://hg.mozilla.org/mozilla-central/rev/b49fbd3ad1b1
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: