Closed Bug 1395922 Opened 2 years ago Closed 2 years ago

Handle HTMLMediaElement::SetMediaKeys asynchronously.

Categories

(Core :: Audio/Video: Playback, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: kikuo, Assigned: kikuo)

References

Details

Attachments

(6 files, 5 obsolete files)

59 bytes, text/x-review-board-request
cpearce
: review+
Details
59 bytes, text/x-review-board-request
cpearce
: review+
jya
: review+
Details
59 bytes, text/x-review-board-request
cpearce
: review+
Details
59 bytes, text/x-review-board-request
cpearce
: review+
jya
: review+
Details
59 bytes, text/x-review-board-request
cpearce
: review+
Details
59 bytes, text/x-review-board-request
cpearce
: review+
Details
In order to make HTMLMediaElement be able to detach / reattach MediaKeys.
Since the CDMProxy is set to MediaFormatReader in MFR's task queue.

Making HTMLMediaElement::SetMediaKeys async would be a prerequisite.
Priority: -- → P3
Attachment #8904965 - Flags: review?(cpearce)
Attachment #8904966 - Flags: review?(cpearce)
Comment on attachment 8904965 [details]
Bug 1395922 [Part1]- Handle HTMLMediaElement::SetMediaKeys asynchronously.

https://reviewboard.mozilla.org/r/176792/#review182162

::: dom/html/HTMLMediaElement.h:1529
(Diff revision 1)
>  
>    // Encrypted Media Extension media keys.
>    RefPtr<MediaKeys> mMediaKeys;
> +  RefPtr<MediaKeys> mIncomingMediaKeys;
> +  RefPtr<DetailedPromise> mSetMediaKeysDOMPromise;
> +  Atomic<bool> mAttachingMediaKey;

I don't think mAttachcingMediaKey needs to be atomic. You seem to only be using it on the main thread?

::: dom/html/HTMLMediaElement.cpp:219
(Diff revision 1)
> +  bool DetachExistingMediaKeys() {
> +    // 5.1 If mediaKeys is not null, CDM instance represented by mediaKeys is
> +    // already in use by another media element, and the user agent is unable
> +    // to use it with this element, let this object's attaching media keys
> +    // value be false and reject promise with a new DOMException whose name
> +    // is QuotaExcmIncomingMediaKeyseededError.

Typo: QuotaExcmIncomingMediaKeyseededError

::: dom/html/HTMLMediaElement.cpp:236
(Diff revision 1)
> +      // with a new DOMException whose name is NotSupportedError.
> +      // 5.2.2 If the association cannot currently be removed, let this object's
> +      // attaching media keys value be false and reject promise with a new
> +      // DOMException whose name is InvalidStateError.
> +      if (mElement->mDecoder) {
> +        AsyncSetCDMProxy();

I'm confused by this.

This is the branch you're supposed to take if we don't support removing the association between the HTMLMediaElement and the MediaKeys. But in that branch, you remove part of the association, by calling AsyncSetCDMProxy(), which will eventually resolve the promise to a success value. But you return false here, so that the caller gives up re-attaching.

It seems to me that here you should either not be calling AsyncSetCDMProxy() if we can't (yet) handle removing the association, or if we can handle removing the association, you should call AsyncSetCDMProxy().

If we can't handle removing the association, you should ensure we reject the promise here.

::: dom/html/HTMLMediaElement.cpp:287
(Diff revision 1)
> +    }
> +    return true;
> +  }
> +
> +  NS_IMETHOD Run() {
> +      if (!DetachExistingMediaKeys()) {

You have an extra indent here. Can you run `./mach clang-format` before you land this please? That would automatically fix this.

::: dom/html/HTMLMediaElement.cpp:288
(Diff revision 1)
> +    return true;
> +  }
> +
> +  NS_IMETHOD Run() {
> +      if (!DetachExistingMediaKeys()) {
> +        return NS_OK;

This could be:
  if (!DetachExistingMediaKeys() ||
      !AttachNeweMediaKeysCDMProxy()) {
    return NS_OK;
  }

This makes the code more concise.

::: dom/html/HTMLMediaElement.cpp:300
(Diff revision 1)
> +  }
> +protected:
> +  RefPtr<HTMLMediaElement> mElement;
> +};
> +
> +

You have an extra blank line here. Can you run `./mach clang-format` before you land thtis please. That will automatically correct things like this.

::: dom/html/HTMLMediaElement.cpp:7000
(Diff revision 1)
> -    // media element for decrypting media data.
> -    if (NS_FAILED(aMediaKeys->Bind(this))) {
> -      // 5.3.2 If the preceding step failed, run the following steps:
> -      // 5.3.2.1 Set the mediaKeys attribute to null.
> -      mMediaKeys = nullptr;
> -      // 5.3.2.2 Let this object's attaching media keys value be false.
> +  mAttachingMediaKey = true;
> +  mIncomingMediaKeys = aMediaKeys;
> +  mSetMediaKeysDOMPromise = promise;
> +  // 4. Let promise be a new promise.
> +  // 5. Run the following steps in parallel:
> +  nsCOMPtr<nsIRunnable> r =

You can actually use RefPtr\<Runnable\> for this.

::: dom/html/HTMLMediaElement.cpp:7663
(Diff revision 1)
> +          // DOMException whose name is InvalidStateError.
> +          mAttachingMediaKey = false;
> +          promise->MaybeReject(result.Code(), result.Message());
> +        } else {
> +          if (mMediaKeys) {
> +            mMediaKeys->Unbind();

Didn't you already unbind on line 242?

::: dom/html/HTMLMediaElement.cpp:7667
(Diff revision 1)
> +          if (mMediaKeys) {
> +            mMediaKeys->Unbind();
> +          }
> +          // 5.4 Set the mediaKeys attribute to mediaKeys.
> +          mMediaKeys = mIncomingMediaKeys;
> +          // 5.5 Let this object's attaching media keys value be false.

You should set mIncomingMediaKeys to null on all code paths in this function, so we don't keep the old MediaKeys alive longer than we need to.

::: dom/html/HTMLMediaElement.cpp:7674
(Diff revision 1)
> +          // 5.6 Resolve promise.
> +          promise->MaybeResolveWithUndefined();
> +        }
> +      });
> +    mAbstractMainThread->Dispatch(r.forget());
> +    mSetMediaKeysDOMPromise = nullptr;

Calling mSetMediaKeysDOMPromise.forget() on line 7651 will set mSetMediaKeysDOMPromise to null, so this line is unnecessary.

