1134434 - [EME] ReadMetadata should be independent from EME keys

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Yury pointed out that loadedmetadata not firing when we have encrypted media could be confusing for devs, and that proper layout can depend on loadedmetadata firing.

The EME spec agrees:
"encrypted" event precondition:
 - The element's readyState is equal to HAVE_METADATA or greater.

In ReadMetadata, we might be able to pass a Promise<CDMProxy> into CreateCDMWrapper instead of a CDMProxy* and block decryption on that promise resolving (which should also fire off a "waitingforkey" event, which we don't currently do).

Comment 1

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: 1134434.patch

Finally getting around to uploading these.

This patch just splits MP4Reader::ReadMetadata into two parts so as to not block the loadedmetadata event from happening. Also makes the HTMLMediaElement responsible for firing the encrypted event.

Comment 2

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: 1134434-fix-test.patch

Remove test_eme_access_control -- only tests CORS on non-MSE EME, which is breaks after the above patch.

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8575750 [details] [diff] [review]
1134434.patch

Review of attachment 8575750 [details] [diff] [review]:
-----------------------------------------------------------------

We should just do FinishDecoderSetup inside a EnsureDecodersSetup() function inside Request*Data().

And I think you should get a MSE person to look over the changes to MSE code.

::: dom/media/MediaDecoderReader.h
@@ +161,5 @@
>    // Fills aInfo with the latest cached data required to present the media,
>    // ReadUpdatedMetadata will always be called once ReadMetadata has succeeded.
>    virtual void ReadUpdatedMetadata(MediaInfo* aInfo) { };
>  
> +  virtual nsresult FinishDecoderSetup() { return NS_OK; };

So now MediaDecoderReader has the following functions related to metadata reading:

PreReadMetadata(), ReadMetadata(), ReadUpdatedMetadata(), and now FinishDecoderSetup().

That's rather unfortunate.

Can't you just do what MP4Reader::FinishDecoderSetup() does inside the first call to Request*Data() if it's not been done yet?

::: dom/media/MediaDecoderStateMachine.cpp
@@ +2261,5 @@
>      // Inform the element that we've loaded the metadata.
>      EnqueueLoadedMetadataEvent();
>    }
>  
> +  if (mState == DECODER_STATE_DECODING_METADATA &&

You could merge this into the "if (mState == DECODER_STATE_DECODING_METADATA)" block right below so you don't check mState twice.

@@ +2333,5 @@
>    DECODER_LOG("DecodeFirstFrame started");
>  
> +  {
> +    ReentrantMonitorAutoExit mon(mDecoder->GetReentrantMonitor());
> +    nsresult res = mReader->FinishDecoderSetup();

You're calling this on the state machine thread, we should try to limit calls the reader on the decode thread if at all possible, especially for non-trivial things like this. All the more reason to do this inside the Request*Data() functions.

Comment 4

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

(In reply to Chris Pearce (:cpearce) from comment #3)
> We should just do FinishDecoderSetup inside a EnsureDecodersSetup() function
> inside Request*Data().

Hrm, you're right. That'd be much simpler. New patch soon.

Comment 5

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: 1134434.patch v2

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8576952 [details] [diff] [review]
1134434.patch v2

Review of attachment 8576952 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with nits.

::: dom/media/fmp4/MP4Reader.cpp
@@ +446,5 @@
>  
> +bool
> +MP4Reader::EnsureDecodersSetup()
> +{
> +  MonitorAutoLock mon(mDecoderSetupMonitor);

You shouldn't need the monitor, this should only be called on the decode task queue, so just assert that this is only called on the decode task queue:
MOZ_ASSERT(GetTaskQueue()->IsCurrentThreadIn());

::: dom/media/mediasource/MediaSourceReader.cpp
@@ +1011,5 @@
> +  aTo.mIsEncrypted = true;
> +
> +  // We assume here that the encryption type will be the same between the two
> +  // streams.
> +  aTo.mType = aFrom.mType;

You should fail if the type is not the same, with a NS_WARNING at least.

Comment 7

[Gerald Squelart (he/him)]

Attachment: 1134434.patch v2 rebased

Rebase of "1134434.patch v2", still needs work as per comment 6.

Comment 8

[Gerald Squelart (he/him)]

Attachment: 1134434-fix-test.patch rebased

Rebase of "1134434-fix-test.patch".

Comment 9

[Gerald Squelart (he/him)]

Attachment: 1134434.patch v2 rebased again

Another rebase.

Comment 10

[Gerald Squelart (he/him)]

Attachment: 1134434.patch v2 rebased again*2

Rebased again, previous could not build.
*Note* this is probably a dumb rebase, will need more work!

Comment 11

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b7e5ddf82a1a
https://hg.mozilla.org/integration/mozilla-inbound/rev/706ee8458ffb

Comment 12

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

+ Bustage-B-Gone

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

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b7e5ddf82a1a
https://hg.mozilla.org/mozilla-central/rev/706ee8458ffb
https://hg.mozilla.org/mozilla-central/rev/f99eb0b983d6