Closed
Bug 1341894
Opened 7 years ago
Closed 7 years ago
Make AnnexB::ConvertExtraDataToAnnexB() correct crypto subsample offsets
Categories
(Core :: Audio/Video: Playback, defect, P3)
Core
Audio/Video: Playback
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: cpearce, Assigned: cpearce)
References
Details
Attachments
(1 file)
When AnnexB::ConvertExtraDataToAnnexB() prepends the Annex B NAL with SPS/PPS to keyframes, it will mess up the crypto subsample offsets. Currently the decrypting decoders correct for this manually, but it would be nicer if our conversion code does the entire conversion.
Comment hidden (mozreview-request) |
Comment 2•7 years ago
|
||
mozreview-review |
Comment on attachment 8840154 [details] Bug 1341894 - Correct encryption subsamples in AVCC -> AnnexB conversion. https://reviewboard.mozilla.org/r/114652/#review116268 ::: dom/media/platforms/agnostic/gmp/GMPVideoDecoder.cpp:235 (Diff revision 1) > // to complete. > aGMP->Close(); > return; > } > > + bool isOpenH264 = aGMP->GetDisplayName().EqualsLiteral("gmpopenh264"); Would be nicer if the GMP itself told what it wanted. ::: dom/media/platforms/agnostic/gmp/GMPVideoDecoder.cpp:329 (Diff revision 1) > } > > mLastStreamOffset = sample->mOffset; > > + if (mConvertToAnnexB) { > + mp4_demuxer::AnnexB::ConvertSampleToAnnexB(sample); Why not do that im the WidevineVideoDecoder instead? That seems nicer conceptually to me. In any case you dont need to perform the conversion here, let the H264Converter do it: GMPVideoDecoder::ConversionNeeded const { return mConvertToAnnexB ? kNeedAnnexB ... } even better would be to extend the GMP interface to let the GMP report what it needs, and pass that straight through
Attachment #8840154 -
Flags: review?(jyavenard) → review+
Assignee | ||
Comment 3•7 years ago
|
||
(In reply to Jean-Yves Avenard [:jya] from comment #2) > Comment on attachment 8840154 [details] > Bug 1341894 - Correct encryption subsamples in AVCC -> AnnexB conversion. > > https://reviewboard.mozilla.org/r/114652/#review116268 > > ::: dom/media/platforms/agnostic/gmp/GMPVideoDecoder.cpp:235 > (Diff revision 1) > > // to complete. > > aGMP->Close(); > > return; > > } > > > > + bool isOpenH264 = aGMP->GetDisplayName().EqualsLiteral("gmpopenh264"); > > Would be nicer if the GMP itself told what it wanted. Once I've landed bug 1315850 we'll have separate interfaces for the CDM and OpenH264 decoding. So we won't need this hack, we can just assume GMPs take pseudo-AVCC and CDMs take AnnexB. > > ::: dom/media/platforms/agnostic/gmp/GMPVideoDecoder.cpp:329 > (Diff revision 1) > > } > > > > mLastStreamOffset = sample->mOffset; > > > > + if (mConvertToAnnexB) { > > + mp4_demuxer::AnnexB::ConvertSampleToAnnexB(sample); > > Why not do that im the WidevineVideoDecoder instead? > > That seems nicer conceptually to me. Then in the WidevineVideoDecoder we need to copy the data out of the buffer we use to shuffle it over IPC into a MediaRawData just so it can be converted. While the sample is in the Gecko process it's already in a MediaRawData. And we'd also need to expose the conversion code to the WidevineVideoDecoder too. This way is less code, and less exposure of Gecko internals to the Widevine code. > > In any case you dont need to perform the conversion here, let the > H264Converter do it: > > GMPVideoDecoder::ConversionNeeded const > { > return mConvertToAnnexB ? kNeedAnnexB ... > } > This works. I'll use that.
Comment hidden (mozreview-request) |
Pushed by cpearce@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6a5f53588952 Correct encryption subsamples in AVCC -> AnnexB conversion. r=jya
Comment 6•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6a5f53588952
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
•