Closed Bug 1134434 Opened 8 years ago Closed 8 years ago

[EME] ReadMetadata should be independent from EME keys


(Core :: Audio/Video, defect)

Not set



Tracking Status
firefox39 --- fixed


(Reporter: eflores, Assigned: eflores)




(2 files, 5 obsolete files)

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).
Attached patch 1134434.patch (obsolete) — Splinter Review
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.
Attachment #8575750 - Flags: review?(cpearce)
Attached patch 1134434-fix-test.patch (obsolete) — Splinter Review
Remove test_eme_access_control -- only tests CORS on non-MSE EME, which is breaks after the above patch.
Attachment #8575751 - Flags: review?(cpearce)
Comment on attachment 8575750 [details] [diff] [review]

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

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.
Attachment #8575750 - Flags: review?(cpearce) → review-
Attachment #8575751 - Flags: review?(cpearce) → review+
(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.
Attached patch 1134434.patch v2 (obsolete) — Splinter Review
Attachment #8575750 - Attachment is obsolete: true
Attachment #8576952 - Flags: review?(cpearce)
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:

::: 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.
Attachment #8576952 - Flags: review?(cpearce) → review+
Blocks: 1144409
Attached patch 1134434.patch v2 rebased (obsolete) — Splinter Review
Rebase of "1134434.patch v2", still needs work as per comment 6.
Attachment #8576952 - Attachment is obsolete: true
Rebase of "1134434-fix-test.patch".
Attachment #8575751 - Attachment is obsolete: true
Blocks: 1145011
Attached patch 1134434.patch v2 rebased again (obsolete) — Splinter Review
Another rebase.
Attachment #8579771 - Attachment is obsolete: true
Rebased again, previous could not build.
*Note* this is probably a dumb rebase, will need more work!
Attachment #8581199 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.