[EME] Playback should *stop* when we're waiting for key

RESOLVED FIXED in Firefox 52

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: cpearce, Assigned: jya)

Tracking

(Blocks: 1 bug)

unspecified
mozilla52
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox52 fixed)

Details

(URL)

Attachments

(9 attachments, 1 obsolete attachment)

58 bytes, text/x-review-board-request
cpearce
: review+
Details
58 bytes, text/x-review-board-request
cpearce
: review+
Details
58 bytes, text/x-review-board-request
jwwang
: review+
Details
58 bytes, text/x-review-board-request
jwwang
: review+
Details
58 bytes, text/x-review-board-request
cpearce
: review+
Details
58 bytes, text/x-review-board-request
cpearce
: review+
Details
58 bytes, text/x-review-board-request
jwwang
: review+
Details
6.52 KB, patch
Details | Diff | Splinter Review
58 bytes, text/x-review-board-request
jya
: review+
Details
Our waiting for key implementation doesn't stop playback when we lack a key. We're required to by the spec.

If the video stream is starts to wait for a key, but the audio stream doesn't need to wait for a key, the audio stream keeps playing. So current time keeps advancing. And then we fail the drm-mp4-playback-temporary-multikey-sequential-readyState.html web platform test.

Our existing waiting for key implementation just makes the audio or video request take longer to complete, and while that is happening it fires off a notification to the Media element. If the audio stream keeps producing samples during this time, we'll keep playing.

What we probably need to do instead is when we detect that we're waiting for a key, we should reject the Request{Video,Audio}Data promise with a "waiting for key" error code. Then the MFR/MDSM can follow a similar path to the waiting for data response. While waiting the MDSM will need to suspend playback at the end of the currently data which is decoded for the stream which is waiting for key.
(Reporter)

Comment 1

2 years ago
JW, JYA: what do you think?
Flags: needinfo?(jyavenard)
Flags: needinfo?(jwwang)
(Assignee)

Comment 2

2 years ago
SGTM
We also could hijack the existing "WAITING" promise mechanism instead.
This is currently used by MSE when it's missing data.
Currently this condition is only handled via the demuxer, but it could be extended to the decoder result too.
So MDSM calls RequestVideoData, the decoder returns WAITING, the VideoPromise is rejected with WAITING_FOR_DATA and the MDSM then calls the MFR::WaitForData to retrieve the waiting promise.
Promise that will get resolved once new data arrived (MSE) or the key is now available (EME)

that's assuming the EME decoder can tell the MFR it's waiting for a key though, I don't believe there's such mechanism in place yet.

When there's a pending waiting promise, the MDSM will stall like you want
Flags: needinfo?(jyavenard)
Sounds reasonable.

