1144409 - encrypted event should be fired once per initData

Status: RESOLVED FIXED

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

Description

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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.

Comment 1

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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.

Comment 2

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1144409-p1-multiple-encrypted-on-metadata-loaded.patch

This 1st patch keeps different init data separate and sends them in discrete 'encrypted' events from HTMLMediaElement::MetadataLoaded().

Comment 3

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1144409-p2-encrypted-on-future-initdata.patch

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.

Comment 4

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Note: Tests for these patches are in bug 1142379 and bug 1138294 (as they use multiple keys and also change keys mid-stream).

Comment 5

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

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))

Comment 6

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1144409-p1-multiple-encrypted-on-metadata-loaded.patch

Updated patch as per comment 5, carrying r+.

Comment 7

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1144409-p2-encrypted-on-future-initdata.patch

Updated patch as per comment 5, carrying r+.

Comment 8

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

checkin-needed please.
Tests in bug 1142379 and bug 1138294.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f902c54a371

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f3b3547c610a
https://hg.mozilla.org/integration/mozilla-inbound/rev/6cb38e697216

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 11

[Chris Peterson [:cpeterson]]

This bug doesn't need to block Adobe MVP.

Comment 12

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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

Comment 13

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1144409-one-encrypted-per-initdata.patch

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+.

Comment 14

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1144409-test-one-encrypted-per-initdata.patch

Updated EME playback test to count number of encrypted events.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/95866bc69415
https://hg.mozilla.org/integration/mozilla-inbound/rev/13c7176938cd

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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 -          ^

Comment 17

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1144409-one-encrypted-per-initdata.patch

Fixed B2G build, missed removing one #ifdef MOZ_EME in a base class. Carrying r+.

Comment 18

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1144409-one-encrypted-per-initdata.patch

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+.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/88edb1af4e14
https://hg.mozilla.org/integration/mozilla-inbound/rev/890c0d707e35

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/88edb1af4e14
https://hg.mozilla.org/mozilla-central/rev/890c0d707e35