AnnexB h264 videos aren't properly handled

RESOLVED FIXED in Firefox 36

Status

()

Core
Audio/Video
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jya, Assigned: jya)

Tracking

(Blocks: 1 bug)

Trunk
mozilla37
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox36 fixed, firefox37 fixed)

Details

Attachments

(11 attachments, 13 obsolete attachments)

1.61 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
30.59 KB, patch
Details | Diff | Splinter Review
5.11 KB, patch
Details | Diff | Splinter Review
5.13 KB, patch
Details | Diff | Splinter Review
3.90 KB, patch
Details | Diff | Splinter Review
15.18 KB, patch
kentuckyfriedtakahe
: review+
Details | Diff | Splinter Review
6.85 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
3.02 KB, patch
rillian
: review+
Details | Diff | Splinter Review
3.02 KB, patch
Details | Diff | Splinter Review
15.46 KB, patch
Details | Diff | Splinter Review
16.44 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

3 years ago
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.
(Assignee)

Updated

3 years ago
Blocks: 1105771
See Also: bug 1105771
(Assignee)

Comment 1

3 years ago
Created attachment 8537766 [details] [diff] [review]
Simplify MP4 extradata handling

Simplify handling of extra_data. This also totally obfuscate the inner details of the extra_data content
Attachment #8537766 - Flags: review?(ajones)
(Assignee)

Updated

3 years ago
Assignee: nobody → jyavenard
Status: NEW → ASSIGNED
(Assignee)

Comment 2

3 years ago
Created attachment 8537767 [details] [diff] [review]
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
Attachment #8537767 - Flags: review?(ajones)
(Assignee)

Comment 3

3 years ago
Created attachment 8537768 [details] [diff] [review]
Use more accurate names in MP4Sample and Annex B utility functions

Rename some functions and class members to properly reflect what they actually are
Attachment #8537768 - Flags: review?(ajones)
(Assignee)

Comment 4

3 years ago
Created attachment 8537771 [details] [diff] [review]
Add AVC Annex B to AVCC format conversion utility

Add AVCC to Annex B conversion routines
Attachment #8537771 - Flags: review?(ajones)
(Assignee)

Comment 5

3 years ago
Created attachment 8537772 [details] [diff] [review]
Add AnnexB support to FFmpeg h264 decoder

Add AnnexB support to FFmpeg decoder
Attachment #8537772 - Flags: review?(ajones)
(Assignee)

Comment 6

3 years ago
Created attachment 8537773 [details] [diff] [review]
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
Attachment #8537773 - Flags: review?(cpearce)
(Assignee)

Comment 7

3 years ago
Created attachment 8537775 [details] [diff] [review]
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
Attachment #8537775 - Flags: review?(cpearce)
(Assignee)

Comment 8

3 years ago
Created attachment 8537777 [details] [diff] [review]
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
Attachment #8537777 - Flags: review?(cpearce)
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.
Attachment #8537766 - Flags: review?(ajones) → review+
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.
Attachment #8537767 - Flags: review?(ajones) → review-
Attachment #8537768 - Flags: review?(ajones) → review+
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
Attachment #8537771 - Flags: review?(ajones) → review-
Attachment #8537772 - Flags: review?(ajones) → review+
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).
Attachment #8537773 - Flags: review?(cpearce) → review-
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!
Attachment #8537775 - Flags: review?(cpearce) → review+
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?
(Assignee)

Updated

3 years ago
Blocks: 1113073
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...
Attachment #8537777 - Flags: review?(cpearce) → review-
(Assignee)

Comment 16

3 years ago
Created attachment 8538837 [details] [diff] [review]
Simplify MP4 extradata handling

Fix compilation issue on B2G
Attachment #8538837 - Flags: review?(ajones)
(Assignee)

Updated

3 years ago
Attachment #8537766 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8538837 - Attachment is obsolete: true
Attachment #8538837 - Flags: review?(ajones)
(Assignee)

Comment 17

3 years ago
Created attachment 8538891 [details] [diff] [review]
Simplify MP4 extradata handling

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

Comment 18

