Closed
Bug 1339748
Opened 7 years ago
Closed 7 years ago
EMEDecoderModule doesn't need AVCC conversion
Categories
(Core :: Audio/Video: Playback, defect, P3)
Core
Audio/Video: Playback
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 hidden (mozreview-request) |
Assignee | ||
Comment 2•7 years ago
|
||
mozreview-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 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+
Reporter | ||
Comment 3•7 years ago
|
||
mozreview-review-reply |
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?
Comment hidden (mozreview-request) |
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
Comment 6•7 years ago
|
||
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)
Assignee | ||
Comment 7•7 years ago
|
||
mozreview-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/#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
Reporter | ||
Comment 8•7 years ago
|
||
mozreview-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/#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?
Assignee | ||
Comment 9•7 years ago
|
||
mozreview-review-reply |
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..
Reporter | ||
Comment 10•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1569e2d773b
Reporter | ||
Comment 11•7 years ago
|
||
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)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 13•7 years ago
|
||
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
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 17•7 years ago
|
||
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.
Assignee | ||
Updated•7 years ago
|
Attachment #8837493 -
Attachment is obsolete: true
Reporter | ||
Comment 18•7 years ago
|
||
mozreview-review |
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+
Reporter | ||
Comment 19•7 years ago
|
||
mozreview-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+
Reporter | ||
Comment 20•7 years ago
|
||
(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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8838762 -
Attachment is obsolete: true
Attachment #8838762 -
Flags: review?(jyavenard)
Comment 23•7 years ago
|
||
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
Comment 24•7 years ago
|
||
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)
Comment hidden (mozreview-request) |
Comment 26•7 years ago
|
||
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
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(jyavenard)
Comment 27•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e6fa8ff0d0be https://hg.mozilla.org/mozilla-central/rev/16f9a685598b
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•