Closed Bug 1341894 Opened 3 years ago Closed 3 years ago

Make AnnexB::ConvertExtraDataToAnnexB() correct crypto subsample offsets

Categories

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

defect

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 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+
(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.
Pushed by cpearce@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6a5f53588952
Correct encryption subsamples in AVCC -> AnnexB conversion. r=jya
https://hg.mozilla.org/mozilla-central/rev/6a5f53588952
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Depends on: 1451681
You need to log in before you can comment on or make changes to this bug.