1059049 - [EME] Ensure CDMProxy is passed through from MSReader to subreader

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

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

For EME to work inside MSE, we need the CDMProxy to be passed through MediaSourceDecoder's SetCDMProxy() override to the subreaders somehow.

Comment 1

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Defer parsing of saio/saiz boxes until after trun

The stagefright demuxer makes some strong but incorrect assumptions about the order of boxes in the `traf' box. This patch merely hacks around the problem by storing the (offset,size) of the saio/saiz boxes if the trun box hasn't been seen yet, and then parsing them after.

Comment 2

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Improve logging slightly in test_encryptedMediaExtensions.html

Comment 3

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Test for EME through MSE

Comment 4

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Comment on attachment 8502912 [details] [diff] [review]
Test for EME through MSE

Test. Singular.

Comment 5

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Test for EME through MSE

Comment 6

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Comment on attachment 8502914 [details] [diff] [review]
Test for EME through MSE

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

::: content/media/test/gizmo-frag-cenc.xml
@@ +11,5 @@
> +    MP4Box -dash 1000 -rap -segment-name gizmo-frag-cenc -subsegs-per-sidx 5 -rem 2 gizmo-cenc.mp4
> +
> +  The above command will generate a set of fragments in |gizmo-frag-cenc*.m4s|
> +  and a single |gizmo-frag-cencinit.mp4| containing just the init segment.
> +  

grr. whitespace.

Comment 7

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Pass CDMProxy through MediaSourceDecoder to subdecoders

Comment 8

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

Comment on attachment 8502914 [details] [diff] [review]
Test for EME through MSE

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

::: content/media/test/test_encryptedMediaExtensions.html
@@ +133,5 @@
> +    req.send(null);
> +  }
> +
> +  ms.addEventListener("sourceopen", function () {
> +    sb = ms.addSourceBuffer('video/mp4; codecs="avc1.64000d,mp4a.40.2"');

Can you pass test.type here, and include the codecs string in the type?

Comment 9

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

Comment on attachment 8502915 [details] [diff] [review]
Pass CDMProxy through MediaSourceDecoder to subdecoders

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

You need #ifdef MOZ_EME around all your EME specific code.

Test it with `ac_add_options --disable-eme` please.

::: content/media/mediasource/MediaSourceDecoder.cpp
@@ +189,5 @@
>  
> +nsresult
> +MediaSourceDecoder::SetCDMProxy(CDMProxy* aProxy)
> +{
> +  mCDMProxy = aProxy;

I think to make this threadsafe, you should either take the decoder monitor, or assert the caller holds it?

::: content/media/mediasource/MediaSourceDecoder.h
@@ +70,5 @@
>    // calls Attach/DetachMediaSource on this decoder to set and clear
>    // mMediaSource.
>    dom::MediaSource* mMediaSource;
>    nsRefPtr<MediaSourceReader> mReader;
> +  nsRefPtr<CDMProxy> mCDMProxy;

MediaDecoder already has a (private) mCDMProxy field, why do you need this again here? You can access the stored CDMProxy with MediaDecoder::GetCDMProxy().

::: content/media/mediasource/SourceBufferDecoder.h
@@ +91,5 @@
> +
> +  virtual CDMProxy* GetCDMProxy() MOZ_OVERRIDE
> +  {
> +    MOZ_ASSERT(OnDecodeThread() || NS_IsMainThread());
> +    return mCDMProxy;

You write to mCDMProxy on the main thread, but you can read it on the decode thread without any kind of synchronization. You should have some kind of memory barrier here. You'll _probably_ be ok without it, but you should still be careful.

::: content/media/mediasource/TrackBuffer.cpp
@@ +270,5 @@
>    nsresult rv = reader->ReadMetadata(&mi, getter_Transfers(tags));
> +  reader->SetIdle();
> +
> +  if (NS_SUCCEEDED(rv) && reader->IsWaitingMediaResources()) {
> +    ReentrantMonitorAutoEnter mon(mParentDecoder->GetReentrantMonitor());

What if IsWaitingMediaResources() returns true because we're waiting for non CDM resources? We'll end up waiting for a call to SetCDMProxy() that has already happened, so we'll never stop waiting...

Comment 10

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Pass CDMProxy through MediaSourceDecoder to subdecoders

Comment 11

[Matthew Gregan [:kinetik]]

Comment on attachment 8502957 [details] [diff] [review]
Pass CDMProxy through MediaSourceDecoder to subdecoders

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

r+ with nits

::: content/media/mediasource/MediaSourceDecoder.cpp
@@ +190,5 @@
> +#ifdef MOZ_EME
> +nsresult
> +MediaSourceDecoder::SetCDMProxy(CDMProxy* aProxy)
> +{
> +  MediaDecoder::SetCDMProxy(aProxy);

I know this always returns NS_OK now, but if that's ever changed, it'd be nice if the rv was already checked and handled here.

::: content/media/mediasource/TrackBuffer.cpp
@@ +432,5 @@
> +
> +  for (uint32_t i = 0; i < mWaitingDecoders.Length(); ++i) {
> +    CDMCaps::AutoLock caps(aProxy->Capabilites());
> +    caps.CallOnMainThreadWhenCapsAvailable(
> +      NS_NewRunnableMethodWithArg<nsRefPtr<SourceBufferDecoder>>(this,

SourceBufferDecoders are kept alive by mDecoders, so this can be a raw pointer.

::: content/media/mediasource/TrackBuffer.h
@@ +131,5 @@
>    // Access protected by mParentDecoder's monitor.
>    nsTArray<nsRefPtr<SourceBufferDecoder>> mInitializedDecoders;
>  
> +  // Decoders which are still waiting on either a Content Decryption Module or
> +  // more data to be able to finish ReadMetadata.

They're only waiting on the CDM now, right?

Comment 12

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Comment on attachment 8502908 [details] [diff] [review]
Defer parsing of saio/saiz boxes until after trun

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

I wish we didn't have to this kind of mumbo jumbo. Oh well...

Comment 13

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: plug-bdm-leak.patch

+ one liner to make ASAN happy. Unsure why this hadn't turned up before.

Comment 14

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6dc9dc5e1b73
https://hg.mozilla.org/integration/mozilla-inbound/rev/eedb87c0217f
https://hg.mozilla.org/integration/mozilla-inbound/rev/4794ad4ea17f
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c73f7c68c41
https://hg.mozilla.org/integration/mozilla-inbound/rev/fcbf0749ad45

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6dc9dc5e1b73
https://hg.mozilla.org/mozilla-central/rev/eedb87c0217f
https://hg.mozilla.org/mozilla-central/rev/4794ad4ea17f
https://hg.mozilla.org/mozilla-central/rev/7c73f7c68c41
https://hg.mozilla.org/mozilla-central/rev/fcbf0749ad45

Comment 16

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

Mass update firefox-status to track EME uplift.