Closed Bug 1339748 Opened 7 years ago Closed 7 years ago

EMEDecoderModule doesn't need AVCC conversion

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: cpearce, Assigned: jya)

References

Details

Attachments

(2 files, 2 obsolete files)

Now that we've removed the Adobe GMP which requires AVVCC, we can stop making Gecko convert samples sent to the CDM for decoding from AnnexB to AVCC, since the current CDMs convert the AVCC back to AnnexB before decoding anyway; we're needlessly converting.
Comment on attachment 8837493 [details]
Bug 1339748 - Convert samples to AnnexB before passing to CDMs, rather than inside CDMs.

https://reviewboard.mozilla.org/r/112600/#review114234

There wasn't any unecessary conversion performed, as the data parsed is always avcc (annexB is only ever used with mpeg-ts).

So this just shift the conversion from being done within the GMP to within the H264Converter.

::: media/libstagefright/binding/AnnexB.cpp:74
(Diff revision 1)
>      if (!samplewriter->Prepend(annexB->Elements(), annexB->Length())) {
>        return false;
>      }
> +
> +    // For keyframes, ConvertSampleToAnnexB will prepend AnnexB extra data
> +    // at the start of the input. This will mess up the encryption subsammple

This is not technically correct.

And that's what the second argument passed to ConvertSampleToAnnexB is for.

https://dxr.mozilla.org/mozilla-central/source/media/libstagefright/binding/include/mp4_demuxer/AnnexB.h#23

It's also not "AnnexB" extra data (there's no such thing) that is prepend, but the SPS/PPS NAL

I would argue that passing AVCC to the decoder is more efficient; as data we will retrieve from a MP4 
is *always* AVCC.

So when the PDM ask for AVCC there's no unecessary double conversion; quite the opposite.
The H264Converter will pass data as-is.

Now you simply shift the conversion from within the CDM to the H264Converter.

it will also remove the need for this change which appears quite ugly to me...
Attachment #8837493 - Flags: review?(jyavenard) → review+
Comment on attachment 8837493 [details]
Bug 1339748 - Convert samples to AnnexB before passing to CDMs, rather than inside CDMs.

https://reviewboard.mozilla.org/r/112600/#review114234

My point was to shift the conversion into the H264Converter yes, so that we don't explicitly need to do the conversion in so many places.

> This is not technically correct.
> 
> And that's what the second argument passed to ConvertSampleToAnnexB is for.
> 
> https://dxr.mozilla.org/mozilla-central/source/media/libstagefright/binding/include/mp4_demuxer/AnnexB.h#23
> 
> It's also not "AnnexB" extra data (there's no such thing) that is prepend, but the SPS/PPS NAL
> 
> I would argue that passing AVCC to the decoder is more efficient; as data we will retrieve from a MP4 
> is *always* AVCC.
> 
> So when the PDM ask for AVCC there's no unecessary double conversion; quite the opposite.
> The H264Converter will pass data as-is.
> 
> Now you simply shift the conversion from within the CDM to the H264Converter.
> 
> it will also remove the need for this change which appears quite ugly to me...

I'll update the comment so that it's technically correct.

I don't know what you mean by: "it will also remove the need for this change which appears quite ugly to me..."

It sounds like you think we don't need this change. If we don't correct the subsamples, they'll be wrong and decryption will fail.

Was there some action you expect me to take based on this comment?
Pushed by cpearce@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5cc18343abdd
Convert samples to AnnexB before passing to CDMs, rather than inside CDMs. r=jya
Backed out for failing test_eme_canvas_blocked.html on Linux x64 asan:

https://hg.mozilla.org/integration/autoland/rev/2670e3095f9446ac8f3eb4fd44fa666b9ac0ce47

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=5cc18343abddfcd0700378a89b656374a732e407
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=77743319&repo=autoland

