Closed Bug 1144409 Opened 5 years ago Closed 5 years ago

encrypted event should be fired once per initData

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: gerald, Assigned: gerald)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files, 6 obsolete files)

Bug 1134434 handles encrypted Initialization Data in a separate task, so as not to block loadedmetadata.

However, in the case of a media source with two sourceBuffers, each receiving an init segment at about the same time, the Initialization Data from both streams get treated together and this results in only one 'encrypted' event being fired, with the combined keys from both streams.

From the specs:
"The following steps are run when the media element encounters Initialization Data in the media data during the resource fetch algorithm:[...]
    Queue a task to fire a simple event named encrypted at the media element."

This suggests that each occurrence of Initialization Data should produce one distinct encrypted event with the corresponding initData.
Note: Not only should multiple 'encrypted' events be fired (if necessary) after the initial metadata read, further Initialization Data arriving during play (e.g. when switching resolution) require additional 'encrypted' events.
Blocks: 1138294
This 1st patch keeps different init data separate and sends them in discrete 'encrypted' events from HTMLMediaElement::MetadataLoaded().
Attachment #8579775 - Flags: review?(cpearce)
This 2nd patch tries and dispatches 'encrypted' events when encountered, but if the readyState is HAVE_NOTHING, the events are queued until HTMLMediaElement::MetadataLoaded transitions readyState to HAVE_METADATA and can fire all queued events. This ensures that future init data encounters results in new 'encrypted' events.
Attachment #8579776 - Flags: review?(cpearce)
Note: Tests for these patches are in bug 1142379 and bug 1138294 (as they use multiple keys and also change keys mid-stream).
Blocks: 1145011
Comment on attachment 8579775 [details] [diff] [review]
1144409-p1-multiple-encrypted-on-metadata-loaded.patch

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

::: dom/media/MediaInfo.h
@@ +121,5 @@
> +  }
> +
> +  void AddInitData(const nsString& aType, nsTArray<uint8_t>&& aInitData)
> +  {
> +    InitData* data = mInitDatas.AppendElement();

I would write a constructor for InitData(type, initData), and then do:

mInitDatas.AppendElement(InitData(aType, aInitData))
Attachment #8579775 - Flags: review?(cpearce) → review+
Attachment #8579776 - Flags: review?(cpearce) → review+
Updated patch as per comment 5, carrying r+.
Attachment #8579775 - Attachment is obsolete: true
Attachment #8581445 - Flags: review+
Updated patch as per comment 5, carrying r+.
Attachment #8579776 - Attachment is obsolete: true
Attachment #8581446 - Flags: review+
I had to back these out for the various mochitest-3 failures that showed up after these landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/dd850ffd05bc
https://hg.mozilla.org/integration/mozilla-inbound/rev/7167a20b3bf8
Flags: needinfo?(gsquelart)
This bug doesn't need to block Adobe MVP.
Priority: -- → P2
I will dissociate this patch from the other bugs (bug 1142379 and bug 1138294), which test much more than what this patch actually does.
I'm preparing a smaller test focused on this bug, i.e., verifying the number of encrypted events generated: https://treeherder.mozilla.org/#/jobs?repo=try&revision=09023612e6d7
Flags: needinfo?(gsquelart)
Combined p1 and p2 patches, only difference is removing a couple of #ifdef MOZ_EME so that 'encrypted' is also fired on non-EME builds.
Carrying r+.
Attachment #8581445 - Attachment is obsolete: true
Attachment #8581446 - Attachment is obsolete: true
Attachment #8584235 - Flags: review+
Updated EME playback test to count number of encrypted events.
Attachment #8584236 - Flags: review?(edwin)
Blocks: 1148179
Backed out for B2G Emulator-L bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/cffa0a89f2f8

15:11:13    ERROR -  ../../dist/include/mozilla/dom/HTMLMediaElement.h:562:8: error: 'void mozilla::dom::HTMLMediaElement::DispatchEncrypted(const nsTArray<unsigned char>&, const nsAString_internal&)' marked override, but does not override
15:11:13     INFO -     void DispatchEncrypted(const nsTArray<uint8_t>& aInitData,
15:11:13     INFO -          ^
Fixed B2G build, missed removing one #ifdef MOZ_EME in a base class. Carrying r+.
Attachment #8584235 - Attachment is obsolete: true
Attachment #8586436 - Flags: review+
Proper #ifdef MOZ_EME fix, now tested on all platforms:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c21cd9a885ad

No actual code change, carrying r+.
Attachment #8586436 - Attachment is obsolete: true
Attachment #8586696 - Flags: review+
Priority: P2 → P1
https://hg.mozilla.org/mozilla-central/rev/88edb1af4e14
https://hg.mozilla.org/mozilla-central/rev/890c0d707e35
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.