Closed Bug 1059049 Opened 6 years ago Closed 5 years ago

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

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
firefox37 --- fixed
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: cpearce, Assigned: eflores)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 2 obsolete files)

For EME to work inside MSE, we need the CDMProxy to be passed through MediaSourceDecoder's SetCDMProxy() override to the subreaders somehow.
Assignee: cpearce → edwin
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.
Attachment #8502908 - Flags: review?(ajones)
Attached patch Test for EME through MSE (obsolete) — Splinter Review
Attachment #8502912 - Flags: review?(jyavenard)
Attachment #8502912 - Flags: review?(cpearce)
Comment on attachment 8502912 [details] [diff] [review]
Test for EME through MSE

Test. Singular.
Attachment #8502912 - Attachment description: Tests for EME through MSE → Test for EME through MSE
Attachment #8502912 - Attachment is obsolete: true
Attachment #8502912 - Flags: review?(jyavenard)
Attachment #8502912 - Flags: review?(cpearce)
Attachment #8502914 - Flags: review?(jyavenard)
Attachment #8502914 - Flags: review?(cpearce)
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.
Attachment #8502910 - Flags: review?(cpearce) → review+
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?
Attachment #8502914 - Flags: review?(cpearce) → review+
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...
Attachment #8502915 - Flags: review?(cpearce) → review-
Attachment #8502915 - Flags: review?(kinetik)
Attachment #8502915 - Attachment is obsolete: true
Attachment #8502957 - Flags: review?(kinetik)
Attachment #8502957 - Flags: review?(cpearce)
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?
Attachment #8502957 - Flags: review?(kinetik) → review+
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...
Attachment #8502908 - Flags: review?(ajones) → review+
Attachment #8502957 - Flags: review?(cpearce) → review+
Attachment #8502914 - Flags: review?(jyavenard) → review+
+ one liner to make ASAN happy. Unsure why this hadn't turned up before.
Attachment #8503908 - Flags: review?(cpearce)
Attachment #8503908 - Flags: review?(cpearce) → review+
Mass update firefox-status to track EME uplift.
You need to log in before you can comment on or make changes to this bug.