[task 2017-02-15T23:39:20.009788Z] 23:39:20     INFO - [23:39:19.367] video-only with 2 keys-1 got message from CDM: {"kids":["flcdA35XHQN-Vx0DflcdAw","flcdBH5XHQR-Vx0EflcdBA"],"type":"temporary"}
[task 2017-02-15T23:39:20.010645Z] 23:39:20     INFO - TEST-PASS | dom/media/test/test_eme_canvas_blocked.html | [23:39:19.367] video-only with 2 keys-1 key session type should match 
[task 2017-02-15T23:39:20.012717Z] 23:39:20     INFO - TEST-PASS | dom/media/test/test_eme_canvas_blocked.html | [23:39:19.368] video-only with 2 keys-1 message event should contain key ID array 
[task 2017-02-15T23:39:20.013606Z] 23:39:20     INFO - [23:39:19.370] video-only with 2 keys-1 found key 7e5733337e5733337e5733337e573333 for key id 7e571d037e571d037e571d037e571d03
[task 2017-02-15T23:39:20.014501Z] 23:39:20     INFO - [23:39:19.374] video-only with 2 keys-1 found key 7e5744447e5744447e5744447e574444 for key id 7e571d047e571d047e571d047e571d04
[task 2017-02-15T23:39:20.017566Z] 23:39:20     INFO - [23:39:19.376] video-only with 2 keys-1 sending update message to CDM: {"keys":[{"kty":"oct","kid":"flcdA35XHQN-Vx0DflcdAw","k":"flczM35XMzN-VzMzflczMw"},{"kty":"oct","kid":"flcdBH5XHQR-Vx0EflcdBA","k":"fldERH5XRER-V0REfldERA"}],"type":"temporary"}
[task 2017-02-15T23:39:20.018428Z] 23:39:20     INFO - [23:39:19.380] video-only with 2 keys-1 waitingforkey
[task 2017-02-15T23:39:20.019268Z] 23:39:20     INFO - [23:39:19.381] video-only with 2 keys-1 MediaKeySession update ok!
[task 2017-02-15T23:39:20.020162Z] 23:39:20     INFO - [23:39:19.382] video-only with 2 keys-1 session[1].generateRequest(cenc, 0000004470737368010000001077efecc0b24d02ace33c1e52e2fb4b000000027e571d037e571d037e571d037e571d037e571d047e571d047e571d047e571d0400000000) succeeded
[task 2017-02-15T23:39:20.021056Z] 23:39:20     INFO - [23:39:19.383] vp9 in mp4-0 got message from CDM: {"kids":["LNsO1hGYU-eFBnHD6ZBsPA"],"type":"temporary"}
[task 2017-02-15T23:39:20.021918Z] 23:39:20     INFO - TEST-PASS | dom/media/test/test_eme_canvas_blocked.html | [23:39:19.383] vp9 in mp4-0 key session type should match 
[task 2017-02-15T23:39:20.022922Z] 23:39:20     INFO - TEST-PASS | dom/media/test/test_eme_canvas_blocked.html | [23:39:19.384] vp9 in mp4-0 message event should contain key ID array 
[task 2017-02-15T23:39:20.026545Z] 23:39:20     INFO - [23:39:19.387] vp9 in mp4-0 found key 808B9ADAC384DE1E4F56140F4AD76194 for key id 2cdb0ed6119853e7850671c3e9906c3c
[task 2017-02-15T23:39:20.027439Z] 23:39:20     INFO - [23:39:19.388] vp9 in mp4-0 sending update message to CDM: {"keys":[{"kty":"oct","kid":"LNsO1hGYU-eFBnHD6ZBsPA","k":"gIua2sOE3h5PVhQPStdhlA"}],"type":"temporary"}
[task 2017-02-15T23:39:20.028273Z] 23:39:20     INFO - [23:39:19.393] vp9 in mp4-0 waitingforkey
[task 2017-02-15T23:39:20.029130Z] 23:39:20     INFO - [23:39:19.394] vp9 in mp4-0 MediaKeySession update ok!
[task 2017-02-15T23:39:20.029987Z] 23:39:20     INFO - [23:39:19.395] vp9 in mp4-0 session[2].generateRequest(cenc, 0000003470737368010000001077efecc0b24d02ace33c1e52e2fb4b000000012cdb0ed6119853e7850671c3e9906c3c00000000) succeeded
[task 2017-02-15T23:39:20.030854Z] 23:39:20     INFO - [23:39:19.396] vp9 in mp4-0 createSession(temporary) for (cenc, 0000003470737368010000001077efecc0b24d02ace33c1e52e2fb4b000000012cdb0ed6119853e7850671c3e9906c3c00000000)
[task 2017-02-15T23:39:20.032270Z] 23:39:20     INFO - [23:39:19.396] vp9 in mp4-0 session[].generateRequest(cenc, 0000003470737368010000001077efecc0b24d02ace33c1e52e2fb4b000000012cdb0ed6119853e7850671c3e9906c3c00000000)
[task 2017-02-15T23:39:20.033861Z] 23:39:20     INFO - [23:39:19.398] video-only with 2 keys-1 error
[task 2017-02-15T23:39:20.035631Z] 23:39:20     INFO - Buffered messages finished
[task 2017-02-15T23:39:20.044247Z] 23:39:20     INFO - TEST-UNEXPECTED-FAIL | dom/media/test/test_eme_canvas_blocked.html | video-only with 2 keys-1 got error event; [object Event] 
[task 2017-02-15T23:39:20.044572Z] 23:39:20     INFO -     bail/<@dom/media/test/eme.js:26:5
[task 2017-02-15T23:39:20.046454Z] 23:39:20     INFO -     EventHandlerNonNull*SetupEME@dom/media/test/eme.js:317:15
[task 2017-02-15T23:39:20.047975Z] 23:39:20     INFO -     startTest@dom/media/test/test_eme_canvas_blocked.html:21:11
[task 2017-02-15T23:39:20.048331Z] 23:39:20     INFO -     MediaTestManager/this.nextTest@dom/media/test/manifest.js:1705:7
[task 2017-02-15T23:39:20.049495Z] 23:39:20     INFO -     MediaTestManager/this.runTests/<@dom/media/test/manifest.js:1642:7
[task 2017-02-15T23:39:20.049826Z] 23:39:20     INFO -     Async*MediaTestManager/this.runTests@dom/media/test/manifest.js:1641:5
[task 2017-02-15T23:39:20.050183Z] 23:39:20     INFO -     beginTest@dom/media/test/test_eme_canvas_blocked.html:47:3
[task 2017-02-15T23:39:20.052585Z] 23:39:20     INFO -     Async*SetupEMEPref@dom/media/test/eme.js:466:3
[task 2017-02-15T23:39:20.053570Z] 23:39:20     INFO -     @dom/media/test/test_eme_canvas_blocked.html:52:3
[task 2017-02-15T23:39:20.054457Z] 23:39:20     INFO - [object Event]
Flags: needinfo?(cpearce)
Comment on attachment 8837493 [details]
Bug 1339748 - Convert samples to AnnexB before passing to CDMs, rather than inside CDMs.