It is easy to change the buffering criteria of MDSM so it can enter buffering (thus stopping playback) once WAITING_FOR_DATA is received.
Flags: needinfo?(jwwang)
(In reply to Jean-Yves Avenard [:jya] from comment #2)
> We also could hijack the existing "WAITING" promise mechanism instead.
> This is currently used by MSE when it's missing data.
> Currently this condition is only handled via the demuxer, but it could be
> extended to the decoder result too.
Can you open a new bug to do that?
Flags: needinfo?(jyavenard)
(In reply to JW Wang [:jwwang] [:jw_wang] from comment #3)
> Sounds reasonable.
> 
> It is easy to change the buffering criteria of MDSM so it can enter
> buffering (thus stopping playback) once WAITING_FOR_DATA is received.

Btw, I think we will break some mediasource tests if MDSM enters buffering as soon as WAITING_FOR_DATA is received. We should have a new error code such as WAITING_FOR_KEY instead.
(Assignee)

Comment 6

2 years ago
(In reply to JW Wang [:jwwang] [:jw_wang] from comment #5)
> (In reply to JW Wang [:jwwang] [:jw_wang] from comment #3)
> > Sounds reasonable.
> > 
> > It is easy to change the buffering criteria of MDSM so it can enter
> > buffering (thus stopping playback) once WAITING_FOR_DATA is received.
> 
> Btw, I think we will break some mediasource tests if MDSM enters buffering
> as soon as WAITING_FOR_DATA is received. We should have a new error code
> such as WAITING_FOR_KEY instead.

I thought the MDSM enter buffering mode only once its queue is empty?
this is what we want for EME too.

I'm fairly sure there will be nothing to do on the MDSM side if the MFR returns WAITING_FOR_DATA when waiting for keys
Flags: needinfo?(jyavenard)
(In reply to Jean-Yves Avenard [:jya] from comment #6)
> I thought the MDSM enter buffering mode only once its queue is empty?
> this is what we want for EME too.
> 
> I'm fairly sure there will be nothing to do on the MDSM side if the MFR
> returns WAITING_FOR_DATA when waiting for keys

Per comment 0, we want to stop playback immediately once waiting-for-key happens even though video queue is not exhausted, right?
Flags: needinfo?(cpearce)
(Assignee)

Updated

2 years ago
Assignee: nobody → jyavenard
(Assignee)

Comment 8

2 years ago
(In reply to JW Wang [:jwwang] [:jw_wang] from comment #7)
> Per comment 0, we want to stop playback immediately once waiting-for-key
> happens even though video queue is not exhausted, right?

doesn't make much sense to me to stop processing the frames we have already decoded properly.

It's the same deal as moving readyState back to HAVE_CURRENT_DATA. We only do so once there's no longer any frames to play
(Reporter)

Comment 9

2 years ago
We're supposed to stop playback at the last decrypted and decoded frame of the decrypted stream. That means in the example above we'll still have decoded audio data enqueued after the end of the video frames that we can decrypt, and we'll stop playback at the last video frame we can decrypt and we won't play the audio.
Flags: needinfo?(cpearce)
(Assignee)

Comment 10

2 years ago
ok... so it's exactly the behaviour currently occurring when we have a gap in the data on either audio or video.

it stops at the intersection of the buffered range.
I don't think we'll even need to modify the MDSM.
I'll start working on this shortly. Still finishing testing bug 1311876
Great! I will let jya take this bug.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 19

2 years ago
I get some intermittent timeout with that test page if I refresh the page too quickly.
[MediaPlayback #2]: V/MediaFormatReader MediaFormatReader(12f338000)::Update: No need for additional input (pending:0)
[MediaPlayback #1]: V/MediaDecoder DecodedAudioDataSink=12be2f400 One new audio packet available.
[MediaPlayback #2]: V/MediaDecoder VideoSink=12d5c6be0 playing video frame 1958333 (id=30) (vq-queued=1)
[MediaPlayback #2]: D/MediaDecoder Decoder=106f81c00 state=DECODING change state to: BUFFERING
[MediaPlayback #2]: D/MediaDecoder Decoder=106f81c00 state=DECODING Exiting DECODING, decoded for 2.017s
[MediaPlayback #2]: D/MediaDecoder Decoder=106f81c00 StopPlayback()
[MediaPlayback #2]: V/MediaDecoder VideoSink=12d5c6be0  playing (1) -> (0)
[MediaPlayback #2]: V/MediaDecoder VideoSink=12d5c6be0 playing video frame 1958333 (id=30) (vq-queued=1)
[MediaPlayback #2]: D/MediaDecoder Decoder=106f81c00 state=BUFFERING Playback rate: 0.0KB/s (unreliable) download rate: 0.0KB/s (unreliable)
[MediaPlayback #2]: D/MediaDecoder Decoder=106f81c00 Changed mNextFrameStatus to NEXT_FRAME_UNAVAILABLE_BUFFERING
[Main Thread]: D/nsMediaElement MediaElement 12106c800 UpdateReadyStateInternal() Next frame not available
[Main Thread]: D/nsMediaElement 12106c800 Ready state changed to HAVE_CURRENT_DATA
[Main Thread]: D/nsMediaElementEvents 12106c800 Queuing event timeupdate
[Main Thread]: D/nsMediaElementEvents 12106c800 Queuing event waiting
[Main Thread]: D/nsMediaElement MediaElement 12106c800 UpdateReadyStateInternal() Next frame not available
[Main Thread]: D/nsMediaElementEvents 12106c800 Dispatching event timeupdate
[Main Thread]: D/nsMediaElementEvents 12106c800 Dispatching event waiting
[MediaPlayback #1]: D/MediaDecoder Decoder=106f81c00 state=BUFFERING In buffering mode, waiting to be notified: outOfAudio: 0, mAudioStatus: idle, outOfVideo: 1, mVideoStatus: waiting
[Main Thread]: D/nsMediaElementEvents 12106c800 Queuing event stalled
[Main Thread]: D/nsMediaElementEvents 12106c800 Dispatching event stalled


ohhh.. i see why, it doesn't go through the NEXT_FRAME_UNAVAILABLE state
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 23

2 years ago
mozreview-review
Comment on attachment 8805043 [details]
Bug 1312886: P1. Fix comments.

https://reviewboard.mozilla.org/r/88876/#review88160

::: dom/media/MediaFormatReader.h:309
(Diff revision 1)
>      MozPromiseRequestHolder<MediaDataDecoder::InitPromise> mInitPromise;
>      // False when decoder is created. True when decoder Init() promise is resolved.
>      bool mDecoderInitialized;
>      bool mOutputRequested;
>      // Set to true once the MediaDataDecoder has been fed a compressed sample.
>      // No more sample will be passed to the decoder while true.

"No more sample*s* will be passed"

Since we're correcting comments...
Attachment #8805043 - Flags: review?(cpearce) → review+
(Reporter)

Comment 24

2 years ago
mozreview-review
Comment on attachment 8805044 [details]
Bug 1312886: P2. Never move past HAVE_FUTURE_DATA if we're waiting for a key,

https://reviewboard.mozilla.org/r/88878/#review88164
Attachment #8805044 - Flags: review?(cpearce) → review+
(Reporter)

Comment 25

2 years ago
mozreview-review
Comment on attachment 8805048 [details]
Bug 1312886: P5. Enter buffering mode when waiting for a key.

https://reviewboard.mozilla.org/r/88886/#review88172
Attachment #8805048 - Flags: review?(cpearce) → review+
(Reporter)

Comment 26

2 years ago
mozreview-review
Comment on attachment 8805049 [details]
Bug 1312886: P6. Show waitingforkey state in about:media.

https://reviewboard.mozilla.org/r/88888/#review88174
Attachment #8805049 - Flags: review?(cpearce) → review+
(Reporter)

Comment 27

2 years ago
Thanks for jumping on this so quickly. Given the amount of work you've done here, it would have taken me a lot longer to get it working.

Comment 28

2 years ago
mozreview-review
Comment on attachment 8805046 [details]
Bug 1312886: [MSE] P3. Only rely on waiting promise to determine buffering state.

https://reviewboard.mozilla.org/r/88882/#review88206
Attachment #8805046 - Flags: review?(jwwang) → review+

Comment 29

2 years ago
mozreview-review
Comment on attachment 8805047 [details]
Bug 1312886: P4. Override MediaResource reporting if we have a pending promise.

https://reviewboard.mozilla.org/r/88884/#review88210
Attachment #8805047 - Flags: review?(jwwang) → review+

Comment 30

2 years ago
mozreview-review
Comment on attachment 8805058 [details]
Bug 1312886: P7. Handle case where nextFrameStatus jumps straight to NEXT_FRAME_UNAVAILABLE_BUFFERING.

https://reviewboard.mozilla.org/r/88908/#review88212
Attachment #8805058 - Flags: review?(jwwang) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8805045 - Attachment is obsolete: true
Attachment #8805045 - Flags: review?(dglastonbury)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 41

2 years ago
Created attachment 8805416 [details] [diff] [review]
Update EME multikey-sequential WPT from upstream so they pass

This should make clearkey-mp4-playback-temporary-multikey-sequential[-readyState].html pass.
(Reporter)

Updated

2 years ago
Assignee: jyavenard → cpearce
Status: NEW → ASSIGNED
(Assignee)

Updated

2 years ago
Assignee: cpearce → jyavenard
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 50

2 years ago
mozreview-review
Comment on attachment 8805419 [details]
Bug 1312886: P8. Update EME multikey-sequential WPT from upstream so they pass.

https://reviewboard.mozilla.org/r/89134/#review88280
Attachment #8805419 - Flags: review?(jyavenard) → review+

Comment 51

2 years ago
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/246305d4d3bb
P1. Fix comments. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/e24a86e2fb9f
P2. Never move past HAVE_FUTURE_DATA if we're waiting for a key, r=cpearce
https://hg.mozilla.org/integration/autoland/rev/3ffa32dd1a2b
[MSE] P3. Only rely on waiting promise to determine buffering state. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/be4b0b1e4733
P4. Override MediaResource reporting if we have a pending promise. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/fdf007f31cd6
P5. Enter buffering mode when waiting for a key. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/76f8b94d7b66
P6. Show waitingforkey state in about:media. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/09371a75ac7e
P7. Handle case where nextFrameStatus jumps straight to NEXT_FRAME_UNAVAILABLE_BUFFERING. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/5e28fc45d136
P8. Update EME multikey-sequential WPT from upstream so they pass. r=jya
(Assignee)

Updated

2 years ago
Depends on: 1313583
You need to log in before you can comment on or make changes to this bug.