3 years ago
(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
Flags: needinfo?(ajones)
(Assignee)

Comment 19

3 years ago
Created attachment 8539030 [details] [diff] [review]
Simplify MP4 extradata handling

Rename buffer class to ByteBuffer
(Assignee)

Updated

3 years ago
Attachment #8538891 - Attachment is obsolete: true
Attachment #8537767 - Flags: review- → review+
(Assignee)

Comment 20

3 years ago
(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.
(Assignee)

Comment 21

3 years ago
(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
Flags: needinfo?(ajones)
(Assignee)

Comment 22

3 years ago
Created attachment 8539769 [details] [diff] [review]
Simplify MP4 extradata handling

rebase
(Assignee)

Updated

3 years ago
Attachment #8539030 - Attachment is obsolete: true
(Assignee)

Comment 23

3 years ago
Created attachment 8539770 [details] [diff] [review]
Retrieve SPS and PPS from AVCC stream when necessary

rebase
(Assignee)

Updated

3 years ago
Attachment #8537767 - Attachment is obsolete: true
(Assignee)

Comment 24

3 years ago
Created attachment 8539771 [details] [diff] [review]
Use more accurate names in MP4Sample and Annex B utility functions

rebase... revert some changes
(Assignee)

Updated

3 years ago
Attachment #8537768 - Attachment is obsolete: true
(Assignee)

Comment 25

3 years ago
Created attachment 8539772 [details] [diff] [review]
Add AnnexB support to FFmpeg h264 decoder

rebase
(Assignee)

Updated

3 years ago
Attachment #8537772 - Attachment is obsolete: true
(Assignee)

Comment 26

3 years ago
Created attachment 8539773 [details] [diff] [review]
Add AVC Annex B to AVCC format conversion utility

:ajones
(Assignee)

Updated

3 years ago
Attachment #8537771 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8539773 - Flags: review?(ajones)
(Assignee)

Comment 27

3 years ago
Created attachment 8539774 [details] [diff] [review]
Make PlatformDecoderModule ref counted

Make PlatformDecoderModule ref counted.
Attachment #8539774 - Flags: review?(ajones)
(Assignee)

Comment 28

3 years ago
Created attachment 8539775 [details] [diff] [review]
Create AVCC converter wrapper class

Carrying r+ changes, rework memory allocation
Attachment #8539775 - Flags: review?(cpearce)
(Assignee)

Updated

3 years ago
Attachment #8537773 - Attachment is obsolete: true
(Assignee)

Comment 29

3 years ago
Created attachment 8539776 [details] [diff] [review]
Use AVCC wrapper for EME. Adds AnnexB and AVC3 support

Small changes, rename PlatformDecoderModule member to DecoderNeedsAVCC, it's more descriptive
Attachment #8539776 - Flags: review?(cpearce)
(Assignee)

Updated

3 years ago
Attachment #8537777 - Attachment is obsolete: true
(Assignee)

Comment 30

3 years ago
Created attachment 8539777 [details] [diff] [review]
Use AVCC wrapper for EME. Adds AnnexB and AVC3 support

Small changes, rename PlatformDecoderModule member to DecoderNeedsAVCC, it's more descriptive.
Attachment #8539777 - Flags: review?(cpearce)
(Assignee)

Updated

3 years ago
Attachment #8539776 - Attachment is obsolete: true
Attachment #8539776 - Flags: review?(cpearce)
(Assignee)

Comment 31

3 years ago
Created attachment 8539779 [details] [diff] [review]
Use ref counted pointers to hold references in mac decoder

Mac decoders should use nsRefPtr and not hold directly the pointers
Attachment #8539779 - Flags: review?(giles)
(Assignee)

Comment 33

3 years ago
Created attachment 8539788 [details] [diff] [review]
Use ref counted pointers to hold references in mac decoder
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.
Attachment #8539775 - Flags: review?(cpearce) → review+
Attachment #8539777 - Flags: review?(cpearce) → review+
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.
Attachment #8539773 - Flags: review?(ajones) → review+
Attachment #8539774 - Flags: review?(ajones) → review+
(Assignee)

Comment 36

3 years ago
(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...
(Assignee)

Comment 37

3 years ago
Created attachment 8540118 [details] [diff] [review]
Add AVC Annex B to AVCC format conversion utility

Carrying r+ nit
(Assignee)

Updated

3 years ago
Attachment #8539773 - Attachment is obsolete: true
(Assignee)

Comment 38

3 years ago
Created attachment 8540120 [details] [diff] [review]
Create AVCC converter wrapper class

Carrying r+
(Assignee)

Updated

3 years ago
Attachment #8539775 - Attachment is obsolete: true
Attachment #8539779 - Flags: review?(giles) → review+
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.
Attachment #8539769 - Flags: approval-mozilla-aurora?
status-firefox36: --- → affected
status-firefox37: --- → fixed
Attachment #8539769 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.