1266027 - Make the MediaDecoderReaderWrapper as a proxy of requesting media data form MediaDecoderReader and be able to change callbacks at runtime.

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect)

Description

[Tzuhao Kuo [:kaku]]

Please refer to the Bug 1235301 comment 17, we need a way to dynamically change the callbacks of requesting media data from MediaDecoderReader.

Comment 1

[Tzuhao Kuo [:kaku]]

Attachment: MozReview Request: Bug 1266027 part 1 - make the MediaDecoderReaderWrapper as a proxy of requesting media data; r=jwwang

Review commit: https://reviewboard.mozilla.org/r/47989/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47989/

Comment 2

[Tzuhao Kuo [:kaku]]

Attachment: MozReview Request: Bug 1266027 part 2 - make MDSM and SeekTask to adopt new MediaDecoderReaderWrapper API; r=jwwang

Review commit: https://reviewboard.mozilla.org/r/47991/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47991/

Comment 3

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

Comment on attachment 8743658 [details]
MozReview Request: Bug 1266027 part 1 - make the MediaDecoderReaderWrapper as a proxy of requesting media data; r=jwwang

https://reviewboard.mozilla.org/r/47989/#review44755

::: dom/media/MediaDecoderReaderWrapper.h:37
(Diff revision 1)
>    typedef MediaDecoderReader::SeekPromise SeekPromise;
>    typedef MediaDecoderReader::WaitForDataPromise WaitForDataPromise;
>    typedef MediaDecoderReader::BufferedUpdatePromise BufferedUpdatePromise;
>    NS_INLINE_DECL_THREADSAFE_REFCOUNTING(MediaDecoderReaderWrapper);
>  
> +  struct CallbacksBase

Use singular form, i.e. CallbackBase.

