1393336 - Remove WaitForCDM state from MDSM.

Status: RESOLVED FIXED

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

Description

[Kilik Kuo [:kikuo] (inactive)]

Per discussion with JW, it's simpler for MDSM to be aware of waiting for data instead of entering WaitForCDM state if the CDMProxy is set to null. (MediaKey is detached from HTMLMediaElement).

Comment 1

[Kilik Kuo [:kikuo] (inactive)]

Attachment: Bug 1393336 - Remove WaitForCDM state from MDSM.

Review commit: https://reviewboard.mozilla.org/r/172594/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/172594/

Comment 2

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8901103 [details]
Bug 1393336 - Remove WaitForCDM state from MDSM.

https://reviewboard.mozilla.org/r/172594/#review177844

I'd like to make it possible to detach / reattach MediaKey for HTMLMediaElement, it's necessary to make MediaDecoder able to  set null as cdm proxy to MFR.

Removing WaitForCDM state in MSMD could simply the logic.

Comment 3

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8901103 [details]
Bug 1393336 - Remove WaitForCDM state from MDSM.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/172594/diff/1-2/

Comment 4

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8901103 [details]
Bug 1393336 - Remove WaitForCDM state from MDSM.

https://reviewboard.mozilla.org/r/172594/#review178460

LGTM. r?jya for MFR changes.

Comment 5

[Jean-Yves Avenard [:jya]]

Comment on attachment 8901103 [details]
Bug 1393336 - Remove WaitForCDM state from MDSM.

https://reviewboard.mozilla.org/r/172594/#review178576

::: dom/media/MediaDecoder.cpp:1462
(Diff revision 2)
> -  mCDMProxyPromiseHolder.ResolveIfExists(aProxy, __func__);
> +  nsCOMPtr<nsIRunnable> r = NS_NewRunnableFunction(
> +    "MediaFormatReader::SetCDMProxy",
> +    [reader, proxy]() {
> +    reader->SetCDMProxy(proxy);
> +    });
> +  mReader->OwnerThread()->Dispatch(r.forget());

I think dispatching to the reader's thread should be up to the reader.

MediaDecoder shouldn't have to know about the MediaDecoderReader threading model.

Comment 6

[JW Wang [:jwwang] [:jw_wang]]

We had a similar discussion here about the thread model of MDR:
https://bugzilla.mozilla.org/show_bug.cgi?id=1215003&mark=18-22#c18

Comment 7

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8901103 [details]
Bug 1393336 - Remove WaitForCDM state from MDSM.

https://reviewboard.mozilla.org/r/172594/#review178576

> I think dispatching to the reader's thread should be up to the reader.
> 
> MediaDecoder shouldn't have to know about the MediaDecoderReader threading model.

I suggest that we could have a discussion in Bug 1394728 to find out which way we want MFR to go or how should MediaDecoder / MDSM / MFR inter-op with each.

I would drop this issue for now as I just wanna follow the way how MFR is used in MediaDecoder to avoid a inconsistent style.

Comment 8

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8901103 [details]
Bug 1393336 - Remove WaitForCDM state from MDSM.

https://reviewboard.mozilla.org/r/172594/#review178990

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b31c285e131d
Some orange, but they're not related.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/59834f182e67
Remove WaitForCDM state from MDSM. r=jwwang,jya

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/59834f182e67