::: dom/media/MediaDecoder.cpp:1388
(Diff revision 1)
>  {
>    MOZ_ASSERT(NS_IsMainThread());
> -  RefPtr<CDMProxy> proxy = aProxy;
> -  RefPtr<MediaFormatReader> reader = mReader;
> -  nsCOMPtr<nsIRunnable> r = NS_NewRunnableFunction(
> -    "MediaFormatReader::SetCDMProxy",
> +  mReader->SetCDMProxy(aProxy)
> +    ->Then(mAbstractMainThread, __func__,
> +      [=] (const MediaResult& aResult) {
> +        AbstractThread::AutoEnter context(AbstractMainThread());

Do you need the "AbstractThread::AutoEnter context(AbstractMainThread());"? Doesn't the promise's dispatch on the main thread wrapper setup the thread context for you?
Attachment #8904965 - Flags: review?(cpearce) → review-
Comment on attachment 8904966 [details]
Bug 1395922 [Part2]- [Test case] Playback clear content and drawImage via CanvasContext2D after detaching MediaKeys from HTMLMediaElement.

https://reviewboard.mozilla.org/r/176794/#review182182

::: dom/media/test/test_eme_unsetMediaKeys_then_capture.html:15
(Diff revision 1)
> +<body>
> +<pre id="test">
> +<script class="testbody" type="text/javascript">
> +var manager = new MediaTestManager;
> +
> +function startTest(test, token)

Please put a comment describing what case this test is testing.

::: dom/media/test/test_eme_unsetMediaKeys_then_capture.html:98
(Diff revision 1)
> +
> +function beginTest() {
> +  manager.runTests([gEMETests[0]], startTest);
> +}
> +
> +if (!IsMacOSSnowLeopardOrEarlier()) {

We don't support SnowLeopard anymore, so you don't need this !IsMacOSSNowLeopardOrEarlier() check. We can actually remove it from all our tests, but that should be done in a follow up.
Attachment #8904966 - Flags: review?(cpearce) → review-
Comment on attachment 8904965 [details]
Bug 1395922 [Part1]- Handle HTMLMediaElement::SetMediaKeys asynchronously.

https://reviewboard.mozilla.org/r/176792/#review182162

> I don't think mAttachcingMediaKey needs to be atomic. You seem to only be using it on the main thread?

My original intention is to dispatch the task to a non-main thread, and I'm not sure if the mAbstractMainThread is the one and the only main thread. It seems to be a DocGroup-specific main thread.
If they are the same thing, the atomic is unnecessary.

> I'm confused by this.
> 
> This is the branch you're supposed to take if we don't support removing the association between the HTMLMediaElement and the MediaKeys. But in that branch, you remove part of the association, by calling AsyncSetCDMProxy(), which will eventually resolve the promise to a success value. But you return false here, so that the caller gives up re-attaching.
> 
> It seems to me that here you should either not be calling AsyncSetCDMProxy() if we can't (yet) handle removing the association, or if we can handle removing the association, you should call AsyncSetCDMProxy().
> 
> If we can't handle removing the association, you should ensure we reject the promise here.

I found this hard to handle when I read the steps from spec.
Since, in our implementation, for cdm instance, removing-the-association and re-attaching is only an operation(pointer assignment in MFR), but they are seperately described in different steps.
The promise may be rejected if MFR's shutting down while removing the association. That's the only chance I can think of by now.


I do see flaws in this sequence, i.e new MediaKeys is not bound to HTMLMediaElement if there's an old MediaKeys in this case. I'll solve it in my next patch.

> Didn't you already unbind on line 242?

If the code goest to line 236, which means removing-the-association & attaching-new-association, we need to unbind when the promise is resolving.

But according to the issues above, I would re-write the flow a bit.

> Do you need the "AbstractThread::AutoEnter context(AbstractMainThread());"? Doesn't the promise's dispatch on the main thread wrapper setup the thread context for you?

Hmm ...I figured out the purpose of mAbstractMainThread.  "AbstractThread::AutoEnter context(AbstractMainThread());" is not needed, thanks :)
Comment on attachment 8904965 [details]
Bug 1395922 [Part1]- Handle HTMLMediaElement::SetMediaKeys asynchronously.

https://reviewboard.mozilla.org/r/176792/#review184346

I divided the process of setting cdm proxy to MFR into 2 phase. 
First, set originaly cdm proxy to nullptr and second, set new incoming proxy to MFR if there's one.
So that it would be easier to match the flow from spec.
Comment on attachment 8904965 [details]
Bug 1395922 [Part1]- Handle HTMLMediaElement::SetMediaKeys asynchronously.

https://reviewboard.mozilla.org/r/176792/#review185784

I'm on leave for the rest of the week, so please feel free to request review from jw.

I'm going to r- this again, as I think we need to handle the CDMProxy being unset/re-set in the MediaFormatReader. We need to shutdown and re-create any MediaDataDecoders that depend on the CDMProxy, as I describe in my comments below.

::: dom/html/HTMLMediaElement.cpp:232
(Diff revision 2)
> +    // 5.2.2 If the association cannot currently be removed, let this object's
> +    // attaching media keys value be false and reject promise with a new
> +    // DOMException whose name is InvalidStateError.
> +    if (mElement->mDecoder) {
> +      // We don't know if there's any API to query CDM if it supports removing
> +      // the association or not by now, but Gecko supports now.

This comment is't correct. Please update it.

The CDM has no concept of what media element it is attached to. It only see license requests, keyIds and promise IDs. So we don't need a special CDM API to implement this; we just need our media stack to be able to handle changing the CDMProxy at runtime.

The complexity of changing the CDMProxy at runtime is that when you change the MediaFormatReader's CDMProxy, any existing ChromiumCDMVideoDecoders and EMEDecryptors will point to the old CDMProxy. So we need to recreate the EME MediaDataDecoders to use the new CDMProxy.

If we're removing the CDMProxy and we have EME MediaDataDecoders, we need to revert the MediaFormatReader back to a waiting for keys state.

::: dom/media/MediaFormatReader.cpp:1327
(Diff revision 2)
>    RefPtr<CDMProxy> proxy = aProxy;
>    RefPtr<MediaFormatReader> self = this;
>    nsCOMPtr<nsIRunnable> r =
>      NS_NewRunnableFunction("MediaFormatReader::SetCDMProxy", [=]() {
>        MOZ_ASSERT(self->OnTaskQueue());
> +      LOGV("SetCDMProxy (%x)", proxy.get());

Does this cause any active MediaDataDecoders that depend on the CDMProxy to be re-created, or shutdown  if the CDMProxy is being set to null?
Attachment #8904965 - Flags: review?(cpearce) → review-
Comment on attachment 8904966 [details]
Bug 1395922 [Part2]- [Test case] Playback clear content and drawImage via CanvasContext2D after detaching MediaKeys from HTMLMediaElement.

https://reviewboard.mozilla.org/r/176794/#review185786

::: dom/media/test/test_eme_unsetMediaKeys_then_capture.html:18
(Diff revision 2)
> +var manager = new MediaTestManager;
> +
> +// Test that if we can capture a video frame while playing clear content after
> +// removing the MediaKeys object which was used for a previous encrypted content
> +// playback on the same video element
> +function startTest(test, token)

We also should test (in a separate test case would probably be best) starting up an EME session, setting the MediaKeys, starting playback, stopping playback, removing the MediaKeys. Re-attaching the same MediaKeys, and restarting playback. This should test the re-creating of EME MediaDataDecoders code.
Attachment #8904966 - Flags: review?(cpearce) → review-
(In reply to (Away Sep19-22) Chris Pearce (:cpearce) from comment #9)
> Comment on attachment 8904965 [details]
> Bug 1395922 [Part1]- Handle HTMLMediaElement::SetMediaKeys asynchronously.
> 
> https://reviewboard.mozilla.org/r/176792/#review185784
> 
> I'm on leave for the rest of the week, so please feel free to request review
> from jw.
> 
> I'm going to r- this again, as I think we need to handle the CDMProxy being
> unset/re-set in the MediaFormatReader. We need to shutdown and re-create any
> MediaDataDecoders that depend on the CDMProxy, as I describe in my comments
> below.
> 
> ::: dom/html/HTMLMediaElement.cpp:232
> (Diff revision 2)
> > +    // 5.2.2 If the association cannot currently be removed, let this object's
> > +    // attaching media keys value be false and reject promise with a new
> > +    // DOMException whose name is InvalidStateError.
> > +    if (mElement->mDecoder) {
> > +      // We don't know if there's any API to query CDM if it supports removing
> > +      // the association or not by now, but Gecko supports now.
> 
> This comment is't correct. Please update it.
> 
> The CDM has no concept of what media element it is attached to. It only see
> license requests, keyIds and promise IDs. So we don't need a special CDM API
> to implement this; we just need our media stack to be able to handle
> changing the CDMProxy at runtime.
> 
> The complexity of changing the CDMProxy at runtime is that when you change
> the MediaFormatReader's CDMProxy, any existing ChromiumCDMVideoDecoders and
> EMEDecryptors will point to the old CDMProxy. So we need to recreate the EME
> MediaDataDecoders to use the new CDMProxy.

Oh! I totally missed that part. Thanks for pointing it out.

> 
> If we're removing the CDMProxy and we have EME MediaDataDecoders, we need to
> revert the MediaFormatReader back to a waiting for keys state.
> 
> ::: dom/media/MediaFormatReader.cpp:1327
> (Diff revision 2)
> >    RefPtr<CDMProxy> proxy = aProxy;
> >    RefPtr<MediaFormatReader> self = this;
> >    nsCOMPtr<nsIRunnable> r =
> >      NS_NewRunnableFunction("MediaFormatReader::SetCDMProxy", [=]() {
> >        MOZ_ASSERT(self->OnTaskQueue());
> > +      LOGV("SetCDMProxy (%x)", proxy.get());
> 
> Does this cause any active MediaDataDecoders that depend on the CDMProxy to
> be re-created, or shutdown  if the CDMProxy is being set to null?
(In reply to (Away Sep19-22) Chris Pearce (:cpearce) from comment #9)
> Comment on attachment 8904965 [details]
> Bug 1395922 [Part1]- Handle HTMLMediaElement::SetMediaKeys asynchronously.
> 
> https://reviewboard.mozilla.org/r/176792/#review185784
> 
> I'm on leave for the rest of the week, so please feel free to request review
> from jw.
> 
> I'm going to r- this again, as I think we need to handle the CDMProxy being
> unset/re-set in the MediaFormatReader. We need to shutdown and re-create any
> MediaDataDecoders that depend on the CDMProxy, as I describe in my comments
> below.
> 
> ::: dom/html/HTMLMediaElement.cpp:232
> (Diff revision 2)
> > +    // 5.2.2 If the association cannot currently be removed, let this object's
> > +    // attaching media keys value be false and reject promise with a new
> > +    // DOMException whose name is InvalidStateError.
> > +    if (mElement->mDecoder) {
> > +      // We don't know if there's any API to query CDM if it supports removing
> > +      // the association or not by now, but Gecko supports now.
> 
> This comment is't correct. Please update it.
> 
> The CDM has no concept of what media element it is attached to. It only see
> license requests, keyIds and promise IDs. So we don't need a special CDM API
> to implement this; we just need our media stack to be able to handle
> changing the CDMProxy at runtime.
> 
> The complexity of changing the CDMProxy at runtime is that when you change
> the MediaFormatReader's CDMProxy, any existing ChromiumCDMVideoDecoders and
> EMEDecryptors will point to the old CDMProxy. So we need to recreate the EME
> MediaDataDecoders to use the new CDMProxy.

Ah! That's the part I missed. Thanks for pointing it out, I'll provide a patch for this.

> 
> If we're removing the CDMProxy and we have EME MediaDataDecoders, we need to
> revert the MediaFormatReader back to a waiting for keys state.
>
Attachment #8904965 - Attachment is obsolete: true
Attachment #8904966 - Attachment is obsolete: true
Attachment #8910592 - Flags: review?(jwwang)
Attachment #8910593 - Flags: review?(jwwang)
Attachment #8910594 - Flags: review?(jwwang)
Comment on attachment 8910594 [details]
Bug 1395922 [Part3]- [Test case] Encrypted content playback could be stopped and resumed after detaching and reattaching same MediaKeys.

https://reviewboard.mozilla.org/r/182042/#review187404

I found that the playback clock deos not stop when CDMProxy was removed, and that results to frame discarding after re-attaching original CDMProxy and seeking to the time when CDMProxy was removed. Need to fix that.
Cancel the r?
Attachment #8910592 - Flags: review?(jwwang)
Attachment #8910593 - Flags: review?(jwwang)
Attachment #8910594 - Flags: review?(jwwang)
Comment on attachment 8910592 [details]
Bug 1395922 [Part1]- Handle HTMLMediaElement::SetMediaKeys asynchronously.

https://reviewboard.mozilla.org/r/182038/#review187406

::: dom/media/MediaDecoder.cpp:1386
(Diff revision 1)
>  
> -void
> +RefPtr<MediaDecoderOwner::SetCDMPromise>
>  MediaDecoder::SetCDMProxy(CDMProxy* aProxy)
>  {
>    MOZ_ASSERT(NS_IsMainThread());
> -  RefPtr<CDMProxy> proxy = aProxy;
> +  return mReader->SetCDMProxy(aProxy);

See MediaDecoderStateMachine::InvokeSeek().

It is more concise to use InvokeAsync().

::: dom/media/MediaDecoderOwner.h:12
(Diff revision 1)
>  #define MediaDecoderOwner_h_
>  
> +#include "mozilla/MozPromise.h"
>  #include "mozilla/UniquePtr.h"
>  #include "MediaInfo.h"
> +#include "nsIDocument.h"

We will resue MediaDecoderOwner in Servo. Please don't add Gecko specific stuff to it. You should create a new header to define SetCDMPromise.

::: dom/media/MediaFormatReader.h:780
(Diff revision 1)
> +  // Check if the CDMProxy attaching procedure is done and resolve the promise
> +  // which is requested from MediaElement.
> +  void CheckCDMProxyAttachingStatus(TrackType aTrack);
> +
> +  // Used to identify if it's still in CDMProxy attaching procedure.
> +  uint8_t mAttachingCDMProxy;

EnumSet is your friend.

::: dom/media/MediaFormatReader.cpp:1322
(Diff revision 1)
>  
>    return NS_OK;
>  }
>  
> -void
> +RefPtr<MediaDecoderOwner::SetCDMPromise>
>  MediaFormatReader::SetCDMProxy(CDMProxy* aProxy)

1. Assert OnTaskQueue(). The caller must use InvokeAsync() to ensure this function runs on the task queue thread of this reader.
2. No need to dispatch a task to do things asynchronously at #1349 for you're already on the right thread following 1.

::: dom/media/MediaFormatReader.cpp:1340
(Diff revision 1)
>      NS_NewRunnableFunction("MediaFormatReader::SetCDMProxy", [=]() {
>        MOZ_ASSERT(self->OnTaskQueue());
> -      self->mCDMProxy = proxy;
> -      if (HasAudio()) {
> -        self->ScheduleUpdate(TrackInfo::kAudioTrack);
> +      LOGV("SetCDMProxy (%x)", proxy.get());
> +
> +      self->mAttachingCDMProxy |= (ATTACHING_CDMPROXY_AUDIO | ATTACHING_CDMPROXY_VIDEO);
> +      if (!proxy) {

Are we allowed to attach a new CDMProxy without detaching the old one first?

::: dom/media/MediaFormatReader.cpp:1346
(Diff revision 1)
> +        // Decoders should be shut down when CDMProxy is going to be detached as
> +        // MFR should not use the old decoders if any new CDMProxy is set.
> +        self->ReleaseResources();
>        }
> -      if (HasVideo()) {
> +      self->mCDMProxy = proxy;
> -        self->ScheduleUpdate(TrackInfo::kVideoTrack);
> +      self->ScheduleUpdate(TrackInfo::kVideoTrack);

Why bother to schedule an update when there is no video track?

::: dom/media/MediaFormatReader.cpp:2242
(Diff revision 1)
>      ->Track(decoder.mDrainRequest);
>    LOG("Requesting %s decoder to drain", TrackTypeToStr(aTrack));
>  }
>  
>  void
> +MediaFormatReader::CheckCDMProxyAttachingStatus(TrackType aTrack)

Can you explain how this function works to determine when setting cdmproxy is done?

::: dom/media/MediaFormatReader.cpp:2246
(Diff revision 1)
>  void
> +MediaFormatReader::CheckCDMProxyAttachingStatus(TrackType aTrack)
> +{
> +  MOZ_ASSERT(OnTaskQueue());
> +  auto& decoder = GetDecoderData(aTrack);
> +  if (decoder.mFlushed && mAttachingCDMProxy != 0) {

Why checking mFlushed?
Attachment #8910592 - Flags: review?(jwwang)
Comment on attachment 8910592 [details]
Bug 1395922 [Part1]- Handle HTMLMediaElement::SetMediaKeys asynchronously.

https://reviewboard.mozilla.org/r/182038/#review187406

> Are we allowed to attach a new CDMProxy without detaching the old one first?

According to the modification I added in HTMLMediaElement, when |HTMLMediaElement::SetMediaKeys| is called, at first we will set MFR::mCDMProxy to null (triggered in HTMLMediaElement::SetMediaKeysRunner::TryAsyncRemoveCDMAssociation), and then set MFR::mCDMProxy to the new one (triggred in HTMLMediaElement::SetMediaKeysRunner::TryAsyncMakeAssociationWithCDM).
Comment on attachment 8910592 [details]
Bug 1395922 [Part1]- Handle HTMLMediaElement::SetMediaKeys asynchronously.

https://reviewboard.mozilla.org/r/182038/#review187406

> Why bother to schedule an update when there is no video track?

For me, it's simple and neat here to not consider audio or video separately, and it cost quite little to scheule an update.

> Can you explain how this function works to determine when setting cdmproxy is done?

MFR won't craete MediaDataDecoder if there's no mCDMProxy.
I'd like to resolve the SetCDMProxy promise once the MDD are shut down after MFR::SetCDMProxy(nullptr) is called, so SetCDMProxy(nullptr) should trigger the shutdown procedure which is MFR::ReleaseResource.

So when SetCDMProxy is called, the variable mAttachingCDMProxy would be set to 3 (bitwise operation, 3 = 1 | 2, for both Audio and Video),
if MDDs are shut down, the DecoderData.mFlush will be set to true, and this line

    mAttachingCDMProxy ^= flag;

will flip the value back to 0.
After that, MFR can be sure the procedure is done. (No existing MDDs before MFR::mCDMProxy being set.)

> Why checking mFlushed?

Shutdown MDD makes DecoderData.mFlushed becomes true.
Attachment #8910592 - Attachment is obsolete: true
Attachment #8910592 - Flags: review?(jwwang)
Attachment #8910593 - Attachment is obsolete: true
Attachment #8910594 - Attachment is obsolete: true
Comment on attachment 8913126 [details]
Bug 1395922 - [P1] Refactor code and move them into specific functions.

https://reviewboard.mozilla.org/r/184548/#review189682

Addressed issues from cpearce's & jwwang's previous reviews.
As I refactored these part of code, I found that the algorithms description for EME [1] were not commented informatively.
Those should be potential follow-ups, i.e. resume playback algorithm [2] .

[1] https://www.w3.org/TR/2017/REC-encrypted-media-20170918/#htmlmediaelement-algorithms
[2] https://www.w3.org/TR/2017/REC-encrypted-media-20170918/#resume-playback
Attachment #8913126 - Flags: review?(cpearce)
Attachment #8913127 - Flags: review?(cpearce)
Attachment #8913128 - Flags: review?(cpearce)
Attachment #8913129 - Flags: review?(cpearce)
Attachment #8913130 - Flags: review?(cpearce)
Attachment #8913131 - Flags: review?(cpearce)
Comment on attachment 8913126 [details]
Bug 1395922 - [P1] Refactor code and move them into specific functions.

https://reviewboard.mozilla.org/r/184550/#review190498

::: dom/html/HTMLMediaElement.cpp:6922
(Diff revision 1)
> +  // 5.6 Resolve promise.
> +  aPromise->MaybeResolveWithUndefined();
> +}
> +
> +bool
> +HTMLMediaElement::TryMakeAssociationWithCDM(CDMProxy* aProxy)

This function always reports that it succeeds; it always returns true. This is confusing. You should make this function return void, and not name it TryFoo. Better to call it MaybeFoo if it may or may not do thing, but we won't fail if it doesn't do the thing.
Attachment #8913126 - Flags: review?(cpearce) → review+
Comment on attachment 8913131 [details]
Bug 1395922 - [P6][Test] Playback should resume after reattaching same MediaKeys with valid sessions.

https://reviewboard.mozilla.org/r/184560/#review190504

::: dom/media/test/test_eme_detach_reattach_same_mediakeys_during_playback.html:63
(Diff revision 1)
> +    .then(p.resolve, p.reject);
> +    return p.promise;
> +  }
> +
> +  function setMediaKeysToElement(mk, solve, reject) {
> +    v.setMediaKeys(mk)

Can't this be:
v.setMediaKeys(mk).then(solve, reject);

::: dom/media/test/test_eme_detach_reattach_same_mediakeys_during_playback.html:95
(Diff revision 1)
> +  }
> +  function onVideoPlaying(ev) {
> +    function onSetMediaKeysToNullOK() {
> +      ok(true, TimeStamp(token) + " Set MediaKeys to null. OK!");
> +      SimpleTest.requestFlakyTimeout("To reattach mediakeys back again in 2s.");
> +      sleep(2000).then(() => {

Can this be:
sleep(2000).then(ReattachOriMediaKeys);

::: dom/media/test/test_eme_detach_reattach_same_mediakeys_during_playback.html:103
(Diff revision 1)
> +    }
> +    function onSetMediaKeysToNullFailed() {
> +      ok(false, TimeStamp(token) + " Set MediaKeys to null. FAILED!");
> +    }
> +
> +    v.removeEventListener("playing", onVideoPlaying);

You could use the "once" function to do the add/remove listener for you.

::: dom/media/test/test_eme_detach_reattach_same_mediakeys_during_playback.html:111
(Diff revision 1)
> +      setMediaKeysToElement(null, onSetMediaKeysToNullOK, onSetMediaKeysToNullFailed);
> +    });
> +  }
> +
> +  v.addEventListener("ended", onVideoEnded);
> +  v.addEventListener("playing", onVideoPlaying);

It's entirely possible that the playing event never fires because we fail to produce decoded data faster than it's consumed by playback, even though we are actually playing.

This also means that the "ended" event listener may fire before the "playing" event fires.

So it's a bad idea to make tests rely on the "playing" event.

You'd be better to load the media and not play it, and wait on the "canplay" event, as when that fires you know frames have been decoded. Then remove the MediaKeys and seek (so that the decoder is flushed), and then wait upon "canplay" again.
Attachment #8913131 - Flags: review?(cpearce) → review-
Comment on attachment 8913130 [details]
Bug 1395922 - [P5][Test] Capture clear content with canvas 2d after remove MediaKeys from the same HTMLMediaElement.

https://reviewboard.mozilla.org/r/184558/#review190514

::: dom/media/test/test_eme_unsetMediaKeys_then_capture.html:47
(Diff revision 1)
> +    function playClearVideo() {
> +      var p1 = once(v, 'ended', function onended(e) {
> +        ok(true, TimeStamp(token) + " (CLEAR) content playback ended.");
> +        console.log(" bipbop.mp4 playback ended !!");
> +      });
> +      var p2 = once(v, 'playing', function onplay(e) {

You probably want to use "loadeddata" here, because as I described in the review of the next test, you can't always rely on "playing" firing.
Attachment #8913130 - Flags: review?(cpearce) → review+
Comment on attachment 8913127 [details]
Bug 1395922 - [P2] Make MediaFormatReader::SetCDMProxy asynchronously with a promise.

https://reviewboard.mozilla.org/r/184552/#review190518

Please address those comments, and then re-request review from both myself and jya.

::: dom/media/MediaFormatReader.h:207
(Diff revision 1)
>    bool UseBufferingHeuristics() const { return mTrackDemuxersMayBlock; }
>  
> -  void SetCDMProxy(CDMProxy* aProxy);
> +  void CheckCDMAttachingStatus(TrackType aTrack);
> +  RefPtr<media::SetCDMPromise> SetCDMProxy(CDMProxy* aProxy);
> +  MozPromiseHolder<media::SetCDMPromise> mSetCDMPromise;
> +  SetCDMActionSet mSetCDMActionSet{};

Why don't you make this a TrackSet, then you don't need to create a new enum which is equivalent, and it would simplify your logic.

It's not clear that you're actually using the values of this enum to cause any action to happen, so calling it an action set seems misleading.

::: dom/media/MediaFormatReader.cpp:1319
(Diff revision 1)
>  
>    return NS_OK;
>  }
>  
>  void
> +MediaFormatReader::CheckCDMAttachingStatus(TrackType aTrack)

CheckCDMAttachingStatus() is not a good name. What you're doing in this function is you're resolving the mSetCDMPromise if all tracks have processed an update since the last CDM set and have shutdown their decoders. Right? So the name should describe that.

::: dom/media/MediaFormatReader.cpp:1325
(Diff revision 1)
> +{
> +  // When a CDM proxy is set, MFR would shutdown old MediaDataDecoders (if
> +  // there's one) and would create new one for specific track in the next Update.
> +  MOZ_ASSERT(OnTaskQueue());
> +  auto& decoder = GetDecoderData(aTrack);
> +  if (!decoder.mDecoder && !mSetCDMActionSet.isEmpty()) {

Why are you only resolving the promise if the decoder is null? Is it so that you can detect when each track has shutdown its decoder? You know that the decoders will be null after you call ReleaseResources() in SetCDMProxy().

::: dom/media/MediaFormatReader.cpp:1337
(Diff revision 1)
> +
> +    MOZ_ASSERT(!mSetCDMPromise.IsEmpty());
> +    if (mSetCDMActionSet.isEmpty()) {
> +      mSetCDMPromise.Resolve(MediaResult(NS_OK), __func__);
> +    }
> +    ScheduleUpdate(aTrack);

Why do you need to schedule another update here? It's not obvious why you need to, as it would seem the logic for the current pass shouldn't be different for a second pass?

You t least need a comment here to describe why you need to do this, as it's not obvious you do need to.

::: dom/media/MediaFormatReader.cpp:1376
(Diff revision 1)
> +  // should be decrypted/decoded by new created MediaDataDecoder.
> +  ReleaseResources();
> +  mCDMProxy = aProxy;
> +
> +  if (HasAudio()) {
> +    ScheduleUpdate(TrackInfo::kAudioTrack);

You've got two sets of if audio/video. Can you schedule these update in the if statements above, so that you only need one set of if statements?
Attachment #8913127 - Flags: review?(cpearce) → review-
Comment on attachment 8913128 [details]
Bug 1395922 - [P3] Make HTMLMediaElement::SetMediaKeys asynchronously.

https://reviewboard.mozilla.org/r/184554/#review190500

::: dom/html/HTMLMediaElement.cpp:7050
(Diff revision 1)
>    if (mMediaKeys == aMediaKeys) {
>      promise->MaybeResolveWithUndefined();
>      return promise.forget();
>    }
>  
>    // Note: Our attaching code is synchronous, so we can skip the following steps.

You're making attaching the mediakeys async, so this comment on line 7050 is now out of date.
Attachment #8913128 - Flags: review?(cpearce) → review+
Comment on attachment 8913129 [details]
Bug 1395922 - [P4] Make MDSM enter buffering state when MediaKeys is removed and resume the playback if setting same MediaKeys back.

https://reviewboard.mozilla.org/r/184556/#review190526

I don't think we should do it this way, as it results in us firing events which are detectable to JavaScript.

::: dom/html/HTMLMediaElement.cpp:6886
(Diff revision 1)
>      mDecoder->SetCDMProxy(nullptr)
>        ->Then(mAbstractMainThread, __func__,
>            [self](const MediaResult& aResult) {
>              self->mSetCDMRequest.Complete();
>  
> +            if (self->mPlayTime.IsStarted()) {

You could just check whether the media element's paused attribute is true?

But regardless, I don't think you should do it this way. I think it makes more sense to do this at the MediaDecoderStateMachine or MediaFormatReader level.

::: dom/html/HTMLMediaElement.cpp:7057
(Diff revision 1)
> +  //        HAVE_FUTURE_DATA or HAVE_ENOUGH_DATA as appropriate.
> +  double playedTime = mPlayTime.Total();
> +  if (playedTime != 0.f) {
> +    IgnoredErrorResult rv;
> +    RefPtr<Promise> toBeIgnored = Play(rv);
> +    SetCurrentTime(playedTime);

This will cause us to fire "seeking", "seeked" and "play" events. 

This would confuse custom JavaScript controls.

We definitely should not be firing "seeking", and "seeked". I don't think we should be firing "play" either, as that's supposed to fire when JS calls play().
Attachment #8913129 - Flags: review?(cpearce) → review-
Comment on attachment 8913126 [details]
Bug 1395922 - [P1] Refactor code and move them into specific functions.

https://reviewboard.mozilla.org/r/184550/#review190498

> This function always reports that it succeeds; it always returns true. This is confusing. You should make this function return void, and not name it TryFoo. Better to call it MaybeFoo if it may or may not do thing, but we won't fail if it doesn't do the thing.

The part will be addressed in P3 to indicate if the operation is done synchronously or not, so I'll leave it as it is.
Comment on attachment 8913127 [details]
Bug 1395922 - [P2] Make MediaFormatReader::SetCDMProxy asynchronously with a promise.

https://reviewboard.mozilla.org/r/184552/#review190518

> Why are you only resolving the promise if the decoder is null? Is it so that you can detect when each track has shutdown its decoder? You know that the decoders will be null after you call ReleaseResources() in SetCDMProxy().

Code has been changed, this issue is gone.

> Why do you need to schedule another update here? It's not obvious why you need to, as it would seem the logic for the current pass shouldn't be different for a second pass?
> 
> You t least need a comment here to describe why you need to do this, as it's not obvious you do need to.

Issue is gone now.
Comment on attachment 8913129 [details]
Bug 1395922 - [P4] Make MDSM enter buffering state when MediaKeys is removed and resume the playback if setting same MediaKeys back.

https://reviewboard.mozilla.org/r/184556/#review190526

> You could just check whether the media element's paused attribute is true?
> 
> But regardless, I don't think you should do it this way. I think it makes more sense to do this at the MediaDecoderStateMachine or MediaFormatReader level.

Implementation has been changed. Close the issue.

> This will cause us to fire "seeking", "seeked" and "play" events. 
> 
> This would confuse custom JavaScript controls.
> 
> We definitely should not be firing "seeking", and "seeked". I don't think we should be firing "play" either, as that's supposed to fire when JS calls play().

Addressed in new implementation.
Comment on attachment 8913127 [details]
Bug 1395922 - [P2] Make MediaFormatReader::SetCDMProxy asynchronously with a promise.

https://reviewboard.mozilla.org/r/184552/#review195302

::: dom/media/MediaFormatReader.h:199
(Diff revision 2)
>    // cases like MSE.
>    bool UseBufferingHeuristics() const { return mTrackDemuxersMayBlock; }
>  
> -  void SetCDMProxy(CDMProxy* aProxy);
> +  void ResolveSetCDMPromiseIfDone(TrackType aTrack);
> +  RefPtr<media::SetCDMPromise> SetCDMProxy(CDMProxy* aProxy);
> +  MozPromiseHolder<media::SetCDMPromise> mSetCDMPromise;

Data fields should be private, use functions to ensure access of interal state is safe.

So move mSetCDMPromise and mSetCDMForTracks into a "private:" region.

::: dom/media/MediaFormatReader.cpp:1321
(Diff revision 2)
>  }
>  
>  void
> +MediaFormatReader::ResolveSetCDMPromiseIfDone(TrackType aTrack)
> +{
> +  // When a CDM proxy is set, MFR would shutdown old MediaDataDecoders (if

English grammar: You're mixing singular and plurals here. "MediaDataDecoders" is plural (more than one thing) whereas "if there's one" implies one thing (singular).

::: dom/media/MediaFormatReader.cpp:1364
(Diff revision 2)
> -      if (HasVideo()) {
> +  if (HasVideo()) {
> -        self->ScheduleUpdate(TrackInfo::kVideoTrack);
> +    mSetCDMForTracks += TrackInfo::kVideoTrack;
> +    ScheduleUpdate(TrackInfo::kVideoTrack);
> -      }
> +  }
> -    });
> -  OwnerThread()->Dispatch(r.forget());
> +
> +  // Shutdown all decoders as switching CDM proxy indicates new content

This comment is misleading. Switching CDMProxy doesn't necessarily indicate new content. It does however mean that it's inappropriate for the existing decoders to continue to decode using the old CDMProxy.

Please update this comment.
Attachment #8913127 - Flags: review?(cpearce) → review+
Comment on attachment 8913129 [details]
Bug 1395922 - [P4] Make MDSM enter buffering state when MediaKeys is removed and resume the playback if setting same MediaKeys back.

https://reviewboard.mozilla.org/r/184556/#review195314

::: dom/media/MediaFormatReader.cpp:2468
(Diff revision 2)
>        decoder.RejectPromise(NS_ERROR_DOM_MEDIA_END_OF_STREAM, __func__);
>      } else if (decoder.mWaitingForKey) {
>        LOG("Rejecting %s promise: WAITING_FOR_DATA due to waiting for key",
>            TrackTypeToStr(aTrack));
>        decoder.RejectPromise(NS_ERROR_DOM_MEDIA_WAITING_FOR_DATA, __func__);
> +    } else if (decoder.mWaitingForCDM) {

It seems mWaitingForCDM should be true if mCDMProxy is null and if mSetCDMForTracks contains aTrack? Why not write a helper function that computes that, so you don't have to ensure that mWaitingForCDM is consistent with that logic?

"Let the world be its own model". It's usually simpler to calculate some state based on it's inputs, rather than maintain state which is a function of inputs, as then you need to worry about keeping the state in sync with its inputs.

::: dom/media/platforms/PDMFactory.cpp:443
(Diff revision 2)
>  PDMFactory::SetCDMProxy(CDMProxy* aProxy)
>  {
> -  MOZ_ASSERT(aProxy);
> -
>  #ifdef MOZ_WIDGET_ANDROID
>    if (IsWidevineKeySystem(aProxy->KeySystem())) {

You've removed the assertion that aProxy is not null, but on Android you're dereferencing aProxy, which may be null.

It seems that if aProxy is null, you probably don't want mEMEPDM to be non-null?
Attachment #8913129 - Flags: review?(cpearce) → review-
Comment on attachment 8913131 [details]
Bug 1395922 - [P6][Test] Playback should resume after reattaching same MediaKeys with valid sessions.

https://reviewboard.mozilla.org/r/184560/#review195326

Great, thanks.
Attachment #8913131 - Flags: review?(cpearce) → review+
Comment on attachment 8913127 [details]
Bug 1395922 - [P2] Make MediaFormatReader::SetCDMProxy asynchronously with a promise.

https://reviewboard.mozilla.org/r/184552/#review195330

I don't see why you need a promise seeing that the caller makes no use of it.

::: dom/media/MediaDecoder.h:700
(Diff revision 2)
>  
>    bool mTelemetryReported;
>    const MediaContainerType mContainerType;
>    bool mCanPlayThrough = false;
> +
> +  MozPromiseRequestHolder<media::SetCDMPromise> mSetCDMRequest;

you never disconnect this request holder.

so why do you need one in the first place?

::: dom/media/MediaDecoder.cpp:1388
(Diff revision 2)
>  MediaDecoder::SetCDMProxy(CDMProxy* aProxy)
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>    RefPtr<CDMProxy> proxy = aProxy;
> -  RefPtr<MediaFormatReader> reader = mReader;
> -  nsCOMPtr<nsIRunnable> r = NS_NewRunnableFunction(
> +  RefPtr<MediaDecoder> self = this;
> +  InvokeAsync(mReader->OwnerThread(),

I particularly dislike when MediaDecoder needs to know about the MediaFormatReader's threading model...

but seeing that it's done like that elsewhere....

::: dom/media/MediaDecoder.cpp:1395
(Diff revision 2)
> -    [reader, proxy]() {
> -    reader->SetCDMProxy(proxy);
> -    });
> -  mReader->OwnerThread()->Dispatch(r.forget());
> +              [self, proxy]() { return self->mReader->SetCDMProxy(proxy); })
> +    ->Then(mAbstractMainThread,
> +           __func__,
> +           [self](const MediaResult& aResult) {
> +             self->mSetCDMRequest.Complete();
> +             SetCDMPromise::CreateAndResolve(aResult, __func__);

what is this supposed to do ?

shouldn't you be returning that promise?

Why do you even bother with a promise here?
you never use the result anywhere

all you care is that you call MFR::SetCDMProxy on its own thread.

::: dom/media/MediaDecoder.cpp:1399
(Diff revision 2)
> +             self->mSetCDMRequest.Complete();
> +             SetCDMPromise::CreateAndResolve(aResult, __func__);
> +           },
> +           [self](const MediaResult& aError) {
> +             self->mSetCDMRequest.Complete();
> +             SetCDMPromise::CreateAndReject(aError, __func__);

same here

::: dom/media/MediaFormatReader.cpp:1350
(Diff revision 2)
> +      __func__);
> +  }
> +
> +  if (!mSetCDMPromise.IsEmpty()) {
> +    return media::SetCDMPromise::CreateAndReject(
> +      MediaResult(NS_ERROR_DOM_INVALID_STATE_ERR,

shouldn't you handle that error in the caller then to do it again?

or cancel the current one and restart as I'm guessing the existing pending SetCDMProxy is no longer of interest.

::: dom/media/MediaPromiseDefs.h:12
(Diff revision 2)
> +#define MediaPromiseDefs_h_
> +
> +#include "MediaResult.h"
> +#include "mozilla/MozPromise.h"
> +
> +namespace mozilla {

I'm not sure we need this file, may as well just have the single definition in MediaDecoder.h
Attachment #8913127 - Flags: review?(jyavenard) → review-
Comment on attachment 8913129 [details]
Bug 1395922 - [P4] Make MDSM enter buffering state when MediaKeys is removed and resume the playback if setting same MediaKeys back.

https://reviewboard.mozilla.org/r/184556/#review195336

::: commit-message-c3ba0:1
(Diff revision 2)
> +Bug 1395922 - [P4] Make MDSM enters buffering state when MediaKeys is removed and resume the playback if setting same MediaKeys back.

Make MDSM enter buffering state....

::: dom/media/MediaFormatReader.cpp:1360
(Diff revision 2)
> +  if (mCDMProxy) {
> +    // An old cdm proxy exists, so detaching old cdm proxy by shutting down
> +    // MediaDataDecoder.
> +    auto& decoder = GetDecoderData(aTrack);
> +    ShutdownDecoder(aTrack);
> +    // Setting mWaitingForCDM to true can result a rejected promise to MDSM,

can result in a rejected promise ...

::: dom/media/MediaFormatReader.cpp:1362
(Diff revision 2)
> +    // MediaDataDecoder.
> +    auto& decoder = GetDecoderData(aTrack);
> +    ShutdownDecoder(aTrack);
> +    // Setting mWaitingForCDM to true can result a rejected promise to MDSM,
> +    // so that playback enter buffering state, once a qualified cdm proxy is set
> +    // back again, playback continues.

I don't understand that sentence.
Comment on attachment 8913127 [details]
Bug 1395922 - [P2] Make MediaFormatReader::SetCDMProxy asynchronously with a promise.

https://reviewboard.mozilla.org/r/184552/#review196596

::: dom/media/MediaDecoder.cpp:1395
(Diff revision 2)
> -    [reader, proxy]() {
> -    reader->SetCDMProxy(proxy);
> -    });
> -  mReader->OwnerThread()->Dispatch(r.forget());
> +              [self, proxy]() { return self->mReader->SetCDMProxy(proxy); })
> +    ->Then(mAbstractMainThread,
> +           __func__,
> +           [self](const MediaResult& aResult) {
> +             self->mSetCDMRequest.Complete();
> +             SetCDMPromise::CreateAndResolve(aResult, __func__);

Making MFR::SetCDMProxy() returning a promise means that it could be {resolved,rejected} later, of course it's executed on MFR's own thread.

Providing {resolve,reject} function here is to show the interim change, and it is moved to HTMLMediaElement in P3.

I will disconnect the request somewhere.

::: dom/media/MediaPromiseDefs.h:12
(Diff revision 2)
> +#define MediaPromiseDefs_h_
> +
> +#include "MediaResult.h"
> +#include "mozilla/MozPromise.h"
> +
> +namespace mozilla {

When I tried to return the promise from MFR to MediaDecoder then (through MediaDecoderOwner) to  HTMLMediaElement.

It would be simpler/cleaner if we have a separated file to define these promises, so that we don't have to include headers for those files which are supposed to be unrelated to each other.

And by collecting these promise definitions, it could be read/resued easily.
Comment on attachment 8913127 [details]
Bug 1395922 - [P2] Make MediaFormatReader::SetCDMProxy asynchronously with a promise.

https://reviewboard.mozilla.org/r/184552/#review195330

The goal is to make HTMLMediaElement::SetMediaKeys async. P2 is an interim implementation to make it possible.
Comment on attachment 8913127 [details]
Bug 1395922 - [P2] Make MediaFormatReader::SetCDMProxy asynchronously with a promise.

https://reviewboard.mozilla.org/r/184552/#review195330

> you never disconnect this request holder.
> 
> so why do you need one in the first place?

It is disconnected when shutdown now.

> what is this supposed to do ?
> 
> shouldn't you be returning that promise?
> 
> Why do you even bother with a promise here?
> you never use the result anywhere
> 
> all you care is that you call MFR::SetCDMProxy on its own thread.

That's useless, removed.

> shouldn't you handle that error in the caller then to do it again?
> 
> or cancel the current one and restart as I'm guessing the existing pending SetCDMProxy is no longer of interest.

ya, current existing promise will be canceled, a new one is created.
Comment on attachment 8913129 [details]
Bug 1395922 - [P4] Make MDSM enter buffering state when MediaKeys is removed and resume the playback if setting same MediaKeys back.

https://reviewboard.mozilla.org/r/184556/#review195314

> It seems mWaitingForCDM should be true if mCDMProxy is null and if mSetCDMForTracks contains aTrack? Why not write a helper function that computes that, so you don't have to ensure that mWaitingForCDM is consistent with that logic?
> 
> "Let the world be its own model". It's usually simpler to calculate some state based on it's inputs, rather than maintain state which is a function of inputs, as then you need to worry about keeping the state in sync with its inputs.

Thanks for the suggestions ! Addressed

> You've removed the assertion that aProxy is not null, but on Android you're dereferencing aProxy, which may be null.
> 
> It seems that if aProxy is null, you probably don't want mEMEPDM to be non-null?

There's no need to have this change anymore since I destroy the mPlatform when mCDMProxy is set to null.
Comment on attachment 8913127 [details]
Bug 1395922 - [P2] Make MediaFormatReader::SetCDMProxy asynchronously with a promise.

https://reviewboard.mozilla.org/r/184552/#review197358

::: dom/media/MediaDecoder.h:700
(Diff revision 3)
>  
>    bool mTelemetryReported;
>    const MediaContainerType mContainerType;
>    bool mCanPlayThrough = false;
> +
> +  MozPromiseRequestHolder<media::SetCDMPromise> mSetCDMRequest;

this promise request holder serve no purpose.

And you remove that code in the next patch anyway

::: dom/media/MediaDecoder.cpp:439
(Diff revision 3)
>    UnpinForSeek();
>  
>    // Unwatch all watch targets to prevent further notifications.
>    mWatchManager.Shutdown();
>  
> +  mSetCDMRequest.DisconnectIfExists();

unecessary

::: dom/media/MediaDecoder.cpp:1398
(Diff revision 3)
> -    "MediaFormatReader::SetCDMProxy",
> -    [reader, proxy]() {
> -    reader->SetCDMProxy(proxy);
> -    });
> -  mReader->OwnerThread()->Dispatch(r.forget());
> +              __func__,
> +              [self, proxy]() { return self->mReader->SetCDMProxy(proxy); })
> +    ->Then(
> +      mAbstractMainThread,
> +      __func__,
> +      [self](const MediaResult& aResult) { self->mSetCDMRequest.Complete(); },

unecessary

::: dom/media/MediaDecoder.cpp:1400
(Diff revision 3)
> -    reader->SetCDMProxy(proxy);
> -    });
> -  mReader->OwnerThread()->Dispatch(r.forget());
> +    ->Then(
> +      mAbstractMainThread,
> +      __func__,
> +      [self](const MediaResult& aResult) { self->mSetCDMRequest.Complete(); },
> +      [self](const MediaResult& aError) { self->mSetCDMRequest.Complete(); })
> +    ->Track(mSetCDMRequest);

there's no need to track anything, you don't even need to case about the promise returned by mReader->SetCDMProxy as you never use it.

::: dom/media/MediaFormatReader.cpp:1225
(Diff revision 3)
>    mNotifyDataArrivedPromise.DisconnectIfExists();
>    mMetadataPromise.RejectIfExists(NS_ERROR_DOM_MEDIA_CANCELED, __func__);
>    mSeekPromise.RejectIfExists(NS_ERROR_DOM_MEDIA_CANCELED, __func__);
>    mSkipRequest.DisconnectIfExists();
> +  mSetCDMPromise.RejectIfExists(
> +    MediaResult(NS_ERROR_DOM_INVALID_STATE_ERR,

NS_ERROR_DOM_MEDIA_CANCELED 

Why would that be an invalid state?

::: dom/media/MediaFormatReader.cpp:1330
(Diff revision 3)
> +  if (mSetCDMForTracks.contains(aTrack)) {
> +    MOZ_ASSERT(!mSetCDMPromise.IsEmpty());
> +
> +    mSetCDMForTracks -= aTrack;
> +    if (mSetCDMForTracks.isEmpty()) {
> +      mSetCDMPromise.Resolve(MediaResult(NS_OK), __func__);

Resolve(NS_OK, __FUNC__)

though the standard so far as we don't care whatsoever of the value with a resolved promise is to simply make it a boolean.

::: dom/media/MediaFormatReader.cpp:1343
(Diff revision 3)
> -  RefPtr<CDMProxy> proxy = aProxy;
> -  RefPtr<MediaFormatReader> self = this;
> -  nsCOMPtr<nsIRunnable> r =
> -    NS_NewRunnableFunction("MediaFormatReader::SetCDMProxy", [=]() {
> -      MOZ_ASSERT(self->OnTaskQueue());
> -      self->mCDMProxy = proxy;
> +  MOZ_ASSERT(OnTaskQueue());
> +  LOGV("SetCDMProxy (%p)", aProxy);
> +
> +  if (mShutdown) {
> +    return media::SetCDMPromise::CreateAndReject(
> +      MediaResult(NS_ERROR_DOM_INVALID_STATE_ERR,

again, why is that an invalid stat?

make it
NS_ERROR_DOM_MEDIA_CANCELED

::: dom/media/MediaFormatReader.cpp:1374
(Diff revision 3)
> +  mCDMProxy = aProxy;
> +
> +  if (!mInitDone || mSetCDMForTracks.isEmpty()) {
> +    // MFR is not initialized yet or demuxer is initialized without active
> +    // audio and video, the promise can be resolved directly.
> +    return media::SetCDMPromise::CreateAndResolve(MediaResult(NS_OK), __func__);

no media namespace

::: dom/media/MediaFormatReader.cpp:1374
(Diff revision 3)
> +  mCDMProxy = aProxy;
> +
> +  if (!mInitDone || mSetCDMForTracks.isEmpty()) {
> +    // MFR is not initialized yet or demuxer is initialized without active
> +    // audio and video, the promise can be resolved directly.
> +    return media::SetCDMPromise::CreateAndResolve(MediaResult(NS_OK), __func__);

Resolve(true, __func__)

::: dom/media/MediaPromiseDefs.h:14
(Diff revision 3)
> +#include "MediaResult.h"
> +#include "mozilla/MozPromise.h"
> +
> +namespace mozilla {
> +
> +namespace media {

don't use a namespace
Attachment #8913127 - Flags: review?(jyavenard) → review-
Comment on attachment 8913129 [details]
Bug 1395922 - [P4] Make MDSM enter buffering state when MediaKeys is removed and resume the playback if setting same MediaKeys back.

https://reviewboard.mozilla.org/r/184556/#review197366

will continue review when cpearce r+ it.

::: dom/media/MediaFormatReader.cpp:1365
(Diff revision 3)
>  
> +bool
> +MediaFormatReader::IsDecoderWaitingForCDM(TrackType aTrack)
> +{
> +  MOZ_ASSERT(OnTaskQueue());
> +  if (IsEncrypted() && mSetCDMForTracks.contains(aTrack) && !mCDMProxy) {

return IsEncrypted() && mSetCDMForTracks.contains(aTrack) && !mCDMProxy;
Comment on attachment 8913129 [details]
Bug 1395922 - [P4] Make MDSM enter buffering state when MediaKeys is removed and resume the playback if setting same MediaKeys back.

https://reviewboard.mozilla.org/r/184556/#review199726

::: dom/media/MediaFormatReader.cpp:1392
(Diff revision 3)
> -  }
> -
>    // Shutdown all decoders as switching CDM proxy indicates that it's
>    // inappropriate for the existing decoders to continue decoding via the old
>    // CDM proxy.
> -  if (!mSetCDMForTracks.isEmpty()) {
> +  PrepareToSetCDMForTrack(HasAudio(), TrackInfo::kAudioTrack);

I would go:

if (HasAudio()) {
  PrepareToSetCDMForTrack(TrackInfo::kAudioTrack);
}
if (HasVideo()) {...

That way you're only calling PrepareToSetCDMForTrack() if you need to prepare to set the CDM for a track.

I it's better that way, as the caller has more information as to whether we have the track.
Attachment #8913129 - Flags: review?(cpearce) → review+
Comment on attachment 8913129 [details]
Bug 1395922 - [P4] Make MDSM enter buffering state when MediaKeys is removed and resume the playback if setting same MediaKeys back.

https://reviewboard.mozilla.org/r/184556/#review199750
Attachment #8913129 - Flags: review?(jyavenard) → review+
Comment on attachment 8913127 [details]
Bug 1395922 - [P2] Make MediaFormatReader::SetCDMProxy asynchronously with a promise.

https://reviewboard.mozilla.org/r/184552/#review197358

> NS_ERROR_DOM_MEDIA_CANCELED 
> 
> Why would that be an invalid state?

According to the algorithm for SetMediaKeys in EME SPEC [1], if the old CDM cannot be removed or the new CDM cannot be associated, NotSupportedError, InvalidStateError or a DOMException[2] whose name is the appropriate name should be reported.

That's why I think using invalid state is more suitble.

[1] https://w3c.github.io/encrypted-media/#dom-htmlmediaelement-setmediakeys
[2] https://heycam.github.io/webidl/#dfn-DOMException

> again, why is that an invalid stat?
> 
> make it
> NS_ERROR_DOM_MEDIA_CANCELED

same reason as I mentioned above.
Comment on attachment 8913127 [details]
Bug 1395922 - [P2] Make MediaFormatReader::SetCDMProxy asynchronously with a promise.

https://reviewboard.mozilla.org/r/184552/#review200956

::: dom/media/MediaDecoder.h:14
(Diff revision 4)
>  
>  #include "DecoderDoctorDiagnostics.h"
>  #include "MediaDecoderOwner.h"
>  #include "MediaEventSource.h"
>  #include "MediaMetadataManager.h"
> +#include "MediaPromiseDefs.h"

this is unused... remove

::: dom/media/MediaDecoder.cpp:1389
(Diff revision 4)
>  {
>    MOZ_ASSERT(NS_IsMainThread());
> +
>    RefPtr<CDMProxy> proxy = aProxy;
> -  RefPtr<MediaFormatReader> reader = mReader;
> -  nsCOMPtr<nsIRunnable> r = NS_NewRunnableFunction(
> +  RefPtr<MediaDecoder> self = this;
> +  InvokeAsync(mReader->OwnerThread(), __func__, [self, proxy]() {

Don't use InvokeAsync

you have no use for promises here and InvokeAsync is rather heavy handed for such simple task.

So dispatching a task with a nsIRunnable is better

In fact the previous code will do just fine. No need to modify MediaDecoder.cpp nor MediaDecoder.h

Move all this to P4.

This change shouldn't be in P2 as there's no functional change
Attachment #8913127 - Flags: review?(jyavenard) → review+
Comment on attachment 8913127 [details]
Bug 1395922 - [P2] Make MediaFormatReader::SetCDMProxy asynchronously with a promise.

https://reviewboard.mozilla.org/r/184552/#review200956

> Don't use InvokeAsync
> 
> you have no use for promises here and InvokeAsync is rather heavy handed for such simple task.
> 
> So dispatching a task with a nsIRunnable is better
> 
> In fact the previous code will do just fine. No need to modify MediaDecoder.cpp nor MediaDecoder.h
> 
> Move all this to P4.
> 
> This change shouldn't be in P2 as there's no functional change

I meant P3 sorry
Comment on attachment 8913128 [details]
Bug 1395922 - [P3] Make HTMLMediaElement::SetMediaKeys asynchronously.

https://reviewboard.mozilla.org/r/184554/#review200964

::: dom/media/MediaDecoder.cpp:1389
(Diff revision 4)
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>  
>    RefPtr<CDMProxy> proxy = aProxy;
>    RefPtr<MediaDecoder> self = this;
> -  InvokeAsync(mReader->OwnerThread(), __func__, [self, proxy]() {
> +  return InvokeAsync(mReader->OwnerThread(), __func__, [self, proxy]() {

return InvokeAsync(
         mReader->OwnerThread(), this, __func__,
         &MediaFormatReader::SetCDMProxy, aProxy);
         
No need for a lambda and capturing variables.
(though I don't recall if the argument passed to InvokeAsync can be a raw pointer and it will automatically store it in a RefPtr like nsIRunnable does.
Comment on attachment 8913126 [details]
Bug 1395922 - [P1] Refactor code and move them into specific functions.

https://reviewboard.mozilla.org/r/184548/#review201286

Thanks for these review.
Comment on attachment 8913126 [details]
Bug 1395922 - [P1] Refactor code and move them into specific functions.

https://reviewboard.mozilla.org/r/184548/#review201294

Rebase patches.
Comment on attachment 8913127 [details]
Bug 1395922 - [P2] Make MediaFormatReader::SetCDMProxy asynchronously with a promise.

https://reviewboard.mozilla.org/r/184552/#review201350

::: dom/media/MediaDecoder.cpp:1434
(Diff revision 6)
>    MOZ_ASSERT(NS_IsMainThread());
> +
>    RefPtr<CDMProxy> proxy = aProxy;
>    RefPtr<MediaFormatReader> reader = mReader;
> -  nsCOMPtr<nsIRunnable> r = NS_NewRunnableFunction(
> -    "MediaFormatReader::SetCDMProxy",
> +  nsCOMPtr<nsIRunnable> r =
> +    NS_NewRunnableFunction("MediaFormatReader::SetCDMProxy",

please remove this change.. this is just style change for a patch that should only touch MediaFormatReader
Pushed by kikuo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bacda0f99f71
[P1] Refactor code and move them into specific functions. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/f786d928b1e0
[P2] Make MediaFormatReader::SetCDMProxy asynchronously with a promise. r=cpearce,jya
https://hg.mozilla.org/integration/autoland/rev/d46f952f94f8
[P3] Make HTMLMediaElement::SetMediaKeys asynchronously. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/9cd31c6a8e2c
[P4] Make MDSM enter buffering state when MediaKeys is removed and resume the playback if setting same MediaKeys back. r=cpearce,jya
https://hg.mozilla.org/integration/autoland/rev/f59a7e727f39
[P5][Test] Capture clear content with canvas 2d after remove MediaKeys from the same HTMLMediaElement. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/f856af63682e
[P6][Test] Playback should resume after reattaching same MediaKeys with valid sessions. r=cpearce
Backed out for failing Media Tests test_eme_sample_groups_playback.html and test_eme_sample_groups_playback.html

https://bugzilla.mozilla.org/show_bug.cgi?id=1414462
https://bugzilla.mozilla.org/show_bug.cgi?id=1414463

Backout: https://hg.mozilla.org/mozilla-central/rev/8b9167b8a937e09f6673d4d7d693c6ac1c25a3d7
Status: RESOLVED → REOPENED
Flags: needinfo?(kikuo)
Resolution: FIXED → ---
Target Milestone: mozilla58 → ---
Depends on: 1415028
(In reply to nataliaCs from comment #89)
> Backed out for failing Media Tests test_eme_sample_groups_playback.html and
> test_eme_sample_groups_playback.html
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=1414462
> https://bugzilla.mozilla.org/show_bug.cgi?id=1414463
> 
> Backout:
> https://hg.mozilla.org/mozilla-central/rev/
> 8b9167b8a937e09f6673d4d7d693c6ac1c25a3d7

The landed patches should be innocent but just make this intermittent appear easier, because

1) MediaKeys is now able to be replaced after load started, so there's chance that a media data request may be rejected because MFR is replacing cdm proxy. That's the error case in Bug 1414462, a 2nd(unnecessary) call to |LoadEME()| should be removed.
2) A "license-request" log may be logged after Simple.finish(), because the session creation in the 2nd(unnecessary) call to |LoadEME()| may sometimes finish after the end of playback, we should check why sometimes the GMP IPC message takes so long here and there's an old issue Bug 1342336 which is identical to Bug 1414463.

Anyway, I'll re-land this bug after Bug 1415028 is fixed.
Flags: needinfo?(kikuo)
Comment on attachment 8913126 [details]
Bug 1395922 - [P1] Refactor code and move them into specific functions.

https://reviewboard.mozilla.org/r/184548/#review202180

Rebase code and make these patches be based on Bug 1415028.
Pushed by kikuo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8c1397fea2dd
[P1] Refactor code and move them into specific functions. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/2849269a64be
[P2] Make MediaFormatReader::SetCDMProxy asynchronously with a promise. r=cpearce,jya
https://hg.mozilla.org/integration/autoland/rev/d013a75c11c8
[P3] Make HTMLMediaElement::SetMediaKeys asynchronously. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/cf1f507a4dcc
[P4] Make MDSM enter buffering state when MediaKeys is removed and resume the playback if setting same MediaKeys back. r=cpearce,jya
https://hg.mozilla.org/integration/autoland/rev/3adbfd979a27
[P5][Test] Capture clear content with canvas 2d after remove MediaKeys from the same HTMLMediaElement. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/c432ab6709bc
[P6][Test] Playback should resume after reattaching same MediaKeys with valid sessions. r=cpearce
Depends on: 1482116
You need to log in before you can comment on or make changes to this bug.