::: dom/media/MediaDecoderReaderWrapper.h:40
(Diff revision 1)
>    NS_INLINE_DECL_THREADSAFE_REFCOUNTING(MediaDecoderReaderWrapper);
>  
> +  struct CallbacksBase
> +  {
> +    virtual ~CallbacksBase() {}
> +    virtual void InvokeSuccessCallback(MediaData*) = 0;

I would prefer the names OnResolved() and OnRejected().

::: dom/media/MediaDecoderReaderWrapper.h:102
(Diff revision 1)
> -  RefPtr<AudioDataPromise> RequestAudioData();
> -  RefPtr<VideoDataPromise> RequestVideoData(bool aSkipToNextKeyframe,
> -                                            media::TimeUnit aTimeThreshold);
> +
> +  template<typename ThisType, typename ResolveMethodType, typename RejectMethodType>
> +  void SetRequestAudioDataCallbacks(ThisType* aThisVal,
> +                                    ResolveMethodType aResolveMethod,
> +                                    RejectMethodType aRejectMethod)
> +  {

Assert these functions are called on the right thread.

::: dom/media/MediaDecoderReaderWrapper.h:103
(Diff revision 1)
> -  RefPtr<VideoDataPromise> RequestVideoData(bool aSkipToNextKeyframe,
> -                                            media::TimeUnit aTimeThreshold);
> +  template<typename ThisType, typename ResolveMethodType, typename RejectMethodType>
> +  void SetRequestAudioDataCallbacks(ThisType* aThisVal,
> +                                    ResolveMethodType aResolveMethod,
> +                                    RejectMethodType aRejectMethod)
> +  {
> +    mRequestAudioDataCBs.reset(

Ask the client to explicitly cancel the current callback before setting a new one. This will prevent the client code from accidentally overwriting the existing callback.

::: dom/media/MediaDecoderReaderWrapper.h:213
(Diff revision 1)
>    bool mShutdown = false;
>    RefPtr<StartTimeRendezvous> mStartTimeRendezvous;
> +
> +  UniquePtr<CallbacksBase> mRequestAudioDataCBs;
> +  UniquePtr<CallbacksBase> mRequestVideoDataCBs;
> +  MozPromiseRequestHolder<MediaDecoderReader::AudioDataPromise> mAudioDataRequest;

AudioDataPromise is already defined above. You can simply say MozPromiseRequestHolder<AudioDataPromise>.

::: dom/media/MediaDecoderReaderWrapper.cpp:199
(Diff revision 1)
>  
>    auto p = InvokeAsync(mReader->OwnerThread(), mReader.get(), __func__,
>                         &MediaDecoderReader::RequestAudioData);
>  
>    if (!mStartTimeRendezvous->HaveStartTime()) {
> -    p = p->Then(mOwnerThread, __func__, mStartTimeRendezvous.get(),
> +    p = p->Then(mOwnerThread.get(), __func__, mStartTimeRendezvous.get(),

Why |.get()| is needed?

::: dom/media/MediaDecoderReaderWrapper.cpp:254
(Diff revision 1)
> +  MOZ_ASSERT(mOwnerThread->IsCurrentThreadIn());
> +  return mVideoDataRequest.Exists();
> +}
> +
> +void
> +MediaDecoderReaderWrapper::DiscardAudioDataRequestIfExists()

Please name it "DisconnectAudioDataRequestIfExists" for consistency.

::: dom/media/MediaDecoderReaderWrapper.cpp:268
(Diff revision 1)
> +  MOZ_ASSERT(mOwnerThread->IsCurrentThreadIn());
> +  mVideoDataRequest.DisconnectIfExists();
> +}
> +
> +void
> +MediaDecoderReaderWrapper::OnAudioDecoded(MediaData* aAudioSample)

The code is much similar to OnVideoDecoded(). You should be able to merge them into one function.

::: dom/media/MediaDecoderReaderWrapper.cpp:275
(Diff revision 1)
> +  MOZ_ASSERT(mOwnerThread->IsCurrentThreadIn());
> +
> +  mAudioDataRequest.Complete();
> +  OnSampleDecoded(aAudioSample);
> +
> +  if (mRequestAudioDataCBs) {

Assert |mRequestAudioDataCBs| is non-null. The client should guarantee when OnAudioDecoded() is called, the callback should always be present here.

::: dom/media/MediaDecoderReaderWrapper.cpp:287
(Diff revision 1)
> +    // {
> +    //   mRequestAudioDataCBs->InvokeSuccessCallback(aAudioSample);
> +    //   mRequestAudioDataCBs = nullptr;
> +    // }
> +
> +    UniquePtr<CallbacksBase> callbacks(Move(mRequestAudioDataCBs));

Isn't |Move(mRequestAudioDataCBs)| effectively resetting/nullifying |mRequestAudioDataCBs|?

::: dom/media/MediaDecoderReaderWrapper.cpp:293
(Diff revision 1)
> +    callbacks->InvokeSuccessCallback(aAudioSample);
> +  }
> +}
> +
> +void
> +MediaDecoderReaderWrapper::OnAudioNotDecoded(MediaDecoderReader::NotDecodedReason aReason)

Ditto. Merge OnXXXNotDecoded()s into one function.

Comment 4

[Tzuhao Kuo [:kaku]]

https://reviewboard.mozilla.org/r/47989/#review44755

> Why |.get()| is needed?

Hmm..., to prevent my editor(Eclipse) from reporting an error......
I can remove it.

> Isn't |Move(mRequestAudioDataCBs)| effectively resetting/nullifying |mRequestAudioDataCBs|?

Yes, it is. But the point is that we cannot nullify the might-be-new-callback *AFTER* invoking the original-callbacks.

Comment 5

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

Comment on attachment 8743659 [details]
MozReview Request: Bug 1266027 part 2 - make MDSM and SeekTask to adopt new MediaDecoderReaderWrapper API; r=jwwang

https://reviewboard.mozilla.org/r/47991/#review44757

::: dom/media/MediaDecoderStateMachine.cpp:1526
(Diff revision 1)
>  
>    if (aValue.mNeedToStopPrerollingVideo) {
>      StopPrerollingVideo();
>    }
>  
> -  SeekCompleted();
> +  // NOTE: Discarding the mSeekTask must be done before calling SeekCompleted().

You can move |mSeekTask->Discard()| into SeekCompleted() to solve the problem you stated.

::: dom/media/MediaDecoderStateMachine.cpp:1616
(Diff revision 1)
>    MOZ_ASSERT(mState != DECODER_STATE_SEEKING);
>  
>    SAMPLE_LOG("Queueing audio task - queued=%i, decoder-queued=%o",
>               AudioQueue().GetSize(), mReader->SizeOfAudioQueueInFrames());
>  
> -  mAudioDataRequest.Begin(
> +  mReader->SetRequestAudioDataCallbacks(this,

I would like the callback to persist until you cancel it without needing to set it each time before calling RequestXXXData().

::: dom/media/MediaDecoderStateMachine.cpp:2262
(Diff revision 1)
>    mVideoCompleted = false;
>    AudioQueue().Reset();
>    VideoQueue().Reset();
>  
>    mMetadataRequest.DisconnectIfExists();
> -  mAudioDataRequest.DisconnectIfExists();
> +  mReader->DiscardAudioDataRequestIfExists();

DiscardXXXDataRequestIfExists() should be internal to MediaDecoderReaderWrapper. You can do them in ResetDecode() internally without exposing them publicly.

Comment 6

[Tzuhao Kuo [:kaku]]

https://reviewboard.mozilla.org/r/47989/#review44755

> Hmm..., to prevent my editor(Eclipse) from reporting an error......
> I can remove it.

Can I keep it...?

Comment 7

[Tzuhao Kuo [:kaku]]

Comment on attachment 8743658 [details]
MozReview Request: Bug 1266027 part 1 - make the MediaDecoderReaderWrapper as a proxy of requesting media data; r=jwwang

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47989/diff/1-2/

Comment 8

[Tzuhao Kuo [:kaku]]

Comment on attachment 8743659 [details]
MozReview Request: Bug 1266027 part 2 - make MDSM and SeekTask to adopt new MediaDecoderReaderWrapper API; r=jwwang

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47991/diff/1-2/

Comment 9

[Tzuhao Kuo [:kaku]]

https://reviewboard.mozilla.org/r/47989/#review44811

::: dom/media/MediaDecoderReaderWrapper.h:62
(Diff revision 2)
> +    void OnRejected(MediaDecoderReader::NotDecodedReason aReason) override
> +    {
> +      ((*mThis).*mRejectMethod)(aReason);
> +    }
> +
> +    RefPtr<ThisType> mThis;

Here, the MediaDecoderReaderWrapper might keep a reference to the callback methods' instance(MDSM or SeekTask) which leads to a *cycle refercence* and this is why the mochitest won't close.

It could be resolved by calling |mReader->CancelRequestAudioDataCallback();| and |mReader->CancelRequestVideoDataCallback();| at MediaDecoderStateMachine::Shutdown(). 

Note that we cannot call |mReader->CancelRequestAudioDataCallback();| and |mReader->CancelRequestVideoDataCallback();| at MediaDecoderStateMachine::Reset() because this might mis-cancel the SeekTask's callbacks. We need to contoral the owrnership of MediaDecoderReaderWrapper between MDSM and SeekTask in a better way......

Comment 10

[Tzuhao Kuo [:kaku]]

Comment on attachment 8743658 [details]
MozReview Request: Bug 1266027 part 1 - make the MediaDecoderReaderWrapper as a proxy of requesting media data; r=jwwang

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47989/diff/2-3/

Comment 11

[Tzuhao Kuo [:kaku]]

Comment on attachment 8743659 [details]
MozReview Request: Bug 1266027 part 2 - make MDSM and SeekTask to adopt new MediaDecoderReaderWrapper API; r=jwwang

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47991/diff/2-3/

Comment 12

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

Comment on attachment 8743658 [details]
MozReview Request: Bug 1266027 part 1 - make the MediaDecoderReaderWrapper as a proxy of requesting media data; r=jwwang

https://reviewboard.mozilla.org/r/47989/#review45143

::: dom/media/MediaDecoderReaderWrapper.h:40
(Diff revision 3)
>    NS_INLINE_DECL_THREADSAFE_REFCOUNTING(MediaDecoderReaderWrapper);
>  
> +  struct CallbackBase
> +  {
> +    virtual ~CallbackBase() {}
> +    virtual void OnResolved(MediaData*, TimeStamp) = 0;

Separate AudioCallback from VideoCallbak since TimeStamp is not used by AudioCallback at all.

Or you can use template tricks to accept both |void(MediaData*)| and |void(MediaData*, TimeStamp)|.

::: dom/media/MediaDecoderReaderWrapper.h:239
(Diff revision 3)
>    const RefPtr<MediaDecoderReader> mReader;
>  
>    bool mShutdown = false;
>    RefPtr<StartTimeRendezvous> mStartTimeRendezvous;
> +
> +  UniquePtr<CallbackBase> mRequestAudioDataCBs;

Use singular form, "mRequestAudioDataCB".

::: dom/media/MediaDecoderReaderWrapper.cpp:191
(Diff revision 3)
>    MOZ_ASSERT(!mShutdown);
>    return mStartTimeRendezvous->AwaitStartTime();
>  }
>  
> -RefPtr<MediaDecoderReaderWrapper::AudioDataPromise>
> +void
> +MediaDecoderReaderWrapper::CancelAudioCallback(CallbackRegistratonID aID)

Assert calling thread.

::: dom/media/MediaDecoderReaderWrapper.cpp:227
(Diff revision 3)
>  
> -  return p->Then(mOwnerThread, __func__, this,
> -                 &MediaDecoderReaderWrapper::OnSampleDecoded,
> -                 &MediaDecoderReaderWrapper::OnNotDecoded)
> -          ->CompletionPromise();
> +  RefPtr<MediaDecoderReaderWrapper> self = this;
> +  mAudioDataRequest.Begin(p->Then(mOwnerThread, __func__,
> +    [self] (MediaData* aAudioSample) {
> +      self->mAudioDataRequest.Complete();
> +      self->OnSampleDecoded(aAudioSample, TimeStamp::Now());

It would make more sense to pass |TimeStamp()| to mean "no decode start time".

::: dom/media/MediaDecoderReaderWrapper.cpp:243
(Diff revision 3)
>  {
>    MOZ_ASSERT(mOwnerThread->IsCurrentThreadIn());
>    MOZ_ASSERT(!mShutdown);
> +  MOZ_ASSERT(mRequestVideoDataCBs, "Request video data without callbacks!");
> +
> +  // Time the video decode, so that if it's slow, we can increase our low

This comment belongs to MDSM.

::: dom/media/MediaDecoderReaderWrapper.cpp:269
(Diff revision 3)
>  
> -  return p->Then(mOwnerThread, __func__, this,
> -                 &MediaDecoderReaderWrapper::OnSampleDecoded,
> -                 &MediaDecoderReaderWrapper::OnNotDecoded)
> -          ->CompletionPromise();
> +  RefPtr<MediaDecoderReaderWrapper> self = this;
> +  mVideoDataRequest.Begin(p->Then(mOwnerThread, __func__,
> +    [self, videoDecodeStartTime] (MediaData* aVideoSample) {
> +      self->mVideoDataRequest.Complete();
> +      self->OnSampleDecoded(aVideoSample, videoDecodeStartTime);

Pass mRequestVideoDataCBs to OnSampleDecoded() so you don't have to check aSample->mType.

::: dom/media/MediaDecoderReaderWrapper.cpp:273
(Diff revision 3)
> -          ->CompletionPromise();
> +      self->mVideoDataRequest.Complete();
> +      self->OnSampleDecoded(aVideoSample, videoDecodeStartTime);
> +    },
> +    [self] (MediaDecoderReader::NotDecodedReason aReason) {
> +      self->mVideoDataRequest.Complete();
> +      self->OnNotDecoded(MediaData::VIDEO_DATA, aReason);

Ditto.

::: dom/media/MediaDecoderReaderWrapper.cpp:394
(Diff revision 3)
> -MediaDecoderReaderWrapper::OnSampleDecoded(MediaData* aSample)
> +MediaDecoderReaderWrapper::OnSampleDecoded(MediaData* aSample,
> +                                           TimeStamp aDecodeStartTime)
>  {
>    MOZ_ASSERT(mOwnerThread->IsCurrentThreadIn());
> +
>    if (!mShutdown) {

We should be able to assert mShutdown is false here since we disconnect promised in ResetDecode() which always happens before Shutdown().

Comment 13

[Tzuhao Kuo [:kaku]]

Comment on attachment 8743658 [details]
MozReview Request: Bug 1266027 part 1 - make the MediaDecoderReaderWrapper as a proxy of requesting media data; r=jwwang

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47989/diff/3-4/

Comment 14

[Tzuhao Kuo [:kaku]]

Comment on attachment 8743659 [details]
MozReview Request: Bug 1266027 part 2 - make MDSM and SeekTask to adopt new MediaDecoderReaderWrapper API; r=jwwang

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47991/diff/3-4/

Comment 15

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

Comment on attachment 8743658 [details]
MozReview Request: Bug 1266027 part 1 - make the MediaDecoderReaderWrapper as a proxy of requesting media data; r=jwwang

https://reviewboard.mozilla.org/r/47989/#review45705

::: dom/media/MediaDecoderReaderWrapper.h:52
(Diff revision 4)
> +      : mThis(aThis), mResolveMethod(aResolveMethod), mRejectMethod(aRejectMethod)
> +    {
> +    }
> +
> +    void
> +    CallHelper(void(ThisType::*)(MediaData*, TimeStamp),

Nit: Client migh pass a function that take (MediaData*, const TimeStamp&). It is a plus to allow such a flexibility.

::: dom/media/MediaDecoderReaderWrapper.h:89
(Diff revision 4)
> +      : mResolveFuntion(Move(aResolveFuntion)), mRejectFunction(Move(aRejectFunction))
> +    {
> +    }
> +
> +    void
> +    CallHelper(void(*)(MediaData*, TimeStamp),

|void(\*)(MediaData\*, TimeStamp)| won't work when client pass a function object. Does this really work with lambda?

::: dom/media/MediaDecoderReaderWrapper.cpp:147
(Diff revision 4)
>                                                       AbstractThread* aOwnerThread,
>                                                       MediaDecoderReader* aReader)
>    : mForceZeroStartTime(aIsRealTime || aReader->ForceZeroStartTime())
>    , mOwnerThread(aOwnerThread)
>    , mReader(aReader)
> +  , mAudioCallbackID(0)

It would be nice for mAudioCallbackID and mVideoCallbackID to occupy different spaces so you won't accidentally pass a VideoCallbackID to cancel the audio callback.

::: dom/media/MediaDecoderReaderWrapper.cpp:184
(Diff revision 4)
>    MOZ_ASSERT(!mShutdown);
>    return mStartTimeRendezvous->AwaitStartTime();
>  }
>  
> -RefPtr<MediaDecoderReaderWrapper::MediaDataPromise>
> +void
> +MediaDecoderReaderWrapper::CancelAudioCallback(CallbackRegistratonID aID)

Assert callbacks are already canceled in Shutdown() because client should always cancel callbacks before calling Shutdown().

::: dom/media/MediaDecoderReaderWrapper.cpp:338
(Diff revision 4)
>  MediaDecoderReaderWrapper::ResetDecode()
>  {
>    MOZ_ASSERT(mOwnerThread->IsCurrentThreadIn());
> +
> +  mAudioDataRequest.DisconnectIfExists();
> +  mVideoDataRequest.DisconnectIfExists();

Assert mAudioDataRequest is already disconnected in Shutdown() since we require client to always call ResetDecode() before Shutdown().

::: dom/media/MediaDecoderReaderWrapper.cpp:395
(Diff revision 4)
>    MOZ_ASSERT(mOwnerThread->IsCurrentThreadIn());
> -  if (!mShutdown) {
> +  MOZ_ASSERT(!mShutdown);
> +
> -    aSample->AdjustForStartTime(StartTime().ToMicroseconds());
> +  aSample->AdjustForStartTime(StartTime().ToMicroseconds());
> +
> +  // NOTE: Do NOT nullify the mRequestAudioDataCB and mRequestVideoDataCB

It looks like this comment is no longer needed.

::: dom/media/MediaDecoderReaderWrapper.cpp:420
(Diff revision 4)
> +void
> +MediaDecoderReaderWrapper::OnNotDecoded(CallbackBase* aCallback,
> +                                        MediaDecoderReader::NotDecodedReason aReason)
> +{
> +  MOZ_ASSERT(mOwnerThread->IsCurrentThreadIn());
> +

assert |!mShutdown|.

Comment 16

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

Comment on attachment 8743659 [details]
MozReview Request: Bug 1266027 part 2 - make MDSM and SeekTask to adopt new MediaDecoderReaderWrapper API; r=jwwang

https://reviewboard.mozilla.org/r/47991/#review45723

::: dom/media/MediaDecoderStateMachine.h:816
(Diff revision 4)
>    bool mIsVideoPrerolling;
>  
>    // Only one of a given pair of ({Audio,Video}DataPromise, WaitForDataPromise)
>    // should exist at any given moment.
>  
> -  MozPromiseRequestHolder<MediaDecoderReader::MediaDataPromise> mAudioDataRequest;
> +  int32_t mAudioCallbackID;

Use CallbackRegistratonID to provide high level abstraction. It will be easier to change the underlying type in the future when necessary.

::: dom/media/MediaDecoderStateMachine.cpp:235
(Diff revision 4)
>    mLowAudioThresholdUsecs(detail::LOW_AUDIO_USECS),
>    mAmpleAudioThresholdUsecs(detail::AMPLE_AUDIO_USECS),
>    mQuickBufferingLowDataThresholdUsecs(detail::QUICK_BUFFERING_LOW_DATA_USECS),
>    mIsAudioPrerolling(false),
>    mIsVideoPrerolling(false),
> +  mAudioCallbackID(-1),

Define an INVALID_ID instead of using a magic number like -1.

::: dom/media/MediaDecoderStateMachine.cpp:924
(Diff revision 4)
>  
>    nsresult rv = mReader->Init();
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  // Configure MediaDecoderReaderWrapper.
> +  r = NS_NewRunnableMethod(this, &MediaDecoderStateMachine::SetMediaDecoderReaderWrapperCallback);

You can do that in InitializationTask() without dispatching a new task.

::: dom/media/MediaDecoderStateMachine.cpp:946
(Diff revision 4)
> +  mVideoCallbackID =
> +    mReader->SetVideoCallback(this,
> +                              &MediaDecoderStateMachine::OnVideoDecoded,
> +                              &MediaDecoderStateMachine::OnVideoNotDecoded);
> +
> +  DECODER_LOG("MDSM set audio callbacks: mAudioCallbackID = %d\n", mAudioCallbackID);

The flexibility is reduced because you assume the callback IDs are always integers.

Comment 17

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

https://reviewboard.mozilla.org/r/47991/#review45727

::: dom/media/MediaDecoderStateMachine.cpp:1224
(Diff revision 4)
>  
>    mBufferedUpdateRequest.DisconnectIfExists();
>  
>    mQueuedSeek.RejectIfExists(__func__);
>    if (mSeekTask) {
>      mSeekTask->Discard();

This trio appears several times. It might be worth wrapping them into a function.

Comment 18

[Tzuhao Kuo [:kaku]]

Comment on attachment 8743658 [details]
MozReview Request: Bug 1266027 part 1 - make the MediaDecoderReaderWrapper as a proxy of requesting media data; r=jwwang

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47989/diff/4-5/

Comment 19

[Tzuhao Kuo [:kaku]]

Comment on attachment 8743659 [details]
MozReview Request: Bug 1266027 part 2 - make MDSM and SeekTask to adopt new MediaDecoderReaderWrapper API; r=jwwang

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47991/diff/4-5/

Comment 20

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

Comment on attachment 8743659 [details]
MozReview Request: Bug 1266027 part 2 - make MDSM and SeekTask to adopt new MediaDecoderReaderWrapper API; r=jwwang

https://reviewboard.mozilla.org/r/47991/#review45927

::: dom/media/MediaDecoderStateMachine.cpp:235
(Diff revisions 4 - 5)
>    mLowAudioThresholdUsecs(detail::LOW_AUDIO_USECS),
>    mAmpleAudioThresholdUsecs(detail::AMPLE_AUDIO_USECS),
>    mQuickBufferingLowDataThresholdUsecs(detail::QUICK_BUFFERING_LOW_DATA_USECS),
>    mIsAudioPrerolling(false),
>    mIsVideoPrerolling(false),
> -  mAudioCallbackID(-1),
> +  mAudioCallbackID(),

You don't need to put them in the initialization list if they are default constructed.

Comment 21

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

Comment on attachment 8743658 [details]
MozReview Request: Bug 1266027 part 1 - make the MediaDecoderReaderWrapper as a proxy of requesting media data; r=jwwang

https://reviewboard.mozilla.org/r/47989/#review45931

::: dom/media/MediaDecoderReaderWrapper.cpp:148
(Diff revision 5)
>                                                       MediaDecoderReader* aReader)
>    : mForceZeroStartTime(aIsRealTime || aReader->ForceZeroStartTime())
>    , mOwnerThread(aOwnerThread)
>    , mReader(aReader)
> +  , mAudioCallbackID(0)     // let audio callback id starts from 0.
> +  , mVideoCallbackID(1000)   // let video callback id starts from 1000.

The spaces could still overlap. Don't use magic numbers which could introduce subtle bugs.

::: dom/media/MediaDecoderReaderWrapperCallback.h:12
(Diff revision 5)
> +#ifndef MediaDecoderReaderWrapperCallback_h_
> +#define MediaDecoderReaderWrapperCallback_h_
> +
> +namespace mozilla {
> +
> +struct CallbackID

CallbackID should be defined inside class MediaDecoderReaderWrapper which is the only code to use it.

Comment 22

[Tzuhao Kuo [:kaku]]

Comment on attachment 8743658 [details]
MozReview Request: Bug 1266027 part 1 - make the MediaDecoderReaderWrapper as a proxy of requesting media data; r=jwwang

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47989/diff/5-6/

Comment 23

[Tzuhao Kuo [:kaku]]

Comment on attachment 8743659 [details]
MozReview Request: Bug 1266027 part 2 - make MDSM and SeekTask to adopt new MediaDecoderReaderWrapper API; r=jwwang

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47991/diff/5-6/

Comment 24

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

Comment on attachment 8743658 [details]
MozReview Request: Bug 1266027 part 1 - make the MediaDecoderReaderWrapper as a proxy of requesting media data; r=jwwang

https://reviewboard.mozilla.org/r/47989/#review46387

::: dom/media/MediaCallbackID.h:33
(Diff revision 6)
> +  bool operator!=(const CallbackID& rhs) const;
> +
> +  operator int() const;
> +
> +private:
> +  nsString mTag;

You can just store a |const char*| as we did in MozPromise.

https://hg.mozilla.org/mozilla-central/file/6adc822f5e27a55551faeb6c47a9bd8b0859a23b/xpcom/threads/MozPromise.h#l434

Comment 25

[Tzuhao Kuo [:kaku]]

Comment on attachment 8743658 [details]
MozReview Request: Bug 1266027 part 1 - make the MediaDecoderReaderWrapper as a proxy of requesting media data; r=jwwang

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47989/diff/6-7/

Comment 26

[Tzuhao Kuo [:kaku]]

Comment on attachment 8743659 [details]
MozReview Request: Bug 1266027 part 2 - make MDSM and SeekTask to adopt new MediaDecoderReaderWrapper API; r=jwwang

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47991/diff/6-7/

Comment 27

[Tzuhao Kuo [:kaku]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2af57013fcfa

Try looks good, thanks for the review!

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2949d5faa1b5
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a6514d2b612

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2949d5faa1b5
https://hg.mozilla.org/mozilla-central/rev/9a6514d2b612