https://reviewboard.mozilla.org/r/112600/#review114434

::: media/libstagefright/binding/AnnexB.cpp:77
(Diff revision 2)
> +
> +    // Prepending the SPS/PPS NAL will mess up the encryption subsample
> +    // offsets. So we need to account for the extra bytes by increasing
> +    // the length of the first clear data subsample. Otherwise decryption
> +    // will fail.
> +    if (aSample->mCrypto.mValid) {

&& aAddSPS

the data is only added if that boolean is true
Comment on attachment 8837493 [details]
Bug 1339748 - Convert samples to AnnexB before passing to CDMs, rather than inside CDMs.

https://reviewboard.mozilla.org/r/112600/#review114470

::: media/libstagefright/binding/AnnexB.cpp:77
(Diff revision 2)
> +
> +    // Prepending the SPS/PPS NAL will mess up the encryption subsample
> +    // offsets. So we need to account for the extra bytes by increasing
> +    // the length of the first clear data subsample. Otherwise decryption
> +    // will fail.
> +    if (aSample->mCrypto.mValid) {

That's checked on line 66, right?
Comment on attachment 8837493 [details]
Bug 1339748 - Convert samples to AnnexB before passing to CDMs, rather than inside CDMs.

https://reviewboard.mozilla.org/r/112600/#review114434

> && aAddSPS
> 
> the data is only added if that boolean is true

oh you're right, my bad..
OK, so there's a problem in my EMEDecoderModule::DecoderNeedsConversion() implementation. If we are using the CDM to decrypt and using a wrapped decoder in Gecko to decode, then we don't know whether the wrapped decoder will need AVCC or AnnexB. There's not a clear path to asking whether the wrapped decoder will require AnnexB or not, as we don't have a reference to a PDM, only the PDMFactory.

The Windows and Android PDMs report that they need AnnexB in their DecoderNeedsConversion() functions.

I was getting test failures on Linux ASAN because I was requesting AnnexB always, and the version of ffmpeg on the machines that run Linux ASAN can't handle that. ffmpeg on every other linux seems to handle AnnexB fine.
Flags: needinfo?(cpearce)
I think the approach would be to only convert the type to what the end decoder will need. I'll take it from there.

It's also going to be nicer that way anyway
Assignee: cpearce → jyavenard
So this should achieve what you wanted to do.
It now checks what the final decoder takes and only a single conversion will ever be performed.

But this is still incomplete.

As it is, all GMP video decoders except widevine will be broken (ok,that's just OpenH264, but the point remains the same).

We need a way for the GMPVideoDecoder to tell which format it needs, right now it assumes it's either AVCC or AnnexB. And this requires to modify the GMP API, which we likely don't want.

Secondly, the GMP API is very obviously tailored to be using AVCC.
To create a decoder, it needs an extra data. There's no such thing with AnnexB, yet the Widevine decoder requires it anyway.
extradata is the SPS/PPS NAL in AVCC format, along with a small header defining the size of the NAL's size.

So in all, I think we should forget the idea, and continue with the requirement that GMPs take AVCC and P3 should be dropped from this patch queue.

We should however keep P1 and P2, as it's going to be required when we move the decoders from other platforms OOP; currently it works because the only decoder running OOP is on windows which takes annexB.
Attachment #8837493 - Attachment is obsolete: true
Comment on attachment 8838760 [details]
Bug 1339748: P1. Proxify all MediaDataDecoder's APIs.

https://reviewboard.mozilla.org/r/113560/#review115184
Attachment #8838760 - Flags: review?(cpearce) → review+
Comment on attachment 8838761 [details]
Bug 1339748: P2. Let the MediaDataDecoder tells the format it wants.

https://reviewboard.mozilla.org/r/113562/#review115156

::: dom/media/platforms/agnostic/BlankDecoderModule.cpp:35
(Diff revision 1)
>  {
>  public:
>    BlankMediaDataDecoder(BlankMediaDataCreator* aCreator,
>                          const CreateDecoderParams& aParams)
>      : mCreator(aCreator)
> +    , mIsH264(aParams.mConfig.GetType() == TrackInfo::kVideoTrack

Can track type be something other than kVideoTrack when MP4Decoder::IsH264(aParams.mConfig.mMimeType) returns true?

That is, can we drop the check on kVideoTrack here?
Attachment #8838761 - Flags: review?(cpearce) → review+
(In reply to Jean-Yves Avenard [:jya] from comment #17)
> So in all, I think we should forget the idea, and continue with the
> requirement that GMPs take AVCC and P3 should be dropped from this patch
> queue.
> 
> We should however keep P1 and P2, as it's going to be required when we move
> the decoders from other platforms OOP; currently it works because the only
> decoder running OOP is on windows which takes annexB.

Landing P1 and P2 and forgetting P3 is OK with me.
Attachment #8838762 - Attachment is obsolete: true
Attachment #8838762 - Flags: review?(jyavenard)
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2a7f99800581
P1. Proxify all MediaDataDecoder's APIs. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/f508a675e8dc
P2. Let the MediaDataDecoder tells the format it wants. r=cpearce
Backed out for asserting in many tests on Windows 8 x64:

https://hg.mozilla.org/integration/autoland/rev/72dc326b8410067c67dddd232fdec502ab370ef4
https://hg.mozilla.org/integration/autoland/rev/e26a2e2e873e206ef7d198c8b60f54d898f48501

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=f508a675e8dc2780bff1aa99d2299494288a9ae1&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable

Failure log example (e10s): https://treeherder.mozilla.org/logviewer.html#?job_id=78570186&repo=autoland

07:02:57     INFO - Assertion failure: NS_GetCurrentThread() == mThread, at c:/builds/moz2_slave/autoland-w64-d-000000000000000/build/src/dom/media/ipc/VideoDecoderChild.cpp:310
07:03:10     INFO - #01: mozilla::dom::VideoDecoderChild::NeedsConversion() [dom/media/ipc/VideoDecoderChild.cpp:304]
07:03:10     INFO - 
07:03:10     INFO - #02: mozilla::H264Converter::Decode(mozilla::MediaRawData *) [dom/media/platforms/wrappers/H264Converter.cpp:100]
07:03:10     INFO - 
07:03:10     INFO - #03: mozilla::MediaFormatReader::DecoderFactory::Wrapper::Decode(mozilla::MediaRawData *) [dom/media/MediaFormatReader.cpp:270]
07:03:10     INFO - 
07:03:10     INFO - #04: mozilla::MediaFormatReader::DecodeDemuxedSamples(mozilla::TrackInfo::TrackType,mozilla::MediaRawData *) [dom/media/MediaFormatReader.cpp:1722]
07:03:10     INFO - 
07:03:10     INFO - #05: mozilla::MediaFormatReader::HandleDemuxedSamples(mozilla::TrackInfo::TrackType,mozilla::AbstractMediaDecoder::AutoNotifyDecoded &) [dom/media/MediaFormatReader.cpp:1825]
07:03:10     INFO - 
07:03:10     INFO - #06: mozilla::MediaFormatReader::Update(mozilla::TrackInfo::TrackType) [dom/media/MediaFormatReader.cpp:2153]
07:03:10     INFO - 
07:03:10     INFO - #07: mozilla::detail::RunnableMethodImpl<mozilla::MediaFormatReader * const,void ( mozilla::MediaFormatReader::*)(mozilla::TrackInfo::TrackType),1,0,mozilla::TrackInfo::TrackType>::Run() [obj-firefox/dist/include/nsThreadUtils.h:892]
07:03:10     INFO - 
07:03:10     INFO - #08: mozilla::AutoTaskDispatcher::TaskGroupRunnable::Run() [obj-firefox/dist/include/mozilla/TaskDispatcher.h:196]
07:03:10     INFO - 
07:03:10     INFO - #09: mozilla::TaskQueue::Runner::Run() [xpcom/threads/TaskQueue.cpp:233]
07:03:10     INFO - 
07:03:10     INFO - #10: nsThreadPool::Run() [xpcom/threads/nsThreadPool.cpp:227]
07:03:10     INFO - 
07:03:10     INFO - #11: nsThread::ProcessNextEvent(bool,bool *) [xpcom/threads/nsThread.cpp:1265]
07:03:10     INFO - 
07:03:10     INFO - #12: NS_ProcessNextEvent(nsIThread *,bool) [xpcom/threads/nsThreadUtils.cpp:389]
07:03:10     INFO - 
07:03:10     INFO - #13: mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate *) [ipc/glue/MessagePump.cpp:368]
07:03:10     INFO - 
07:03:10     INFO - #14: MessageLoop::RunHandler() [ipc/chromium/src/base/message_loop.cc:232]
07:03:10     INFO - 
07:03:10     INFO - #15: MessageLoop::Run() [ipc/chromium/src/base/message_loop.cc:212]
07:03:10     INFO - 
07:03:10     INFO - #16: nsThread::ThreadFunc(void *) [xpcom/threads/nsThread.cpp:497]
07:03:10     INFO - 
07:03:10     INFO - #17: PR_NativeRunThread [nsprpub/pr/src/threads/combined/pruthr.c:406]
07:03:10     INFO - 
07:03:10     INFO - #18: pr_root [nsprpub/pr/src/md/windows/w95thred.c:96]
07:03:10     INFO - 
07:03:10     INFO - #19: ucrtbase.DLL + 0x1cab0
07:03:10     INFO - 
07:03:10     INFO - #20: KERNEL32.DLL + 0x167e
07:03:10     INFO - 
07:03:10     INFO - #21: ntdll.dll + 0x1c3f1
Flags: needinfo?(jyavenard)
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e6fa8ff0d0be
P1. Proxify all MediaDataDecoder's APIs. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/16f9a685598b
P2. Let the MediaDataDecoder tells the format it wants. r=cpearce
Flags: needinfo?(jyavenard)
https://hg.mozilla.org/mozilla-central/rev/e6fa8ff0d0be
https://hg.mozilla.org/mozilla-central/rev/16f9a685598b
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.