1127416 - Fire an observer service notification when metadata loads for an EME media element

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Comment 1

[:Gijs (he/him)]

Attachment: add observer service notification for EME video,

So after a bit of poking about I wrote this test... but it fails. The video that gets passed as the subject is not the same as |v|. However, that *is* the case for the loadedmetadata event itself, which also passes |this| (static_casted to nsIContent*). I... don't really understand how that is possible, unless there's basically an encrypted and an unencrypted video element going around that somehow point to each other or something? I did notice that |v|'s currentSrc pointed to the .sjs? version of the video being played, with the one passed as |subject| pointing to what I'm assuming is the resolved/redirected/actual mp4 file.
Finally, note that this isn't actually important for desktop's usecase here, we just need the owner document of the video to make sense so we can find the right tab and whatnot... but it's still odd.

Comment 2

[Justin Dolske [:Dolske]]

It sounds like the UI string we're converging on in bug 1095734 will want to be able to refer to the specific CDM being used by name. Is that simple enough to do here, or should that be extended in a followup? (I assume the latter, since the infra from bug 1089867 hasn't landed yet.)

Comment 3

[:Gijs (he/him)]

(In reply to Justin Dolske [:Dolske] from comment #2)
> It sounds like the UI string we're converging on in bug 1095734 will want to
> be able to refer to the specific CDM being used by name. Is that simple
> enough to do here, or should that be extended in a followup? (I assume the
> latter, since the infra from bug 1089867 hasn't landed yet.)

It depends; right now I still need to figure out why the right video element isn't being passed, but once I have that sorted, it'd be ready. If 1089867 lands in the meantime, I assume it wouldn't be hard to set up the CDM name as the last parameter of the observer notification.

Comment 4

[:Gijs (he/him)]

(equally, if the element exposes this info in some way then the browser code could also deduce it from the video element itself)

Comment 5

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

Comment on attachment 8557079 [details] [diff] [review]
add observer service notification for EME video,

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

::: dom/media/test/test_eme_obs_notification.html
@@ +20,5 @@
> +  var notifier = function(subject) {
> +    is(subject, v, "Should get passed the video object in question");
> +    gotNotified = subject;
> +  };
> +  SpecialPowers.Services.obs.addObserver(notifier, "media-eme-metadataloaded", false);

One of the main things that MediaTestManager does is it runs multiple tests in parallel. Two currently. So you'll have two observers here with references to video elements baked in. So since we'll run two test cases concurrently here, you'll get these observers firing pretty close to each other, and that would be why you're seeing the "wrong" video element in the observer.

Your observer needs to operate on static data, or at least test that it only responds to notifications for the video element it belongs to.

Comment 6

[:Gijs (he/him)]

Attachment: add observer service notification for EME video,

Ah, thanks, that helped. This passes, then. :-)

Comment 7

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

Comment on attachment 8557613 [details] [diff] [review]
add observer service notification for EME video,

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

Looks good!

Comment 8

[:Gijs (he/him)]

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1adce33c6da

Comment 9

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #8)
> remote:  
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1adce33c6da

This is failing on try:

https://treeherder.mozilla.org/logviewer.html#?job_id=4618760&repo=try

I expect this is because I got rid of the waitForExplicitFinish because I noticed that the media manager requests the same... I now suspect that I still need it even in the test files (it is also in the other EME tests; I'd just thought that was an oversight).

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=2eabfbe18cfc

(the bc3 failures in the previous push are because of my tree not having the test fix I pushed shortly after I pushed the fix for bug 1125378 - I've repushed against tip now)

Comment 10

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #9)
> remote:  
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=2eabfbe18cfc

Green!

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d46487a3cd1d

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d46487a3cd1d

Comment 12

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

I'm going to say that since we're not using this, we don't need to uplift it for the EME 37 uplift (and I will ignore the change to the test added in this bug when I uplift the patch in bug 1131392).