1150853 - Replace MP4Sample with a MediaRawData refcounted object

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Jean-Yves Avenard [:jya]]

this bug track the work of replacing the MP4Sample object with a new refcounted MediaRawData.

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: Part1. Create MediaRawData object type

Add MediaRawData object and generalise the use of CryptoTrack and CryptoSample

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: Part2. Use new MediaRawObject across the board

Use MediaRawData across the board, remove MP4Sample object

Comment 3

[Jean-Yves Avenard [:jya]]

Attachment: Part3. Do not modify provided extradata array

make a copy of extradata object as ffmpeg decoder modifies it

Comment 4

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Comment on attachment 8588922 [details] [diff] [review]
Part3. Do not modify provided extradata array

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

::: dom/media/fmp4/ffmpeg/FFmpegDataDecoder.cpp
@@ +95,5 @@
>    mCodecContext->thread_safe_callbacks = false;
>  
>    if (mExtraData) {
>      mCodecContext->extradata_size = mExtraData->Length();
> +    // FFmepg may use SIMD instructions to access the data which reads the

nit: `mepg'

Comment 5

[Jean-Yves Avenard [:jya]]

Comment on attachment 8588920 [details] [diff] [review]
Part1. Create MediaRawData object type

can't work on opt build

Comment 6

[Jean-Yves Avenard [:jya]]

Attachment: Part1. Create MediaRawData object type

Remove MOZ_ASSERTs, add documentation.

Comment 7

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

I think you should have a gtest that exercises MediaRawData, in particular, the cloning and expanding of the buffer.

Comment 8

[Jean-Yves Avenard [:jya]]

Can do in a separate patch...
However, this is a version I had been using to test the behaviour:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2651571c14b3

Every creation, copy is surrounded by asserts that verify the size, the capacity and the content by running memcmp on all copied content.


here is a try with the last version (without the debugging):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b3b6faddfed4

Comment 9

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

Comment on attachment 8589938 [details] [diff] [review]
Part1. Create MediaRawData object type

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

::: dom/media/MediaData.cpp
@@ +113,3 @@
>  {
>    NS_ASSERTION(mDuration >= 0, "Frame must have non-negative duration.");
> +  mTimecode = aTimecode;

Can you initialize this in the initialization list?

@@ +127,4 @@
>  {
>    NS_ASSERTION(mDuration >= 0, "Frame must have non-negative duration.");
> +  mKeyframe = aKeyframe;
> +  mTimecode = aTimecode;

Can you initialize these in the initialization list?

@@ +535,5 @@
> +  }
> +  if (!mBuffer->SetCapacity(aSize + RAW_DATA_ALIGNMENT * 2)) {
> +    return false;
> +  }
> +  // Find alignment address.

You'll do an unnecessary copy here. The SetCapacity() call will copy any existing data in the existing buffer into the new buffer, and then you copy it again to ensure it's aligned. Maybe you should instead allocate the new buffer in a temporary, and copy the new data over into it, then set mBuffer to the temporary.

@@ +600,5 @@
> +  if (!mTarget->EnsureCapacity(aSize)) {
> +    return false;
> +  }
> +  mData = mBuffer->Elements() + mTarget->mPadding;
> +  return true;

Everytime you call EnsureSize(aSize), you then call `mSize = mTarget->mSize = aSize;`. Why don't you call `mSize = mTarget->mSize = aSize;` here inside EnsureSize() instead?

::: dom/media/MediaData.h
@@ +75,5 @@
>    int64_t GetEndTime() const { return mTime + mDuration; }
>  
>  protected:
> +  explicit MediaData(Type aType)
> +  : mType(aType)

Initializer list should be indented one more tab.

@@ +382,5 @@
> +  CryptoSample mCrypto;
> +  nsRefPtr<DataBuffer> mExtraData;
> +
> +  // Return a deep copy or nullptr if out of memory.
> +  virtual already_AddRefed<MediaRawData> Clone() const;

Comment here saying that the caller *must* delete the writer. Can you call this "CreateWriter()" instead, so it's clear that it allocates a new object?

Alternatively, can this return an nsAutoPtr<MediaRawDataWriter> instead, to force the caller to ensure it gets deleted? Does that work?

Comment 10

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

Comment on attachment 8589938 [details] [diff] [review]
Part1. Create MediaRawData object type

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

::: dom/media/MediaData.h
@@ +312,5 @@
> +
> +class CryptoSample : public CryptoTrack
> +{
> +public:
> +  nsTArray<uint16_t> plain_sizes;

It would be good to fix these members to use mStandardMozillaStyle instead of non_standard_underscored_style. In another patch maybe.

Comment 11

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

Comment on attachment 8589938 [details] [diff] [review]
Part1. Create MediaRawData object type

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

::: dom/media/MediaData.h
@@ +344,5 @@
> +class MediaRawDataWriter
> +{
> +public:
> +  // Pointer to data or null if not-yet allocated
> +  uint8_t* mData;

Since you're only supposed to write the data via the writer, why not only expose the raw data via a const accessor, i.e.:

const uint8_t* Data() const;

Ditto for size.

Comment 12

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

Comment on attachment 8588921 [details] [diff] [review]
Part2. Use new MediaRawObject across the board

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

Looks like you missed GMPEncryptedBufferDataImpl.h/cpp.

::: dom/media/fmp4/apple/AppleVDADecoder.h
@@ +34,5 @@
>      int64_t byte_offset;
>      bool is_sync_point;
>  
> +    explicit AppleFrameRef(const MediaRawData& aSample)
> +    : decode_timestamp(aSample.mTimecode)

Initializer list should be indented 1 level. Yeah, yeah, the code was like that when you got here...

::: dom/media/fmp4/apple/AppleVTDecoder.cpp
@@ +218,5 @@
>    // a custom block source which reuses the aSample buffer.
>    // But note that there may be a problem keeping the samples
>    // alive over multiple frames.
> +  rv = CMBlockBufferCreateWithMemoryBlock(kCFAllocatorDefault, // Struct allocator.
> +                                          const_cast<uint8_t*>(aSample->mData),

Casting away const-ness... eww...

::: dom/media/fmp4/eme/EMEDecoderModule.cpp
@@ +56,1 @@
>                                                         &EMEDecryptor::Input,

Hanging args no longer line up.

@@ +67,1 @@
>                                                         &EMEDecryptor::Decrypted,

Hanging args no longer line up.

::: dom/media/fmp4/eme/SamplesWaitingForKey.cpp
@@ +56,1 @@
>                                                       &MediaDataDecoder::Input,

Hanging args no longer line up.

::: dom/media/fmp4/wmf/WMFAudioMFTManager.cpp
@@ +181,2 @@
>  {
> +  const uint8_t* data = reinterpret_cast<const uint8_t*>(aSample->mData);

Isn't MediaRawData::mData is already a uint8_t*, so you shouldn't need to cast it to const to use it?

::: dom/media/fmp4/wmf/WMFVideoMFTManager.cpp
@@ +241,5 @@
>      // This can happen during shutdown.
>      return E_FAIL;
>    }
>    // Forward sample data to the decoder.
> +  const uint8_t* data = reinterpret_cast<const uint8_t*>(aSample->mData);

Isn't MediaRawData::mData is already a uint8_t*, so you shouldn't need to cast it to const to use it?

::: dom/media/gmp/GMPAudioHost.cpp
@@ -41,5 @@
>   , mRate(aRate)
>  {
> -  mBuffer.AppendElements(aSample->data, aSample->size);
> -  if (aSample->crypto.valid) {
> -    mCrypto = new GMPEncryptedBufferDataImpl(aSample->crypto);

Don't you need to also change GMPEncryptedBufferDataImpl::GMPEncryptedBufferDataImpl(const mp4_demuxer::CryptoSample& aCrypto) too? You're using that here.

::: dom/media/webm/IntelWebMVideoDecoder.cpp
@@ +43,5 @@
>              int64_t aByteOffset,
>              uint8_t* aData,
>              size_t aSize,
>              bool aSyncPoint)
> +  : MediaRawData(aData, aSize)

Initializer list should be indented.

@@ +364,4 @@
>    if (mQueuedVideoSample) {
>      return mQueuedVideoSample.forget();
>    }
> +  nsRefPtr<VP8Sample> sample = nullptr;

you shouldn't need the "= nulptr" here.

::: media/libstagefright/binding/H264.cpp
@@ +18,5 @@
>  class BitReader
>  {
>  public:
> +  explicit BitReader(const mozilla::DataBuffer* aBuffer)
> +  : mBitReader(aBuffer->Elements(), aBuffer->Length())

Initializer list indented 1 level.

Comment 13

[Jean-Yves Avenard [:jya]]

(In reply to Chris Pearce (:cpearce) from comment #9)
> Comment on attachment 8589938 [details] [diff] [review]
> Part1. Create MediaRawData object type
> 
> Review of attachment 8589938 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/MediaData.cpp
> @@ +113,3 @@
> >  {
> >    NS_ASSERTION(mDuration >= 0, "Frame must have non-negative duration.");
> > +  mTimecode = aTimecode;
> 
> Can you initialize this in the initialization list?

those are member of the base class, they can't be initialized that way. Or do you mean having a new base constructor that set the time ? 

> 
> @@ +127,4 @@
> >  {
> >    NS_ASSERTION(mDuration >= 0, "Frame must have non-negative duration.");
> > +  mKeyframe = aKeyframe;
> > +  mTimecode = aTimecode;
> 
> Can you initialize these in the initialization list?

same ...

> You'll do an unnecessary copy here. The SetCapacity() call will copy any
> existing data in the existing buffer into the new buffer, and then you copy
> it again to ensure it's aligned. Maybe you should instead allocate the new
> buffer in a temporary, and copy the new data over into it, then set mBuffer
> to the temporary.

Indeed, but I can't see a way around it if we are to use a nsTFallibleArray as backend.
The only way to avoid the unecessary copy then shift would be to manually manage the buffer. I have no problem doing that.

Having said that, in practice, you almost always get the same buffer. The only time we create a new copy is for Clone() or Prepend(). the way the memory allocation works in the nsTArray is by allocating a multiple of 2 greater than the required size. As we currently only use Prepend to add about 40 bytes, it means that the original capacity is actually enough and we end up only shifting once.

Now, nsTArray isn't the most efficient container, as it's designed to work on any size and as such, copies always occur byte by byte. A memmove/memcpy would be much better.

Luckily, the class is abstracted enough that we can change the backend very easily.


> 
> @@ +600,5 @@
> > +  if (!mTarget->EnsureCapacity(aSize)) {
> > +    return false;
> > +  }
> > +  mData = mBuffer->Elements() + mTarget->mPadding;
> > +  return true;
> 
> Everytime you call EnsureSize(aSize), you then call `mSize = mTarget->mSize
> = aSize;`. Why don't you call `mSize = mTarget->mSize = aSize;` here inside
> EnsureSize() instead?
> 

mSize was a later addition. will do.

Comment 14

[Jean-Yves Avenard [:jya]]

(In reply to Chris Pearce (:cpearce) from comment #11)
> Comment on attachment 8589938 [details] [diff] [review]
> Part1. Create MediaRawData object type
> 
> Review of attachment 8589938 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/MediaData.h
> @@ +344,5 @@
> > +class MediaRawDataWriter
> > +{
> > +public:
> > +  // Pointer to data or null if not-yet allocated
> > +  uint8_t* mData;
> 
> Since you're only supposed to write the data via the writer, why not only
> expose the raw data via a const accessor, i.e.:
> 
> const uint8_t* Data() const;
> 
> Ditto for size.

MediaRawData mData is a const uint8_t*
MediaRawDaraWriter gives you access in writing to the raw data.

I kept the use of mData and mSize so we could easily replaced MP4Sample. Having accessor rather than using member variables would be better across the board yes.
It would simplify the MediaRawData code too.
Would you prefer we move to accessor then?

What about the Crypto container classes. Should we use accessors too?

Comment 15

[Jean-Yves Avenard [:jya]]

(In reply to Chris Pearce (:cpearce) from comment #12)

> > +    explicit AppleFrameRef(const MediaRawData& aSample)
> > +    : decode_timestamp(aSample.mTimecode)
> 
> Initializer list should be indented 1 level. Yeah, yeah, the code was like
> that when you got here...

the code was like that when I got there !

> 
> ::: dom/media/fmp4/apple/AppleVTDecoder.cpp
> @@ +218,5 @@
> >    // a custom block source which reuses the aSample buffer.
> >    // But note that there may be a problem keeping the samples
> >    // alive over multiple frames.
> > +  rv = CMBlockBufferCreateWithMemoryBlock(kCFAllocatorDefault, // Struct allocator.
> > +                                          const_cast<uint8_t*>(aSample->mData),
> 
> Casting away const-ness... eww...
> 


Tell that to Apple who never appears to have any of its API use const.

> ::: dom/media/fmp4/wmf/WMFAudioMFTManager.cpp
> @@ +181,2 @@
> >  {
> > +  const uint8_t* data = reinterpret_cast<const uint8_t*>(aSample->mData);
> 
> Isn't MediaRawData::mData is already a uint8_t*, so you shouldn't need to
> cast it to const to use it?

it is a const uint8_t* yes... so that code is now unecessary. Looks like someone didn't know about const_cast<> :)
> 


> Don't you need to also change
> GMPEncryptedBufferDataImpl::GMPEncryptedBufferDataImpl(const
> mp4_demuxer::CryptoSample& aCrypto) too? You're using that here.

hmm... wonder how that got through all the try test ... This isn't compiled by default ???

Comment 16

[Jean-Yves Avenard [:jya]]

Attachment: Part1. Create MediaRawData object type

Carrying r+. Allocate the buffer to be a default 4kB. Any further allocation will be made big enough by the nsTArray that it will prevent further allocations unecessary under all current use.

Comment 17

[Jean-Yves Avenard [:jya]]

Attachment: Part2. Use new MediaRawObject across the board

Carrying r+. Looks like I had uploaded an obsolete patch. Most comments had already been addressed.

Comment 18

[Jean-Yves Avenard [:jya]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7f283bb1827c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5b615a235096
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c2b367ccb601

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7f283bb1827c
https://hg.mozilla.org/mozilla-central/rev/5b615a235096
https://hg.mozilla.org/mozilla-central/rev/c2b367ccb601

Comment 20

[Martin Thomson [:mt:]]

The Part 2 patch here causes a permanent crash on some systems.  See bug 1153227.

Comment 21

[Jean-Yves Avenard [:jya]]

WebRTC uses MP4???

Comment 22

[Martin Thomson [:mt:]]

OK, we worked it out: the new DataBuffer class shadows the existing DataBuffer used in WebRTC.

Why this didn't cause a compilation error is simply another reason to despise C++.

Comment 23

[Jean-Yves Avenard [:jya]]

(In reply to Martin Thomson [:mt] from comment #22)
> OK, we worked it out: the new DataBuffer class shadows the existing
> DataBuffer used in WebRTC.
> 
> Why this didn't cause a compilation error is simply another reason to
> despise C++.

This shouldn't have compiled. DataBuffer is held into a nsAutoPtr which would call the DataBuffer destructor which is private.
I don't think this is a C++ problem per-say... but that we have C and C++ headers mixed together.