1110534 - AnnexB h264 videos aren't properly handled

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Jean-Yves Avenard [:jya]]

the mp4 demuxer converts AVCC into AnnexB ; however that conversion is done even if the media is already AnnexB, resulting in unplayable content.

No conversion should be performed if the h264 is already AnnexB.
The decoder also needs to be able to report if it can handle AnnexB, at this time all do but the mac hardware decoder. So AnnexB needs to be either disabled on mac or conversion from AnnexB to AVCC needs to be added.

Additionally, if the mp4's edts atom contains an empty or no extradata, the annexB conversion will crash in AnnexB::ConvertSample:
    aSample->Prepend(&(*aSample->prefix_data)[0],
                     aSample->prefix_data->Length());

as prefix_data->Length() is 0, attempting to use prefix_data[0] will assert.

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: Simplify MP4 extradata handling

Simplify handling of extra_data. This also totally obfuscate the inner details of the extra_data content

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: Retrieve SPS and PPS from AVCC stream when necessary

Add methods to parse a mdat and AVCC extradata  and extract the SPS and PPS content

Comment 3

[Jean-Yves Avenard [:jya]]

Attachment: Use more accurate names in MP4Sample and Annex B utility functions

Rename some functions and class members to properly reflect what they actually are

Comment 4

[Jean-Yves Avenard [:jya]]

Attachment: Add AVC Annex B to AVCC format conversion utility

Add AVCC to Annex B conversion routines

Comment 5

[Jean-Yves Avenard [:jya]]

Attachment: Add AnnexB support to FFmpeg h264 decoder

Add AnnexB support to FFmpeg decoder

Comment 6

[Jean-Yves Avenard [:jya]]

Attachment: Create AVCC converter wrapper class

Add AVCCDecoderModule class. This acts as a wrapper to ensuire that the underlying PlatformDecoderModule is only fed AVCC data, and is initialized only when safe to do so. This simplify the addition of AnnexB and AVC3 support for decoder that can't deal with those (EME and mac VDA/VideoToolbox) without having to modify them

Comment 7

[Jean-Yves Avenard [:jya]]

Attachment: Use AVCC wrapper for mac decoder. Adds AnnexB and AVC3 support

Make mac decoder use the AVCC wrapper. Allows the mac to handle AnnexB and AVC3

Comment 8

[Jean-Yves Avenard [:jya]]

Attachment: Use AVCC wrapper for EME. Adds AnnexB and AVC3 support

Make EME decoder use AVCC wrapper. As the EME decoder is itself a wrapper, a method is added so we can de-activate with a finer degree of control when we need to encapsulate the video decoder

Comment 9

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Comment on attachment 8537766 [details] [diff] [review]
Simplify MP4 extradata handling

Review of attachment 8537766 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/libstagefright/binding/include/mp4_demuxer/DecoderData.h
@@ +30,5 @@
>    ~nsRcTArray() {}
>  };
>  
> +typedef nsRcTArray<uint8_t> MP4CharArray;
> +typedef nsRefPtr<MP4CharArray> MP4CharArrayPtr;

I don't like these typedefs. The issues include:

* We shouldn't prefix with MP4 because we want to move away from MP4 specific stuff in this interface
* It is not a char. It is a octet, byte or uint8_t.
* typedefs create an extra level of indirection to figure out the real type

The problem you appear to be trying to address is that nsRefPtr<nsRcTArray<uint8_t>> is a lot of text to parse. Perhaps we could just do:

    typedef nsRcTArray<uint8_t> Buffer;

We can make the arguments to the AnnexB functions simply Buffer* on the basis that incrementing/decrementing the reference count doesn't add any value. Alternatively nsRefPtr<Buffer>& would achieve the same objective.

Comment 10

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Comment on attachment 8537767 [details] [diff] [review]
Retrieve SPS and PPS from AVCC stream when necessary

