Closed Bug 1145011 Opened 9 years ago Closed 8 years ago

Implement waitingForKey event

Categories

(Core :: Audio/Video: Playback, defect, P3)

Product:
Core
Component:
Audio/Video: Playback
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox51 --- fixed

People

(Reporter: mozbugz, Assigned: bryce)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

1145011-waitingForKey-WIP.patch
19.51 KB, patch
Details | Diff | Splinter Review
Bug 1145011 - Implement waitingforkey event.
58 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1145011 - Add test for waitingforkey.
58 bytes, text/x-review-board-request
jya
: review+
Details 
http://w3c.github.io/encrypted-media/#dom-evt-waitingforkey

HTMLMediaElement event 'waitingForKey'; dispatched when: Playback is blocked waiting for a key; preconditions: The element is potentially playing and its readyState is equal to HAVE_FUTURE_DATA or greater.

Algorithm: http://w3c.github.io/encrypted-media/#queue-waitingforkey
Component: Audio/Video → Audio/Video: Playback
The lack of 'waitingforkey' is now affecting integration tests on Shaka Player.  For now, we have to disable the affected tests completely on Firefox.  (Since there is no 'onwaitingforkey' attribute on the video element, we can't do feature detection and just have to test the UA for Firefox.)

Any update on when this event will be added to Firefox's EME implementation?
We're doing a round of EME spec compliance work at the moment. Not sure on a specific timeframe. Probably Firefox 51 or 52.
Priority: -- → P2
Thanks, Chris!
Happy to work on it when planning calls for it.
Reassigning to "nobody" in case someone else wants to take over in the meantime.
Assignee: gsquelart → nobody
In case that helps, this is my WIP patch from 26 March 2015, it will probably require heavy rebasing.
It looks fairly complete, it even has a mochitest; Not sure why I didn't complete it.
P3 is the new P2
Priority: P2 → P3
Assignee: nobody → bvandyk
(In reply to Gerald Squelart [:gerald] from comment #0)
> HTMLMediaElement event 'waitingForKey'; dispatched when: Playback is blocked
> waiting for a key; preconditions: The element is potentially playing and its
> readyState is equal to HAVE_FUTURE_DATA or greater.

The precondition is suspicious to me. How can readyState reach HAVE_FUTURE_DATA if the media element has no keys to decoded at all?
(In reply to JW Wang [:jwwang] from comment #8)
> The precondition is suspicious to me. How can readyState reach
> HAVE_FUTURE_DATA if the media element has no keys to decoded at all?

I think this is that the spec anticipates that you will move into HAVE_FUTURE_DATA from having encrypted data buffered, at which stage the waitingForKey algo will kick in if you don't have keys and move you back into HAVE_METADATA. In our case we won't move into HAVE_FUTURE_DATA in the first place if we haven't decrypted the samples.
Other browsers don't base their readyState on whether they have decoded data, they base their readyState on how much data is downloaded.
I think there is inconsistency  in the spec.

HAVE_METADATA is defined as:
No media data is available for the immediate current playback position.

However, the waiting-for-key algorithm 7.3.4.5 sets the readyState of media element to HAVE_METADATA when there is media data but no keys to decrypt for the current playback position.
(In reply to JW Wang [:jwwang] from comment #8)
> (In reply to Gerald Squelart [:gerald] from comment #0)
> > HTMLMediaElement event 'waitingForKey'; dispatched when: Playback is blocked
> > waiting for a key; preconditions: The element is potentially playing and its
> > readyState is equal to HAVE_FUTURE_DATA or greater.
> 
> The precondition is suspicious to me. How can readyState reach
> HAVE_FUTURE_DATA if the media element has no keys to decoded at all?

With MSE, the readyState is determined based on the buffered range, not based on what the MDSM has already decoded.

There's no exception made if the frames are actually un-decodable due to waiting for keys.

This is something that needs to be modified. Maybe only use the buffered range with MSE *if* it's not encrypted, or if it's encrypted and we have all keys.
Comment on attachment 8781322 [details]
Bug 1145011 - Implement waitingforkey event.

https://reviewboard.mozilla.org/r/71734/#review69736

this can be greatly simplified, with no need for task dispatching.

Using a callback and the handling in the MediaFormatReader looks great to me otherwise.

::: dom/html/HTMLMediaElement.cpp:4464
(Diff revision 1)
>    CheckAutoplayDataReady();
>  
>    if (oldState < nsIDOMHTMLMediaElement::HAVE_FUTURE_DATA &&
>        mReadyState >= nsIDOMHTMLMediaElement::HAVE_FUTURE_DATA &&
>        IsPotentiallyPlaying()) {
> +    mWaitingForKey = false;

Please use an MediaEvent instead, with a callback that would set mWaitingForKey.

I would assume that the MediaDecoder hold the MediaEventProducer.

::: dom/html/HTMLMediaElement.cpp:5867
(Diff revision 1)
> +  //   "CannotDecryptWaitingForKey should happen when IsPotentiallyPlaying");
> +  // NS_ASSERTION(mReadyState >= HAVE_FUTURE_DATA,
> +  //   "CannotDecryptWaitingForKey should happen when readyState >= HAVE_FUTURE_DATA");
> +
> +  // 2. If the media element's waiting for key value is true, abort these steps.
> +  if (!mWaitingForKey) {

Using MediaEvent, you would likely not even need a mWaitingForKey member.

when the MediaEvent is notified, the waitingforkey is fired automatically.

you don't need to worry about when the mWaitingForKey is going back to true as this is implicit when playback resume.

::: dom/media/MediaDecoderOwner.h:162
(Diff revision 1)
>  #endif // MOZ_EME
> +
> +  // Called by the EME decoder when a sample cannot be decrypted because of
> +  // missing keys.
> +  // Main thread only.
> +  virtual void CannotDecryptWaitingForKey() = 0;

I don't believe this would need to be virtual, and the function called when a key is needed could be made private.

::: dom/media/MediaFormatReader.cpp:1530
(Diff revision 1)
> +  {
> +  }
> +  NS_IMETHOD Run() {
> +    // Note: Null check the owner, as the decoder could have been shutdown
> +    // since this event was dispatched.
> +    MediaDecoderOwner* owner = mDecoder->GetOwner();

Back to using MediaEvents, you would simply notify one, the machinery behind it would do the rest.

no need to dispatch a task, it's all automatic

::: dom/media/platforms/PlatformDecoderModule.h:188
(Diff revision 1)
>  
>    virtual bool OnReaderTaskQueue() = 0;
> +
> +  // Denotes that a pending encryption key is preventing more input being fed
> +  // into the decoder.
> +  virtual void WaitingForKey() = 0;

don't make it a pure virtual as for most PDM it's just not required at all.

Only the EME and GMP ones care about it
Attachment #8781322 - Flags: review?(jyavenard)
Comment on attachment 8781322 [details]
Bug 1145011 - Implement waitingforkey event.

https://reviewboard.mozilla.org/r/71734/#review69736

> Using MediaEvent, you would likely not even need a mWaitingForKey member.
> 
> when the MediaEvent is notified, the waitingforkey is fired automatically.
> 
> you don't need to worry about when the mWaitingForKey is going back to true as this is implicit when playback resume.

I've left the member in so that we can do this check to avoid emitting multiple waitingForKey events during a single wait.
Comment on attachment 8781322 [details]
Bug 1145011 - Implement waitingforkey event.

https://reviewboard.mozilla.org/r/71734/#review70914

Please split the code from the tests... thank you

::: dom/media/AbstractMediaDecoder.h:81
(Diff revision 2)
> +  // further output. This only needs to be overridden for decoders that expect
> +  // encryption, such as the MediaSource decoder.
> +  virtual void NotifyWaitingForKey() {}
> +
> +  // Return an event that will be notified when a decoder is waiting for a
> +  // decryption key before it can emit more output.

s/emit/return

::: dom/media/platforms/PlatformDecoderModule.h:188
(Diff revision 2)
>  
>    virtual bool OnReaderTaskQueue() = 0;
> +
> +  // Denotes that a pending encryption key is preventing more input being fed
> +  // into the decoder. This only needs to be overridden for callbacks that
> +  // handle encryption. E.g. benchmarking does not use eme, so this need

that last sentence is unecessary I think

::: dom/media/test/test_eme_waitingforkey.html:1
(Diff revision 2)
> +<!DOCTYPE HTML>

Please split this patch in two.

One for the implementation and the other for the JS tests.

One patch == one scope
Attachment #8781322 - Flags: review?(jyavenard) → review+
Comment on attachment 8781322 [details]
Bug 1145011 - Implement waitingforkey event.

https://reviewboard.mozilla.org/r/71734/#review70914

Spit. Could you please review the split tests (no code changes, just split) so I can autoland?
Comment on attachment 8783339 [details]
Bug 1145011 - Add test for waitingforkey.

https://reviewboard.mozilla.org/r/73180/#review71212
Attachment #8783339 - Flags: review?(jyavenard) → review+
https://hg.mozilla.org/mozilla-central/rev/9ab3299e54c4
https://hg.mozilla.org/mozilla-central/rev/3c5392487166
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: