1139206 - Fix up AudioOffloadPlayer to interact sanely with MediaDecoder seeking

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Bobby Holley (:bholley)]

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.

Comment 1

[Bobby Holley (:bholley)]

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.

Comment 2

[Blake Wu [:bwu][:blakewu]]

Currently I don't have good ideas.
ni sotaro as well.

Comment 3

[Sotaro Ikeda [:sotaro]]

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()

Comment 4

[Sotaro Ikeda [:sotaro]]

By adding nsRefPtr<MediaDecoder::SeekPromise> Seek() method to AudioOffloadPlayer, MediaDecoder's seek seems to work properly.

Comment 5

[Sotaro Ikeda [:sotaro]]

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

Comment 6

[Sotaro Ikeda [:sotaro]]

(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 7

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Update AudioOffloadPlayer seek

Comment 8

[Bobby Holley (:bholley)]

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?

Comment 9

[Sotaro Ikeda [:sotaro]]

(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.

Comment 10

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Update AudioOffloadPlayer seek

Comment 11

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Update AudioOffloadPlayer seek

Fix seek during pause problem.

Comment 12

[Bobby Holley (:bholley)]

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.

Comment 13

[Blake Wu [:bwu][:blakewu]]

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

Comment 14

[Sotaro Ikeda [:sotaro]]

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

Comment 15

[Sotaro Ikeda [:sotaro]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1f5a169f0476

Comment 16

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/mozilla-central/rev/1f5a169f0476

Comment 17

[Sotaro Ikeda [:sotaro]]

This causes bug 1150716 :-(

Comment 18

[Sotaro Ikeda [:sotaro]]

(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?

Comment 19

[(PTO June 19-July 7) Ryan VanderMeulen]

https://wiki.mozilla.org/Sheriffing/How:To:Backouts

Comment 20

[Sotaro Ikeda [:sotaro]]

Thanks for the link.

Comment 21

[Sotaro Ikeda [:sotaro]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/df682adfd132

Comment 22

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

I backed it out of Mozilla central after testing to make sure that the backout worked:
https://hg.mozilla.org/mozilla-central/rev/70a113676b21

Comment 23

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

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.

Comment 24

[(PTO June 19-July 7) Ryan VanderMeulen]

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.

Comment 25

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Update AudioOffloadPlayer seek

unbitrot.

Comment 26

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Update AudioOffloadPlayer seek

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));
      }

Comment 27

[Sotaro Ikeda [:sotaro]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b49fbd3ad1b1

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b49fbd3ad1b1