Review of attachment 8537767 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/libstagefright/binding/AnnexB.cpp
@@ +27,5 @@
>    MOZ_ASSERT(aSample->extra_data);
>  
>    uint8_t* d = aSample->data;
>    while (d + 4 < aSample->data + aSample->size) {
> +    uint32_t nalLen = mozilla::BigEndian::readUint32(d);

Comment: Good drive-by fix; this probably should've use ByteReader too.

@@ +103,5 @@
> +
> +  // Find SPS and PPS NALUs in AVCC data
> +  uint8_t* d = aSample->data;
> +  while (d + 4 < aSample->data + aSample->size) {
> +    uint32_t nalLen = mozilla::BigEndian::readUint32(d);

Please use ByteReader for reading aSample->data

@@ +116,5 @@
> +      numPps++;
> +      uint8_t val[2];
> +      mozilla::BigEndian::writeInt16(&val[0], nalLen);
> +      pps.append(&val[0], 2); // 16 bits size
> +      pps.append(d + 4, nalLen);

We could be reading beyond the end of the buffer here. ByteReader protects against this.

Comment 11

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Comment on attachment 8537771 [details] [diff] [review]
Add AVC Annex B to AVCC format conversion utility

Review of attachment 8537771 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/libstagefright/binding/AnnexB.cpp
@@ +195,5 @@
> +
> +  mozilla::Vector<uint8_t> nalu;
> +  ByteWriter writer(nalu);
> +
> +  avc_parse_nal_units(writer, aSample->data, aSample->size);

We should be handling aSample->data with ByteReader instead of using raw pointers so we don't have to deal with running off the end of the buffer. This includes avc_find_startcode and avc_find_startcode_internal. We may need a different abstraction but we shouldn't have raw pointers floating around the code that understands avc.

::: media/libstagefright/binding/include/mp4_demuxer/ByteReader.h
@@ +159,5 @@
>    const uint8_t* mPtr;
>    size_t mRemaining;
>  };
> +
> +class ByteWriter

This should go in ByteWriter.h

Comment 12

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8537773 [details] [diff] [review]
Create AVCC converter wrapper class

Review of attachment 8537773 [details] [diff] [review]:
-----------------------------------------------------------------

Not too bad, but we should go another round.

::: dom/media/fmp4/AVCCDecoderModule.cpp
@@ +7,5 @@
> +#include "AVCCDecoderModule.h"
> +#include "mp4_demuxer/DecoderData.h"
> +#include "mp4_demuxer/AnnexB.h"
> +
> +namespace mozilla

nit:

namespace mozilla {

Braces riding high for namespaces.

@@ +37,5 @@
> +
> +nsresult
> +AVCCMediaDataDecoder::Init()
> +{
> +  mCurrentConfig = new mp4_demuxer::VideoDecoderConfig;

It seems unnecessary to allocate a second VideoDecoderConfig here. Why don't you just make AVCCMediaDataDecoder::mConfig not a reference, but instead a non-const member of AVCCMediaDataDecoder, and initialize it as a copy of aConfig in the constructor?

As is, you won't pass the const AVCCMediaDataDecoder::mConfig reference to the wrapper decoder unless the MP4Reader doesn't follow the normal patttern of PDM::CreateVideoDecoder(), Init(), Input(), right? So it seems unnecessary to manage a separate VideoDecoderConfig in this class.

Is there a reason why that won't work? You don't change the config once you've passed it to the video decoder right?

@@ +56,5 @@
> +    // Creation will fail if we couldn't extract the SPS from aSample.
> +    nsresult rv = CreateDecoderAndInit();
> +    if (rv == NS_ERROR_NOT_INITIALIZED) {
> +      // We are missing the required SPS to create the decoder.
> +      // Ignore for the time being, the MP4Sample will be dropped.

Might want to mention in this comment that there's no way the sample can be decoded given the input anyway (right?)... Unless we were to parse an unknown amount of the stream ahead of the current position.

@@ +86,5 @@
> +
> +nsresult
> +AVCCMediaDataDecoder::Shutdown()
> +{
> +  delete mCurrentConfig;

If I hadn't told you to make mConfig a copy instead of a reference, I'd instead be telling you here not to use manual memory management, unless there's a good reason. Use an nsAutoPtr if you want to `new` something.

::: dom/media/fmp4/AVCCDecoderModule.h
@@ +10,5 @@
> +#include "PlatformDecoderModule.h"
> +
> +namespace mozilla {
> +
> +class AVCCMediaDataDecoder : public MediaDataDecoder {

The definition of AVCCMediaDataDecoder doesn't need to be in the .h file, since it just implements another public interface, right?

@@ +45,5 @@
> +  PlatformDecoderModule* mPDM;
> +  const mp4_demuxer::VideoDecoderConfig& mConfig;
> +  mp4_demuxer::VideoDecoderConfig* mCurrentConfig;
> +  layers::LayersBackend mLayersBackend;
> +  layers::ImageContainer* mImageContainer;

The raw pointers to the task queue and the image container should be nsRefPtrs. You might need to clear the task queue refptr in your Shutdown() implementation to break cycles.

@@ +52,5 @@
> +  nsRefPtr<MediaDataDecoder> mDecoder;
> +  nsresult mLastError;
> +};
> +
> +class AVCCDecoderModule : public PlatformDecoderModule {

You should have a comment here briefly explaining what this does and more importantly why.

::: dom/media/fmp4/PlatformDecoderModule.h
@@ +67,5 @@
>    static PlatformDecoderModule* Create();
>  
> +  // Perform any per-instance initialization.
> +  // Main thread only.
> +  virtual nsresult Startup() { return NS_OK; };

If we're going to add this to the general interface, we should call this on all PDMs we create inside PlatformDecoderModule::Create(), instead of doing it inside the platform specific #define blocks.

And it runs on the decode task queue; see the assertion in PlatformDecoderModule::Create()? So the comment here about Main thread only is wrong.

Also please add MOZ_OVERRIDE to the other implementers of this function (WMF and Apple PDMs).

Comment 13

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8537775 [details] [diff] [review]
Use AVCC wrapper for mac decoder. Adds AnnexB and AVC3 support

Review of attachment 8537775 [details] [diff] [review]:
-----------------------------------------------------------------

Easy!

Comment 14

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8537777 [details] [diff] [review]
Use AVCC wrapper for EME. Adds AnnexB and AVC3 support

Review of attachment 8537777 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/fmp4/PlatformDecoderModule.cpp
@@ +101,5 @@
>        return nullptr;
>      }
>    }
>  
> +  return new AVCCDecoderModule(new EMEDecoderModule(aProxy,

Can't you only apply the wrapper if cdmDecodeVideo is true, rather than adding PDM::CanUseAVCCDecoderModule()?

::: dom/media/fmp4/eme/EMEDecoderModule.cpp
@@ +260,5 @@
>  
> +bool
> +EMEDecoderModule::CanUseAVCCDecoderModule(const mp4_demuxer::VideoDecoderConfig& aConfig)
> +{
> +  return mCDMDecodesVideo && aConfig.crypto.valid;

Why is aConfig.crypto.valid relevant here?

Comment 15

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8537777 [details] [diff] [review]
Use AVCC wrapper for EME. Adds AnnexB and AVC3 support

Review of attachment 8537777 [details] [diff] [review]:
-----------------------------------------------------------------

If what I suggested (only wrapping the EME PDM if cdmDecodeVideo is true) doesn't work, re-request review...

Comment 16

[Jean-Yves Avenard [:jya]]

Attachment: Simplify MP4 extradata handling

Fix compilation issue on B2G

Comment 17

[Jean-Yves Avenard [:jya]]

Attachment: Simplify MP4 extradata handling

Carrying r+, rename MP4CharBuffer to simply Buffer and use pointers instead of nsRefPtr where possible

Comment 18

[Jean-Yves Avenard [:jya]]

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #10)
> Comment on attachment 8537767 [details] [diff] [review]
> Retrieve SPS and PPS from AVCC stream when necessary
> 
> Review of attachment 8537767 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/libstagefright/binding/AnnexB.cpp
> @@ +27,5 @@
> >    MOZ_ASSERT(aSample->extra_data);
> >  
> >    uint8_t* d = aSample->data;
> >    while (d + 4 < aSample->data + aSample->size) {
> > +    uint32_t nalLen = mozilla::BigEndian::readUint32(d);
> 
> Comment: Good drive-by fix; this probably should've use ByteReader too.
> 
> @@ +103,5 @@
> > +
> > +  // Find SPS and PPS NALUs in AVCC data
> > +  uint8_t* d = aSample->data;
> > +  while (d + 4 < aSample->data + aSample->size) {
> > +    uint32_t nalLen = mozilla::BigEndian::readUint32(d);
> 
> Please use ByteReader for reading aSample->data
> 
> @@ +116,5 @@
> > +      numPps++;
> > +      uint8_t val[2];
> > +      mozilla::BigEndian::writeInt16(&val[0], nalLen);
> > +      pps.append(&val[0], 2); // 16 bits size
> > +      pps.append(d + 4, nalLen);
> 
> We could be reading beyond the end of the buffer here. ByteReader protects
> against this.

This is done in the 4Bytes patch in bug 1113073 (which you have r+ already)...
Here I followed the existing style, using existing functions..

To do what you ask requires extra functionality to the ByteReader which I felt were out of scope for this particular change.

Please reconsider the r- in light of this.
thanks

Comment 19

[Jean-Yves Avenard [:jya]]

Attachment: Simplify MP4 extradata handling

Rename buffer class to ByteBuffer

Comment 20

[Jean-Yves Avenard [:jya]]

(In reply to Chris Pearce (:cpearce) from comment #12)
> Comment on attachment 8537773 [details] [diff] [review]
> Create AVCC converter wrapper class
> 
> Review of attachment 8537773 [details] [diff] [review]:
> -----------------------------------------------------------------
> namespace mozilla {
> 
> Braces riding high for namespaces.

In the fmp4 folder I see both style... with the brace on the same line in most cases. So it's not easy to find out which way to go for new files!

> 
> @@ +37,5 @@
> > +
> > +nsresult
> > +AVCCMediaDataDecoder::Init()
> > +{
> > +  mCurrentConfig = new mp4_demuxer::VideoDecoderConfig;
> 
> It seems unnecessary to allocate a second VideoDecoderConfig here. Why don't
> you just make AVCCMediaDataDecoder::mConfig not a reference, but instead a
> non-const member of AVCCMediaDataDecoder, and initialize it as a copy of
> aConfig in the constructor?

This was to minimize memory usage, as insignificant as it may be; as Init may not always be called depending if the wrapping is active or not.

> 
> As is, you won't pass the const AVCCMediaDataDecoder::mConfig reference to
> the wrapper decoder unless the MP4Reader doesn't follow the normal patttern
> of PDM::CreateVideoDecoder(), Init(), Input(), right? So it seems
> unnecessary to manage a separate VideoDecoderConfig in this class.

Actually, my long term aim is to actually manage on the fly changes. The AVCC wrapper will take care of monitoring the type of data flowing, and changes in the SPS (just like AnnexB decoder are supposed to do).

So if we detect a change then, the AVCC wrapper will destroy the current decoder and create a new one, handling the changes of decoder transparently. I have another patch adding this capability.
To do so, I need a separate VideoDecoderConfig on its own that keeps the current configuration.

Right now, it's only used once.. but that will change soon.

> 
> Is there a reason why that won't work? You don't change the config once
> you've passed it to the video decoder right?

right now, yes.. It won't change once we've found the SPS and created the decoder. But this is only temporary as described above.

> Might want to mention in this comment that there's no way the sample can be
> decoded given the input anyway (right?)... Unless we were to parse an
> unknown amount of the stream ahead of the current position.

ok will amend the comment.

> 
> If I hadn't told you to make mConfig a copy instead of a reference, I'd
> instead be telling you here not to use manual memory management, unless
> there's a good reason. Use an nsAutoPtr if you want to `new` something.

I actually had done so already...

> > +namespace mozilla {
> > +
> > +class AVCCMediaDataDecoder : public MediaDataDecoder {
> 
> The definition of AVCCMediaDataDecoder doesn't need to be in the .h file,
> since it just implements another public interface, right?

yes... So you prefer that the definition be moved to inside the AVCCMediaDecoder.cpp ?
can do...

Matter of habit I guess. I like all my class definitions to be in .h... But yeah, here it certainly doesn't need to be.

> 
> @@ +45,5 @@
> > +  PlatformDecoderModule* mPDM;
> > +  const mp4_demuxer::VideoDecoderConfig& mConfig;
> > +  mp4_demuxer::VideoDecoderConfig* mCurrentConfig;
> > +  layers::LayersBackend mLayersBackend;
> > +  layers::ImageContainer* mImageContainer;
> 
> The raw pointers to the task queue and the image container should be
> nsRefPtrs. You might need to clear the task queue refptr in your Shutdown()
> implementation to break cycles.

I actually changed that and modified PlatformDecoderModule to be RefCounted because it turned out that the PlatformDecoderModule is deleted before the MediaDataDecoder ; which caused crashes as the AVCCMediaDataDecoder was still referencing it.

Was easier to make PlatformDecoderModule ref counted than track how MP4Reader managed its objects.

Which task queue pointer ?

> > +class AVCCDecoderModule : public PlatformDecoderModule {
> 
> You should have a comment here briefly explaining what this does and more
> importantly why.

yes.

> If we're going to add this to the general interface, we should call this on
> all PDMs we create inside PlatformDecoderModule::Create(), instead of doing
> it inside the platform specific #define blocks.

good idea.

> 
> And it runs on the decode task queue; see the assertion in
> PlatformDecoderModule::Create()? So the comment here about Main thread only
> is wrong.

right..

I copied that comment from the AppleDecoderModule Startup() instance.
Will fix that there too.

> 
> Also please add MOZ_OVERRIDE to the other implementers of this function (WMF
> and Apple PDMs).

deal.

Comment 21

[Jean-Yves Avenard [:jya]]

(In reply to Chris Pearce (:cpearce) (PTO until 5 Jan) from comment #14)
> Comment on attachment 8537777 [details] [diff] [review]
> Use AVCC wrapper for EME. Adds AnnexB and AVC3 support
> 
> Review of attachment 8537777 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/fmp4/PlatformDecoderModule.cpp
> @@ +101,5 @@
> >        return nullptr;
> >      }
> >    }
> >  
> > +  return new AVCCDecoderModule(new EMEDecoderModule(aProxy,
> 
> Can't you only apply the wrapper if cdmDecodeVideo is true, rather than
> adding PDM::CanUseAVCCDecoderModule()?

As the wrapper wraps the PDM, it's there that I need to know if the decoder will require AVCC data or not.
And I wont know that at the initialisation time of the PDM, only at the time of creation of the MediaDataDecoder

> 
> ::: dom/media/fmp4/eme/EMEDecoderModule.cpp
> @@ +260,5 @@
> >  
> > +bool
> > +EMEDecoderModule::CanUseAVCCDecoderModule(const mp4_demuxer::VideoDecoderConfig& aConfig)
> > +{
> > +  return mCDMDecodesVideo && aConfig.crypto.valid;
> 
> Why is aConfig.crypto.valid relevant here?

Because those are the conditions required to create the EMEH264Decoder which needs AVCC.
The two others MediaDataDecoder that can be created do not...

In CreateVideoDecoder we have:
  if (mCDMDecodesVideo && aConfig.crypto.valid) {
    nsRefPtr<MediaDataDecoder> decoder(new EMEH264Decoder(mProxy,
                                                          aConfig,
                                                          aLayersBackend,
                                                          aImageContainer,
                                                          aVideoTaskQueue,
                                                          aCallback));
    return decoder.forget();
  }

So if we have mCDMDecodesVideo and aConfig.crypto.valid, then we want the AVCC wrapper active, and disabled otherwise.

I'm not sure what you mean by cdmDecodeVideo.

So in all, I don't see how else I need to do without doing like what I've just done

Comment 22

[Jean-Yves Avenard [:jya]]

Attachment: Simplify MP4 extradata handling

rebase

Comment 23

[Jean-Yves Avenard [:jya]]

Attachment: Retrieve SPS and PPS from AVCC stream when necessary

rebase

Comment 24

[Jean-Yves Avenard [:jya]]

Attachment: Use more accurate names in MP4Sample and Annex B utility functions

rebase... revert some changes

Comment 25

[Jean-Yves Avenard [:jya]]

Attachment: Add AnnexB support to FFmpeg h264 decoder

rebase

Comment 26

[Jean-Yves Avenard [:jya]]

Attachment: Add AVC Annex B to AVCC format conversion utility

:ajones

Comment 27

[Jean-Yves Avenard [:jya]]

Attachment: Make PlatformDecoderModule ref counted

Make PlatformDecoderModule ref counted.

Comment 28

[Jean-Yves Avenard [:jya]]

Attachment: Create AVCC converter wrapper class

Carrying r+ changes, rework memory allocation

Comment 29

[Jean-Yves Avenard [:jya]]

Attachment: Use AVCC wrapper for EME. Adds AnnexB and AVC3 support

Small changes, rename PlatformDecoderModule member to DecoderNeedsAVCC, it's more descriptive

Comment 30

[Jean-Yves Avenard [:jya]]

Attachment: Use AVCC wrapper for EME. Adds AnnexB and AVC3 support

Small changes, rename PlatformDecoderModule member to DecoderNeedsAVCC, it's more descriptive.

Comment 31

[Jean-Yves Avenard [:jya]]

Attachment: Use ref counted pointers to hold references in mac decoder

Mac decoders should use nsRefPtr and not hold directly the pointers

Comment 32

[Jean-Yves Avenard [:jya]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f661f8b7ff7c

Comment 33

[Jean-Yves Avenard [:jya]]

Attachment: Use ref counted pointers to hold references in mac decoder

Comment 34

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8539775 [details] [diff] [review]
Create AVCC converter wrapper class

Review of attachment 8539775 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with nits. The AnnexB.cpp change should be moved to another patch for review by ajones.

::: dom/media/fmp4/AVCCDecoderModule.cpp
@@ +88,5 @@
> +AVCCMediaDataDecoder::Input(mp4_demuxer::MP4Sample* aSample)
> +{
> +  mp4_demuxer::AnnexB::ConvertSampleToAVCC(aSample);
> +  if (!mDecoder) {
> +    mCurrentConfig.extra_data =

Is it possible for the extra_data to already be valid here, and that this operation overwrites a valid extra_data with an invalid extra_data?

If so, please fix before landing.

::: dom/media/fmp4/PlatformDecoderModule.cpp
@@ +116,5 @@
>    MOZ_ASSERT(!NS_IsMainThread());
>  
> +  nsRefPtr<PlatformDecoderModule> m;
> +
> +  while (true) {

I would prefer a to have a new sub-function that just creates and returns the PDM, rather than adding a loop that doesn't actually loop.

i.e.:

nsRefPtr<PDM> m(CreatePDM());
if (m && NS_SUCCEEDED(m->Init())) {
  return m.forget();
}

::: dom/media/fmp4/apple/AppleDecoderModule.h
@@ +15,5 @@
>  public:
>    AppleDecoderModule();
>    virtual ~AppleDecoderModule();
>  
> +  nsresult Startup() MOZ_OVERRIDE;

virtual nsresult Startup() MOZ_OVERRIDE;

The style guide dictates that we're supposed to add "virtual" keyword even when it's unnecessary, for documentation purposes..

::: dom/media/fmp4/wmf/WMFDecoderModule.h
@@ +16,5 @@
>    WMFDecoderModule();
>    virtual ~WMFDecoderModule();
>  
>    // Initializes the module, loads required dynamic libraries, etc.
> +  nsresult Startup() MOZ_OVERRIDE;

virtual nsresult Startup() MOZ_OVERRIDE;

The style guide dictates that we're supposed to add "virtual" keyword even when it's unnecessary, for documentation purposes..

::: media/libstagefright/binding/AnnexB.cpp
@@ +112,5 @@
>  
>    while (aBr.Remaining() >= 6) {
>      uint32_t x32 = aBr.PeekU32();
>      if ((x32 - 0x01010101) & (~x32) & 0x80808080) {
> +      if ((x32 >> 8) == 0x000001) {

This change wasn't in the previous patch, is it supposed to be here? You should get ajones to review this.

Comment 35

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Comment on attachment 8539773 [details] [diff] [review]
Add AVC Annex B to AVCC format conversion utility

Review of attachment 8539773 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/libstagefright/binding/include/mp4_demuxer/ByteReader.h
@@ +155,5 @@
> +  {
> +    size_t rewind = mLength - mRemaining;
> +    if (aCount < rewind) {
> +      rewind = aCount;
> +    }

suggestion: This could be expressed as std::min(aCount, Offset()); We may want MOZ_ASSERT(aCount >= Offset()); because it is most likely to be in error.

Comment 36

[Jean-Yves Avenard [:jya]]

(In reply to Chris Pearce (:cpearce) (PTO until 5 Jan) from comment #34)
> Comment on attachment 8539775 [details] [diff] [review]
> Create AVCC converter wrapper class
> 
> Review of attachment 8539775 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with nits. The AnnexB.cpp change should be moved to another patch for
> review by ajones.

yeah. that's an incorrect refresh, shouldn't be there...

> 
> ::: dom/media/fmp4/AVCCDecoderModule.cpp
> @@ +88,5 @@
> > +AVCCMediaDataDecoder::Input(mp4_demuxer::MP4Sample* aSample)
> > +{
> > +  mp4_demuxer::AnnexB::ConvertSampleToAVCC(aSample);
> > +  if (!mDecoder) {
> > +    mCurrentConfig.extra_data =
> 
> Is it possible for the extra_data to already be valid here, and that this
> operation overwrites a valid extra_data with an invalid extra_data?
> 
> If so, please fix before landing.

no.. If the extra_data was valid then mDecoder would be set and would have been created in the AVCCMediaDataDecoder constructor.

> I would prefer a to have a new sub-function that just creates and returns
> the PDM, rather than adding a loop that doesn't actually loop.

yes... you're right..

brain fart on my part, I just went for a quick change

> ::: media/libstagefright/binding/AnnexB.cpp
> @@ +112,5 @@
> >  
> >    while (aBr.Remaining() >= 6) {
> >      uint32_t x32 = aBr.PeekU32();
> >      if ((x32 - 0x01010101) & (~x32) & 0x80808080) {
> > +      if ((x32 >> 8) == 0x000001) {
> 
> This change wasn't in the previous patch, is it supposed to be here? You
> should get ajones to review this.

yes, this definitely shouldn't be there, and is supposed to be in the AnnexB -> AVCC conversion...

Comment 37

[Jean-Yves Avenard [:jya]]

Attachment: Add AVC Annex B to AVCC format conversion utility

Carrying r+ nit

Comment 38

[Jean-Yves Avenard [:jya]]

Attachment: Create AVCC converter wrapper class

Carrying r+

Comment 39

[Jean-Yves Avenard [:jya]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/40121032ab82
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/57cd7a69c1d9
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e881424890d4
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f0c017e958b3
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3e84e08fef30
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/309c39e9bd09
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/132c98cc7e95
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8b50d291a835
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e3cc156a56d6
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3140742e85dd

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/40121032ab82
https://hg.mozilla.org/mozilla-central/rev/57cd7a69c1d9
https://hg.mozilla.org/mozilla-central/rev/e881424890d4
https://hg.mozilla.org/mozilla-central/rev/f0c017e958b3
https://hg.mozilla.org/mozilla-central/rev/3e84e08fef30
https://hg.mozilla.org/mozilla-central/rev/309c39e9bd09
https://hg.mozilla.org/mozilla-central/rev/132c98cc7e95
https://hg.mozilla.org/mozilla-central/rev/8b50d291a835
https://hg.mozilla.org/mozilla-central/rev/e3cc156a56d6
https://hg.mozilla.org/mozilla-central/rev/3140742e85dd

Comment 41

[Ralph Giles (:rillian)]

Comment on attachment 8539769 [details] [diff] [review]
Simplify MP4 extradata handling

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Less consistent testing, sites more likely so serve flash video.
[Describe test coverage new/current, TBPL]: Landed on m-c.
[Risks and why]: This isn't MSE-specific, so risk is moderate. Regressions should be obvious though since it affects decoder setup.
[String/UUID change made/needed]: None

This approval request is for all patches in this bug.

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f08048b1b169
https://hg.mozilla.org/releases/mozilla-aurora/rev/c59838b615a4
https://hg.mozilla.org/releases/mozilla-aurora/rev/0ce3d41c3201
https://hg.mozilla.org/releases/mozilla-aurora/rev/85a568cecd96
https://hg.mozilla.org/releases/mozilla-aurora/rev/d1682329ba58
https://hg.mozilla.org/releases/mozilla-aurora/rev/4ec9cd2be4f5
https://hg.mozilla.org/releases/mozilla-aurora/rev/ab63a3d5f7cd
https://hg.mozilla.org/releases/mozilla-aurora/rev/5a2e956530c5
https://hg.mozilla.org/releases/mozilla-aurora/rev/4d5fa078ae74
https://hg.mozilla.org/releases/mozilla-aurora/rev/6c63735d2244