Closed Bug 1248861 Opened 10 years ago Closed 10 years ago

All audio MediaDataDecoder should use the same channel ordering

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla48
Tracking Flags:
Tracking Status
firefox47 --- affected
firefox48 --- fixed

People

(Reporter: jya, Assigned: jya)

References

Details

Attachments

(12 files, 1 obsolete file)

MozReview Request: Bug 1248861: P1. Add AlignedBuffer template class. r=cpearce
58 bytes, text/x-review-board-request
cpearce
: review+
Details
MozReview Request: Bug 1248861: P2. Use AlignedByteBuffer with MediaRawData. r=cpearce
58 bytes, text/x-review-board-request
cpearce
: review+
Details
MozReview Request: Bug 1248861: P3. Use AlignedAudioBuffer object with AudioData. r=cpearce
58 bytes, text/x-review-board-request
cpearce
: review+
Details
MozReview Request: Bug 1248861: P4. Add AudioConfig and introduce channel layout definition. r=cpearce
58 bytes, text/x-review-board-request
cpearce
: review+
Details
MozReview Request: Bug 1248861: P5. Add AudioConverter class and relatives. r=cpearce
58 bytes, text/x-review-board-request
cpearce
: review+
Details
MozReview Request: Bug 1248861: P6. Ensure AT decoder channel layout is always in SMPTE order. r=rillian
58 bytes, text/x-review-board-request
rillian
: review+
Details
MozReview Request: Bug 1248861: P7. Ensure vorbis decoder channel layout is always in SMPTE order. r=rillian
58 bytes, text/x-review-board-request
rillian
: review+
Details
MozReview Request: Bug 1248861: P10. Ensure opus decoder channel layout is always in SMPTE order. r=rillian
58 bytes, text/x-review-board-request
rillian
: review+
Details
MozReview Request: Bug 1248861: P11. Fix current downmixer. r=rillian
58 bytes, text/x-review-board-request
rillian
: review+
Details
MozReview Request: Bug 1248861: P8. Added ChannelLayout::MappingTable method. r=gerald
58 bytes, text/x-review-board-request
mozbugz
: review+
Details
MozReview Request: Bug 1248861: P9. Use ChannelLayout::MappingTable. r=gerald
58 bytes, text/x-review-board-request
mozbugz
: review+
Details
MozReview Request: Bug 1248861: P12. Update AudioCompactor to account for alignment padding. r?rillian
58 bytes, text/x-review-board-request
rillian
: review+
Details
Currently, each MediaDataDecoder returns channels in the order typical for the platforms. e.g. Apple and Microsoft use their own, FFmpeg uses SMPTE. We should ensure they all use the same channel ordering. I vote for SMPTE order. /* SMPTE channel layout DUAL-MONO L R DUAL-MONO-LFE L R LFE MONO M MONO-LFE M LFE STEREO L R STEREO-LFE L R LFE 3F L R C 3F-LFE L R C LFE 2F1 L R S 2F1-LFE L R LFE S 3F1 L R C S 3F1-LFE L R C LFE S 2F2 L R LS RS 2F2-LFE L R LFE LS RS 3F2 L R C LS RS 3F2-LFE L R C LFE LS RS 3F3R-LFE L R C LFE BC LS RS 3F4-LFE L R C LFE Rls Rrs LS RS */
Priority: -- → P2
Along with AlignedByteBuffer and AlignedFloatBuffer Review commit: https://reviewboard.mozilla.org/r/44259/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/44259/
Attachment #8738029 - Flags: review?(cpearce)
Attachment #8738030 - Flags: review?(cpearce)
Attachment #8738031 - Flags: review?(cpearce)
Attachment #8738029 - Flags: review?(cpearce) → review+
Comment on attachment 8738029 [details] MozReview Request: Bug 1248861: P1. Add AlignedBuffer template class. r=cpearce https://reviewboard.mozilla.org/r/44259/#review41191 ::: dom/media/MediaData.h:47 (Diff revision 1) > +// zeroed on creation, elements are not individually constructed. > +// An Alignment value of 0 means that the data isn't aligned. > +// > +// Type must be trivially copyable. > +// > +// AlignedBuffer can typically be use in place of UniquePtr<Type[]> however Typo: s/be use/be used/
https://reviewboard.mozilla.org/r/44259/#review41193 ::: dom/media/MediaData.h:118 (Diff revision 1) > + return *this; > + } > + > + Type* Data() const { return mData; } > + size_t Length() const { return mLength; } > + size_t Size() const { return mLength * sizeof(Type); } Also, consider renaming Size() to SizeInBytes() to help make it clear how Size() is different to Length(). We're not consistent about using size for a count of the number of bytes in our code, for example in MediaRawData.
Comment on attachment 8738030 [details] MozReview Request: Bug 1248861: P2. Use AlignedByteBuffer with MediaRawData. r=cpearce https://reviewboard.mozilla.org/r/44261/#review41195
Attachment #8738030 - Flags: review?(cpearce) → review+
Attachment #8738031 - Flags: review?(cpearce) → review+
Comment on attachment 8738031 [details] MozReview Request: Bug 1248861: P3. Use AlignedAudioBuffer object with AudioData. r=cpearce https://reviewboard.mozilla.org/r/44263/#review41197 ::: dom/media/platforms/ffmpeg/FFmpegAudioDecoder.cpp:139 (Diff revision 1) > CopyAndPackAudio(mFrame, numChannels, mFrame->nb_samples); > > media::TimeUnit duration = > FramesToTimeUnit(mFrame->nb_samples, samplingRate); > - if (!duration.IsValid()) { > + if (!audio || !duration.IsValid()) { > NS_WARNING("Invalid count of accumulated audio samples"); May as well make the warning mention "OOM or invalid count..."
Long term goal would be to merge AudioConfig with the existing AudioInfo class which doesn't provide sufficient data to properly determine how to play multichannel audio. Review commit: https://reviewboard.mozilla.org/r/44463/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/44463/
Attachment #8738029 - Attachment description: MozReview Request: Bug 1248861: P1. Add AlignedBuffer template class. r?cpearce → MozReview Request: Bug 1248861: P1. Add AlignedBuffer template class. r=cpearce
Attachment #8738030 - Attachment description: MozReview Request: Bug 1248861: P2. Use AlignedByteBuffer with MediaRawData. r?cpearce → MozReview Request: Bug 1248861: P2. Use AlignedByteBuffer with MediaRawData. r=cpearce
Attachment #8738031 - Attachment description: MozReview Request: Bug 1248861: P3. Use AlignedAudioBuffer object with AudioData. r?cpearce → MozReview Request: Bug 1248861: P3. Use AlignedAudioBuffer object with AudioData. r=cpearce
Attachment #8738340 - Flags: review?(cpearce)
Attachment #8738341 - Flags: review?(cpearce)
Attachment #8738342 - Flags: review?(giles)
Attachment #8738343 - Flags: review?(giles)
Attachment #8738344 - Flags: review?(giles)
To be used in combination with AudioDataBuffer class that will be able to perform format conversion. Can currently only perform channel re-ordering. Future use will add downmixing, upmixing and resampling capabilities. Review commit: https://reviewboard.mozilla.org/r/44465/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/44465/
Comment on attachment 8738029 [details] MozReview Request: Bug 1248861: P1. Add AlignedBuffer template class. r=cpearce Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44259/diff/1-2/
Comment on attachment 8738030 [details] MozReview Request: Bug 1248861: P2. Use AlignedByteBuffer with MediaRawData. r=cpearce Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44261/diff/1-2/
Comment on attachment 8738031 [details] MozReview Request: Bug 1248861: P3. Use AlignedAudioBuffer object with AudioData. r=cpearce Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44263/diff/1-2/
https://reviewboard.mozilla.org/r/44259/#review41207 Some thoughts: ::: dom/media/MediaData.h:32 (Diff revision 2) > > class MediaByteBuffer; > class SharedTrackInfo; > > +// AlignedBuffer: > +// Memory allocations are fallibles. Methods return a boolean indicating if 'fallibles' -> 'fallible' ::: dom/media/MediaData.h:122 (Diff revision 2) > + size_t Length() const { return mLength; } > + size_t Size() const { return mLength * sizeof(Type); } > + Type& operator[](size_t aIndex) const > + { > + MOZ_ASSERT(aIndex < mLength); > + return *(mData + aIndex); Even though strictly equivalent, I think 'return mData[aIndex];' would be more in the spirit of this method. And it would just work if mData was ever changed to be a container type instead of a pointer. ::: dom/media/MediaData.h:165 (Diff revision 2) > + // Replace current content with aData. > + bool Replace(const Type* aData, size_t aLength) > + { > + // If aLength is smaller than our current length, we leave the buffer as is, > + // only adjusting the reported length. > + if (!EnsureCapacity(aLength)) { EnsureCapacity may do an unneeded copy to preserve the old data that we're about to replace. Maybe you could give it a hint (e.g. bool param) to skip that copy? ::: dom/media/MediaData.h:183 (Diff revision 2) > + size_t size = aMallocSizeOf(this); > + size += aMallocSizeOf(mBuffer.get()); > + return size; Wouldn't the simpler 'return aMallocSizeOf(this) + aMallocSizeOf(mBuffer.get());' work too? ::: dom/media/MediaData.h:240 (Diff revision 2) > + Type* mData; > + size_t mLength; > + UniquePtr<Type[]> mBuffer; > + size_t mCapacity; I think 'mBuffer' shouldn't refer to 'Type': You cannot assume that it points exactly at a 'Type' object, so giving it this type would not prevent unsafe dereferencing. Instead, mBuffer should just be a memory block (e.g. UniquePtr<uint8_t[]>; or even a vector/nsTArray<uint8_t> so you don't need the separate 'mCapacity', another safety win). Also, because uint8_t is finer-grained than most Type's you'll use, you should be able to save a bit of memory in EnsureCapacity().
Does DownmixAudioToStereo() also need to take channel ordering into account?
(In reply to JW Wang [:jwwang] from comment #17) > Does DownmixAudioToStereo() also need to take channel ordering into account? I haven't looked into this downmixer in details; but hearing on how it sound, it's pretty bad: Left is left. Center is right Right, left surround + right surround is left+right I intend to replace it with the AudioConverter one which does properly handle channels the downmixer is compatible with DTS-ES (DTS equivalent of Dolby ProLogic). I'll upload soon in another bug.
(In reply to Gerald Squelart [:gerald] from comment #16) > I think 'mBuffer' shouldn't refer to 'Type': You cannot assume that it > points exactly at a 'Type' object, so giving it this type would not prevent > unsafe dereferencing. > Instead, mBuffer should just be a memory block (e.g. UniquePtr<uint8_t[]>; > or even a vector/nsTArray<uint8_t> so you don't need the separate > 'mCapacity', another safety win). > Also, because uint8_t is finer-grained than most Type's you'll use, you > should be able to save a bit of memory in EnsureCapacity(). Or alternatively, I update the doc that Type must only ever be made of POD objects or derivative :)
(In reply to JW Wang [:jwwang] from comment #17) > Does DownmixAudioToStereo() also need to take channel ordering into account? looking at the history of DownmixAudioToStereo, the code comes from the ogg reader which is either opus or vorbis. And vorbis has its own channel layout, which is different to everything else. I'll update the downmix matrix accordingly for until I have the AudioConverter one ready
Current downmixer was using vorbis channel order (which isn't surprising as it was extracted from the Ogg reader). Make it use SMPTE order as that's now what all MediaDataDecoder output. Review commit: https://reviewboard.mozilla.org/r/44485/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/44485/
Attachment #8738386 - Flags: review?(giles)
Comment on attachment 8738029 [details] MozReview Request: Bug 1248861: P1. Add AlignedBuffer template class. r=cpearce Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44259/diff/2-3/
Comment on attachment 8738030 [details] MozReview Request: Bug 1248861: P2. Use AlignedByteBuffer with MediaRawData. r=cpearce Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44261/diff/2-3/
Comment on attachment 8738031 [details] MozReview Request: Bug 1248861: P3. Use AlignedAudioBuffer object with AudioData. r=cpearce Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44263/diff/2-3/
Comment on attachment 8738340 [details] MozReview Request: Bug 1248861: P4. Add AudioConfig and introduce channel layout definition. r=cpearce Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44463/diff/1-2/
Comment on attachment 8738341 [details] MozReview Request: Bug 1248861: P5. Add AudioConverter class and relatives. r=cpearce Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44465/diff/1-2/
Comment on attachment 8738342 [details] MozReview Request: Bug 1248861: P6. Ensure AT decoder channel layout is always in SMPTE order. r=rillian Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44467/diff/1-2/
Comment on attachment 8738343 [details] MozReview Request: Bug 1248861: P7. Ensure vorbis decoder channel layout is always in SMPTE order. r=rillian Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44469/diff/1-2/
Comment on attachment 8738344 [details] MozReview Request: Bug 1248861: P10. Ensure opus decoder channel layout is always in SMPTE order. r=rillian Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44471/diff/1-2/
Implemented Gerald's comment related to using a uint8_t[] as backend and added overflow safety check. I can't use a nsTArray or nsFallibleTArray as backend as it would otherwise re-introduce bug 1190258
https://reviewboard.mozilla.org/r/44259/#review41215 ::: dom/media/MediaData.h:56 (Diff revision 2) > +// becomes: AlignedFloatBuffer buffer(samples) > +// > +// auto buffer = MakeUnique<float[]>(samples) > +// becomes: > +// AlignedFloatBuffer buffer(samples); > +// if (!buffer) { return NS_ERROR_OUT_OF_MEMORY; } It is tricky to remember checking |buffer| after construction. I would prefer to disallow constructors that could fail. E.g. AlignedFloatBuffer buffer; if (!buffer.SetLength(samples)) { return NS_ERROR_OUT_OF_MEMORY; } ::: dom/media/MediaData.h:89 (Diff revision 2) > + return; > + } > + PodCopy(mData, aData, aLength); > + } > + > + AlignedBuffer(const AlignedBuffer<Type, Alignment>& aOther) Can we just say |const AlignedBuffer& aOther|? ::: dom/media/MediaData.h:104 (Diff revision 2) > + aOther.mData = nullptr; > + aOther.mLength = 0; > + aOther.mCapacity = 0; > + } > + > + AlignedBuffer& operator=(AlignedBuffer<Type, Alignment>&& aOther) https://dxr.mozilla.org/mozilla-central/source/mfbt/Variant.h#486 Not sure if this style is more preferred which seems more compact. ::: dom/media/MediaData.h:119 (Diff revision 2) > + } > + > + Type* Data() const { return mData; } > + size_t Length() const { return mLength; } > + size_t Size() const { return mLength * sizeof(Type); } > + Type& operator[](size_t aIndex) const Shouldn't it return |const Type&| for a const member function?
(In reply to JW Wang [:jwwang] from comment #31) > > +// if (!buffer) { return NS_ERROR_OUT_OF_MEMORY; } > > It is tricky to remember checking |buffer| after construction. I would > prefer to disallow constructors that could fail. no more than using fallible constructors, be it MakeUniqueFallible or nsFallibleTarray etc. > > E.g. > AlignedFloatBuffer buffer; > if (!buffer.SetLength(samples)) { > return NS_ERROR_OUT_OF_MEMORY; > } > > ::: dom/media/MediaData.h:89 > (Diff revision 2) > > + return; > > + } > > + PodCopy(mData, aData, aLength); > > + } > > + > > + AlignedBuffer(const AlignedBuffer<Type, Alignment>& aOther) > > Can we just say |const AlignedBuffer& aOther|? > > ::: dom/media/MediaData.h:104 > (Diff revision 2) > > + aOther.mData = nullptr; > > + aOther.mLength = 0; > > + aOther.mCapacity = 0; > > + } > > + > > + AlignedBuffer& operator=(AlignedBuffer<Type, Alignment>&& aOther) > > https://dxr.mozilla.org/mozilla-central/source/mfbt/Variant.h#486 > > Not sure if this style is more preferred which seems more compact. That is not equivalent, as we only want to allow for the exact same Type and Alignment. What you quoted would allow any. > > + Type& operator[](size_t aIndex) const > > Shouldn't it return |const Type&| for a const member function? That the function is const doesn't mean the return value should be. And for operator[] you certainly don't want to as it allows you to do thing like: buffer[index] = blah If you made the return value const, then you would lose code compatibility with working with either Type* or UniquePtr<Type[]> (https://dxr.mozilla.org/mozilla-central/source/mfbt/UniquePtr.h#459) Now I could make the function itself const and non-const, though I don't see much the purpose of it. Overall, consider AlignedBuffer<T> as replacement of MakeUniqueFallible<T[]>
Come to worse, could always do a similar mechanism to the new fallible nsTArray which can now take an optional fallible argument. And we would make it AlignedBuffer infallible instead
Upon further thought, and discussing with Gerald, I feel that memory allocation does deserve to be fallible for several reasons. 1- AlignedBuffer is likely only ever going to be used with external source and as such could have big requirements of which we have no control. Elegantly handling OOM rather than crashing is better. 2- We had to make MediaRawData fallible due to regular OOM crashes. MediaRawData are typically much smaller than decoded data. So it makes sense to make it fallible there too. If you're more comfortable doing AlignedBuffer blah; if (!blah.SetLength(value) { } you already can in its current form
https://reviewboard.mozilla.org/r/44259/#review41215 > https://dxr.mozilla.org/mozilla-central/source/mfbt/Variant.h#486 > > Not sure if this style is more preferred which seems more compact. I mean: AlignedBuffer& operator=(AlignedBuffer&& aOther) { this->~AlignedBuffer(); new (this) AlignedBuffer(Move(aOther)); return *this; } > Shouldn't it return |const Type&| for a const member function? It would be surprising that you are able to do: const AlignedBuffer buf(100); buf[0] = 0;
(In reply to JW Wang [:jwwang] from comment #35) > https://reviewboard.mozilla.org/r/44259/#review41215 > > > https://dxr.mozilla.org/mozilla-central/source/mfbt/Variant.h#486 > > > > Not sure if this style is more preferred which seems more compact. > > I mean: > AlignedBuffer& operator=(AlignedBuffer&& aOther) > { > this->~AlignedBuffer(); > new (this) AlignedBuffer(Move(aOther)); > return *this; > } Oh I misread, I thought you were arguing on the argument declaration. I guess you can't miss any copy that way... a tad weird at first glance though
Comment on attachment 8738029 [details] MozReview Request: Bug 1248861: P1. Add AlignedBuffer template class. r=cpearce Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44259/diff/3-4/
Comment on attachment 8738030 [details] MozReview Request: Bug 1248861: P2. Use AlignedByteBuffer with MediaRawData. r=cpearce Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44261/diff/3-4/
Comment on attachment 8738031 [details] MozReview Request: Bug 1248861: P3. Use AlignedAudioBuffer object with AudioData. r=cpearce Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44263/diff/3-4/
Comment on attachment 8738340 [details] MozReview Request: Bug 1248861: P4. Add AudioConfig and introduce channel layout definition. r=cpearce Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44463/diff/2-3/
Comment on attachment 8738341 [details] MozReview Request: Bug 1248861: P5. Add AudioConverter class and relatives. r=cpearce Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44465/diff/2-3/
Comment on attachment 8738342 [details] MozReview Request: Bug 1248861: P6. Ensure AT decoder channel layout is always in SMPTE order. r=rillian Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44467/diff/2-3/
Comment on attachment 8738343 [details] MozReview Request: Bug 1248861: P7. Ensure vorbis decoder channel layout is always in SMPTE order. r=rillian Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44469/diff/2-3/
Comment on attachment 8738344 [details] MozReview Request: Bug 1248861: P10. Ensure opus decoder channel layout is always in SMPTE order. r=rillian Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44471/diff/2-3/
Comment on attachment 8738386 [details] MozReview Request: Bug 1248861: P11. Fix current downmixer. r=rillian Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44485/diff/1-2/
integrated comments. thanks
https://reviewboard.mozilla.org/r/44463/#review41315 ::: dom/media/MediaInfo.cpp:126 (Diff revision 3) > +AudioConfig::SampleSize(AudioConfig::SampleFormat aFormat) > +{ > + switch (aFormat) { > + case FORMAT_U8: return 1; > + case FORMAT_S16: return 2; > + case FORMAT_S24: These should be marked `MOZ_FALLTHROUGH` to avoid warnings.
Comment on attachment 8738342 [details] MozReview Request: Bug 1248861: P6. Ensure AT decoder channel layout is always in SMPTE order. r=rillian https://reviewboard.mozilla.org/r/44467/#review41341 ::: dom/media/platforms/apple/AppleATDecoder.cpp:470 (Diff revision 3) > + &size, NULL); > + if (!status && size) { > + auto data = MakeUnique<uint8_t[]>(size); > + status = > + AudioConverterGetProperty(mConverter, kAudioConverterInputChannelLayout, > + &size, data.get()); Consider asserting the size set here matches the one returned earlier by `AudioConvertGetPropertyInfo()`. Or the length of data, but I'm not sure `ArrayLength` works on dynamically allocated arrays like this, so might have to use `std::array` or `vector`. ::: dom/media/platforms/apple/AppleATDecoder.cpp:474 (Diff revision 3) > + AudioConverterGetProperty(mConverter, kAudioConverterInputChannelLayout, > + &size, data.get()); > + if (status) { > + LOG("Couldn't get channel layout property (%s) assuming default channel layout", > + FourCC2Str(status)); > + } else { Looks like you can just `return NS_OK;` here and save an indent level. ::: dom/media/platforms/apple/AppleATDecoder.cpp:491 (Diff revision 3) > + sizeof(AudioChannelLayoutTag), &tag, &size); > + } > + if (!status && size) { > + data = MakeUnique<uint8_t[]>(size); > + layout = reinterpret_cast<AudioChannelLayout*>(data.get()); > + Isn't `layout` already in scope here? ::: dom/media/platforms/apple/AppleATDecoder.cpp:517 (Diff revision 3) > + MakeUnique<AudioConfig::ChannelLayout>(mOutputFormat.mChannelsPerFrame, > + channels); > + } > + } > + } > return NS_OK; This looks correct, but is really hard to follow with all the nesting and different values of status. I think it would be easier to read if you returned early everywhere. Seems like you just leave the mChannelLayout alone and return NS_OK if anything fails, so might as well do that directly. Duplicating the `if (!status && size)` conditionals would be less confusing than duplicating `if (tag == kAudioChannelLayoutTag_UseChannelBitmap)`, so invert those.
Attachment #8738342 - Flags: review?(giles) → review+
Comment on attachment 8738343 [details] MozReview Request: Bug 1248861: P7. Ensure vorbis decoder channel layout is always in SMPTE order. r=rillian https://reviewboard.mozilla.org/r/44469/#review41359 ::: dom/media/platforms/agnostic/VorbisDecoder.cpp:290 (Diff revision 3) > > +/* static */ const AudioConfig::Channel* > +VorbisDataDecoder::VorbisLayout(uint32_t aChannels) > +{ > + // From http://www.xiph.org/vorbis/doc/Vorbis_I_spec.html#x1-810004.3.9 > + typedef AudioConfig::Channel Channel; https urls, please. "Section 4.3.9" might be a more reliable reference than that dodgy-looking fragment identifier. :/
Attachment #8738343 - Flags: review?(giles) → review+
Attachment #8738344 - Flags: review?(giles)
Comment on attachment 8738344 [details] MozReview Request: Bug 1248861: P10. Ensure opus decoder channel layout is always in SMPTE order. r=rillian https://reviewboard.mozilla.org/r/44471/#review41379 ::: dom/media/platforms/agnostic/OpusDecoder.cpp:284 (Diff revision 3) > + mAudioConverter = MakeUnique<AudioConverter>(in, out); > + } > + MOZ_ASSERT(mAudioConverter->CanWorkInPlace()); > + AudioSampleBuffer data(Move(buffer)); > + mAudioConverter->Process(data); > + This does a double conversion. `opus_multstream_decode()` is already swizzling the decoded channels to map the internal layout in the compressed stream to vorbis order. It would be better to permute the `OpusParser::mMappingTable` values before passing them to `opus_multistream_decoder_create()` so it outputs SMPTE order directly.
Attachment #8738386 - Flags: review?(giles)
Comment on attachment 8738386 [details] MozReview Request: Bug 1248861: P11. Fix current downmixer. r=rillian https://reviewboard.mozilla.org/r/44485/#review41383 LGTM except for the 6.1 surround ordering. ::: dom/media/VideoUtils.cpp:155 (Diff revision 2) > static const float dmatrix[6][8][2]= { > - /*3*/{{0.5858f,0},{0.4142f,0.4142f},{0, 0.5858f}}, > - /*4*/{{0.4226f,0},{0, 0.4226f},{0.366f,0.2114f},{0.2114f,0.366f}}, > - /*5*/{{0.6510f,0},{0.4600f,0.4600f},{0, 0.6510f},{0.5636f,0.3254f},{0.3254f,0.5636f}}, > - /*6*/{{0.5290f,0},{0.3741f,0.3741f},{0, 0.5290f},{0.4582f,0.2645f},{0.2645f,0.4582f},{0.3741f,0.3741f}}, > - /*7*/{{0.4553f,0},{0.3220f,0.3220f},{0, 0.4553f},{0.3943f,0.2277f},{0.2277f,0.3943f},{0.2788f,0.2788f},{0.3220f,0.3220f}}, > + /*3*/{{0.5858f,0},{0,0.5858f},{0.4142f,0.4142f}}, > + /*4*/{{0.4226f,0},{0,0.4226f},{0.366f, 0.2114f},{0.2114f,0.366f}}, > + /*5*/{{0.6510f,0},{0,0.6510f},{0.4600f,0.4600f},{0.5636f,0.3254f},{0.3254f,0.5636f}}, > + /*6*/{{0.5290f,0},{0,0.5290f},{0.3741f,0.3741f},{0.3741f,0.3741f},{0.4582f,0.2645f},{0.2645f,0.4582f}}, > + /*7*/{{0.4553f,0},{0,0.4553f},{0.3220f,0.3220f},{0.3220f,0.3220f},{0.3943f,0.2277f},{0.2277f,0.3943f},{0.2788f,0.2788f}}, RCENTER comes before LS and RS in the SMPTE order. ::: dom/media/VideoUtils.cpp:177 (Diff revision 2) > static const int16_t dmatrix[6][8][2]= { > - /*3*/{{9598, 0},{6786,6786},{0, 9598}}, > + /*3*/{{9598, 0},{0, 9598},{6786,6786}}, > /*4*/{{6925, 0},{0, 6925},{5997,3462},{3462,5997}}, > - /*5*/{{10663,0},{7540,7540},{0, 10663},{9234,5331},{5331,9234}}, > - /*6*/{{8668, 0},{6129,6129},{0, 8668},{7507,4335},{4335,7507},{6129,6129}}, > - /*7*/{{7459, 0},{5275,5275},{0, 7459},{6460,3731},{3731,6460},{4568,4568},{5275,5275}}, > + /*5*/{{10663,0},{0, 10663},{7540,7540},{9234,5331},{5331,9234}}, > + /*6*/{{8668, 0},{0, 8668},{6129,6129},{6129,6129},{7507,4335},{4335,7507}}, > + /*7*/{{7459, 0},{0, 7459},{5275,5275},{5275,5275},{6460,3731},{3731,6460},{4568,4568}}, Same issue here.
https://reviewboard.mozilla.org/r/44467/#review41341 > Consider asserting the size set here matches the one returned earlier by `AudioConvertGetPropertyInfo()`. Or the length of data, but I'm not sure `ArrayLength` works on dynamically allocated arrays like this, so might have to use `std::array` or `vector`. ArrayLength wouldn't work, the struct is allocated on the stack and has a variable sized array there. We certainly don't check anywhere else in the code that AudioFormatGetProperty changed the value somehow. if what was actually written isn't what we asked it would be particularly screwed up > Looks like you can just `return NS_OK;` here and save an indent level. i don't understand what you mean here and where... > Isn't `layout` already in scope here? layout is just a raw pointer to the data returned. There are three ways the AT can return layout. it can be explicit, based on a tag, or based on the channel number default. I'll document the code a bit more and maybe split it in an external function to make it more easy to track the code path
Review commit: https://reviewboard.mozilla.org/r/44735/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/44735/
Attachment #8738344 - Attachment description: MozReview Request: Bug 1248861: P8. Ensure opus decoder channel layout is always in SMPTE order. r?rillian → MozReview Request: Bug 1248861: P10. Ensure opus decoder channel layout is always in SMPTE order. r?rillian
Attachment #8738386 - Attachment description: MozReview Request: Bug 1248861: P9. Fix current downmixer. r?rillian → MozReview Request: Bug 1248861: P11. Fix current downmixer. r?rillian
Attachment #8738888 - Flags: review?(gsquelart)
Attachment #8738889 - Flags: review?(gsquelart)
Attachment #8738344 - Flags: review?(giles)
Attachment #8738386 - Flags: review?(giles)
Comment on attachment 8738029 [details] MozReview Request: Bug 1248861: P1. Add AlignedBuffer template class. r=cpearce Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44259/diff/4-5/
Comment on attachment 8738030 [details] MozReview Request: Bug 1248861: P2. Use AlignedByteBuffer with MediaRawData. r=cpearce Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44261/diff/4-5/
Comment on attachment 8738031 [details] MozReview Request: Bug 1248861: P3. Use AlignedAudioBuffer object with AudioData. r=cpearce Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44263/diff/4-5/
Comment on attachment 8738340 [details] MozReview Request: Bug 1248861: P4. Add AudioConfig and introduce channel layout definition. r=cpearce Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44463/diff/3-4/
Comment on attachment 8738341 [details] MozReview Request: Bug 1248861: P5. Add AudioConverter class and relatives. r=cpearce Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44465/diff/3-4/
Comment on attachment 8738342 [details] MozReview Request: Bug 1248861: P6. Ensure AT decoder channel layout is always in SMPTE order. r=rillian Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44467/diff/3-4/
Comment on attachment 8738343 [details] MozReview Request: Bug 1248861: P7. Ensure vorbis decoder channel layout is always in SMPTE order. r=rillian Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44469/diff/3-4/
Comment on attachment 8738344 [details] MozReview Request: Bug 1248861: P10. Ensure opus decoder channel layout is always in SMPTE order. r=rillian Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44471/diff/3-4/
Comment on attachment 8738386 [details] MozReview Request: Bug 1248861: P11. Fix current downmixer. r=rillian Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44485/diff/2-3/
Comment on attachment 8738341 [details] MozReview Request: Bug 1248861: P5. Add AudioConverter class and relatives. r=cpearce Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44465/diff/4-5/
Comment on attachment 8738342 [details] MozReview Request: Bug 1248861: P6. Ensure AT decoder channel layout is always in SMPTE order. r=rillian Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44467/diff/4-5/
Comment on attachment 8738343 [details] MozReview Request: Bug 1248861: P7. Ensure vorbis decoder channel layout is always in SMPTE order. r=rillian Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44469/diff/4-5/
Comment on attachment 8738888 [details] MozReview Request: Bug 1248861: P8. Added ChannelLayout::MappingTable method. r=gerald Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44735/diff/1-2/
Comment on attachment 8738889 [details] MozReview Request: Bug 1248861: P9. Use ChannelLayout::MappingTable. r=gerald Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44737/diff/1-2/
Comment on attachment 8738344 [details] MozReview Request: Bug 1248861: P10. Ensure opus decoder channel layout is always in SMPTE order. r=rillian Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44471/diff/4-5/
Comment on attachment 8738386 [details] MozReview Request: Bug 1248861: P11. Fix current downmixer. r=rillian Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44485/diff/3-4/
Comment on attachment 8738888 [details] MozReview Request: Bug 1248861: P8. Added ChannelLayout::MappingTable method. r=gerald https://reviewboard.mozilla.org/r/44735/#review41489
Attachment #8738888 - Flags: review?(gsquelart) → review+
Blocks: 1262753
Comment on attachment 8738889 [details] MozReview Request: Bug 1248861: P9. Use ChannelLayout::MappingTable. r=gerald https://reviewboard.mozilla.org/r/44737/#review41493
Attachment #8738889 - Flags: review?(gsquelart) → review+
https://reviewboard.mozilla.org/r/44467/#review41551 Thanks. Just a couple of comment typos. ::: dom/media/platforms/apple/AppleATDecoder.cpp:422 (Diff revision 5) > + &propertySize, NULL); > + if (status || !propertySize) { > + LOG("Couldn't get channel layout property (%s)", FourCC2Str(status)); > + return NS_ERROR_FAILURE; > + } > + This is better, thanks. ::: dom/media/platforms/apple/AppleATDecoder.cpp:444 (Diff revision 5) > + // directly contains the the channel layout mapping. > + // If tag is kAudioChannelLayoutTag_UseChannelBitmap then the layout will > + // be defined via the bitmap and can be retrieved using > + // kAudioFormatProperty_ChannelLayoutForBitmap property. > + // Otherwise the tag itself describe the layout. > + if (tag != kAudioChannelLayoutTag_UseChannelDescriptions) { s/describe/describes/ in the last line of the comment. ::: dom/media/platforms/apple/AppleATDecoder.cpp:486 (Diff revision 5) > + FourCC2Str(property), FourCC2Str(status)); > + return NS_ERROR_FAILURE; > + } > + // We have no retrieved the channel layout from the tag or bitmap. > + // We can now directly use the channel descriptions. > + layout->mChannelLayoutTag = kAudioChannelLayoutTag_UseChannelDescriptions; // We have retrieved ... `s/ no //` in the first line of the comment.
Attachment #8738386 - Flags: review?(giles) → review+
Comment on attachment 8738386 [details] MozReview Request: Bug 1248861: P11. Fix current downmixer. r=rillian https://reviewboard.mozilla.org/r/44485/#review41553
Comment on attachment 8738344 [details] MozReview Request: Bug 1248861: P10. Ensure opus decoder channel layout is always in SMPTE order. r=rillian https://reviewboard.mozilla.org/r/44471/#review41557 ::: dom/media/platforms/agnostic/OpusDecoder.cpp:110 (Diff revision 5) > + int channels = mOpusParser->mChannels; > // No channel mapping for more than 8 channels. > - if (mOpusParser->mChannels > 8) { > + if (channels > 8) { > OPUS_DEBUG("No channel mapping for more than 8 channels. Source is %d channels", > mOpusParser->mChannels); > return NS_ERROR_FAILURE; Should probably use the local `channels` in the log message too. ::: dom/media/platforms/agnostic/OpusDecoder.cpp:117 (Diff revision 5) > > + AudioConfig::ChannelLayout vorbisLayout( > + channels, VorbisDataDecoder::VorbisLayout(channels)); > + AudioConfig::ChannelLayout smpteLayout(channels); > + uint8_t map[ArrayLength(mOpusParser->mMappingTable)]; > + if (vorbisLayout.MappingTable(smpteLayout, map)) { `OpusParser::mMappingTable` is 255 bytes, to match the maximum size allowed by the format (for discrete multichannel and future surround extensions) but we limit ourselves to 8 channels, so this allocation is 8x bigger than it needs to be. `ChannelLayout::MappingTable` expects an array of MAX_CHANNEL_COUNT, so I'd suggest ``` static_assert(MAX_CHANNEL_COUNT >= 8); uint8_t map[MAX_CHANNEL_COUNT]; ``` If you don't want to worry about it, at least assert that the passed-in array is at least MAX_CHANNEL_COUNT. You can export this as a const ChannelLayout::MAX_CHANNEL_COUNT if you're worried about the define polluting the namespace. ::: dom/media/platforms/agnostic/OpusDecoder.cpp:120 (Diff revision 5) > + AudioConfig::ChannelLayout smpteLayout(channels); > + uint8_t map[ArrayLength(mOpusParser->mMappingTable)]; > + if (vorbisLayout.MappingTable(smpteLayout, map)) { > + for (int32_t i = 0; i < channels; i++) { > + mMappingTable[i] = mOpusParser->mMappingTable[map[i]]; > + } `i` should be `int` or `auto` to match `channels`. ::: dom/media/platforms/agnostic/OpusDecoder.cpp:125 (Diff revision 5) > + } > + } else { > + // Should never get here as vorbis layout is always convertible to SMPTE > + // default layout. > + PodCopy(map, mOpusParser->mMappingTable, ArrayLength(mOpusParser->mMappingTable)); > + } You need to PodCopy into mChannelMap here instead of map, which is discarded.
Attachment #8738344 - Flags: review?(giles) → review+
Comment on attachment 8738342 [details] MozReview Request: Bug 1248861: P6. Ensure AT decoder channel layout is always in SMPTE order. r=rillian Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44467/diff/5-6/
Comment on attachment 8738343 [details] MozReview Request: Bug 1248861: P7. Ensure vorbis decoder channel layout is always in SMPTE order. r=rillian Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44469/diff/5-6/
Comment on attachment 8738888 [details] MozReview Request: Bug 1248861: P8. Added ChannelLayout::MappingTable method. r=gerald Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44735/diff/2-3/
Comment on attachment 8738889 [details] MozReview Request: Bug 1248861: P9. Use ChannelLayout::MappingTable. r=gerald Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44737/diff/2-3/
Comment on attachment 8738344 [details] MozReview Request: Bug 1248861: P10. Ensure opus decoder channel layout is always in SMPTE order. r=rillian Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44471/diff/5-6/
Comment on attachment 8738386 [details] MozReview Request: Bug 1248861: P11. Fix current downmixer. r=rillian Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44485/diff/4-5/
Comment on attachment 8738340 [details] MozReview Request: Bug 1248861: P4. Add AudioConfig and introduce channel layout definition. r=cpearce https://reviewboard.mozilla.org/r/44463/#review41649 ::: dom/media/MediaInfo.h:505 (Diff revision 4) > + , mValid(false) > + {} > + explicit ChannelLayout(uint32_t aChannels) > + : ChannelLayout(aChannels, SMPTEDefault(aChannels)) > + {} > + ChannelLayout(uint32_t aChannels, const Channel* config) aConfig ::: dom/media/MediaInfo.cpp:138 (Diff revision 4) > +} > + > +/* static */ uint32_t > +AudioConfig::FormatToBits(AudioConfig::SampleFormat aFormat) > +{ > + switch (aFormat) { This function has 4 space indent, but the other have 2 space indent.
Attachment #8738340 - Flags: review?(cpearce) → review+
Comment on attachment 8738341 [details] MozReview Request: Bug 1248861: P5. Add AudioConverter class and relatives. r=cpearce https://reviewboard.mozilla.org/r/44465/#review41651 Check the comment in AudioConverter::InitChannelMap() carefully please. ::: dom/media/AudioConverter.cpp:21 (Diff revision 5) > + */ > + > +namespace mozilla { > + > +/** > + * AudioConvert "AudioConvert" is not the class name. Seems comment is out of date and redundant. ::: dom/media/AudioConverter.cpp:33 (Diff revision 5) > + aIn.Rate() == aOut.Rate() && > + aIn.Format() == aOut.Format() && > + aIn.Interleaved() == aOut.Interleaved(), > + "Only channel reordering is supported at this stage"); > + MOZ_DIAGNOSTIC_ASSERT(aOut.Interleaved(), "planar audio format not supported"); > + InitChannelMap(); You're not checking the return value of InitChannelMap(), so why have a return value? ::: dom/media/AudioConverter.cpp:43 (Diff revision 5) > +{ > + if (!CanReorderAudio()) { > + return false; > + } > + for (uint32_t i = 0; i < mIn.Layout().Count(); i++) { > + for (uint32_t j = 0; j < mIn.Layout().Count(); j++) { Shouldn't that be: for (uint32_t j = 0; j < mOut.Layout().Count(); j++) { So that j is "for each mOut.Layout()"? Otherwise, wouldn't you potentially be iterating over invalid layouts for mOut if mIn.Layout().Count() != mOut.Layout().Count()? ::: dom/media/AudioConverter.cpp:56 (Diff revision 5) > +} > + > +bool > +AudioConverter::CanWorkInPlace() const > +{ > + return mIn.Channels() * mIn.Rate() * AudioConfig::SampleSize(mIn.Format()) <= This means you can only downmix right? ::: dom/media/AudioConverter.cpp:63 (Diff revision 5) > +} > + > +size_t > +AudioConverter::Process(void* aOut, const void* aIn, size_t aBytes) > +{ > + if (!CanWorkInPlace()) { You should fail here if either mIn or mOut have invalid layouts.
Attachment #8738341 - Flags: review?(cpearce) → review+
(In reply to Chris Pearce (:cpearce) from comment #84) > Comment on attachment 8738341 [details] > MozReview Request: Bug 1248861: P5. Add AudioConverter class and relatives. > r?cpearce > > https://reviewboard.mozilla.org/r/44465/#review41651 > > Check the comment in AudioConverter::InitChannelMap() carefully please. > > ::: dom/media/AudioConverter.cpp:21 > (Diff revision 5) > > + */ > > + > > +namespace mozilla { > > + > > +/** > > + * AudioConvert > > "AudioConvert" is not the class name. Seems comment is out of date and > redundant. > > ::: dom/media/AudioConverter.cpp:33 > (Diff revision 5) > > + aIn.Rate() == aOut.Rate() && > > + aIn.Format() == aOut.Format() && > > + aIn.Interleaved() == aOut.Interleaved(), > > + "Only channel reordering is supported at this stage"); > > + MOZ_DIAGNOSTIC_ASSERT(aOut.Interleaved(), "planar audio format not supported"); > > + InitChannelMap(); > > You're not checking the return value of InitChannelMap(), so why have a > return value? That code was moved in P8 and we do test it there. > > ::: dom/media/AudioConverter.cpp:43 > (Diff revision 5) > > +{ > > + if (!CanReorderAudio()) { > > + return false; > > + } > > + for (uint32_t i = 0; i < mIn.Layout().Count(); i++) { > > + for (uint32_t j = 0; j < mIn.Layout().Count(); j++) { > > Shouldn't that be: > > for (uint32_t j = 0; j < mOut.Layout().Count(); j++) { > > in.Count() == out.Count() as part of the assertion already > So that j is "for each mOut.Layout()"? Otherwise, wouldn't you potentially > be iterating over invalid layouts for mOut if mIn.Layout().Count() != > mOut.Layout().Count()? invalid layout are tested in CanReorderAudio(), in which case we leave the audio as-is and we will just play it wrong (or more accurately, play it like we've always played it up to now) > This means you can only downmix right? downmix, downsample or going from S32/Float/S24 to S16 so long as the final size is smaller than the original. > You should fail here if either mIn or mOut have invalid layouts. Done in CanReorderAudio()
(In reply to Chris Pearce (:cpearce) from comment #84) > > + if (!CanReorderAudio()) { > > + return false; > > + } > > + for (uint32_t i = 0; i < mIn.Layout().Count(); i++) { > > + for (uint32_t j = 0; j < mIn.Layout().Count(); j++) { > > Shouldn't that be: > > for (uint32_t j = 0; j < mOut.Layout().Count(); j++) { To be more accurate: CanReorderAudio() test that the channel count is the same indirectly. 1- layout can't be invalid (they will be invalid if they contains more than once a channel or if they have more than 8 channels) 2- they must have the same map. That means they have exactly the same channels (just in different order)
Comment on attachment 8738341 [details] MozReview Request: Bug 1248861: P5. Add AudioConverter class and relatives. r=cpearce Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44465/diff/5-6/
Comment on attachment 8738342 [details] MozReview Request: Bug 1248861: P6. Ensure AT decoder channel layout is always in SMPTE order. r=rillian Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44467/diff/6-7/
Comment on attachment 8738343 [details] MozReview Request: Bug 1248861: P7. Ensure vorbis decoder channel layout is always in SMPTE order. r=rillian Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44469/diff/6-7/
Comment on attachment 8738888 [details] MozReview Request: Bug 1248861: P8. Added ChannelLayout::MappingTable method. r=gerald Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44735/diff/3-4/
Comment on attachment 8738889 [details] MozReview Request: Bug 1248861: P9. Use ChannelLayout::MappingTable. r=gerald Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44737/diff/3-4/
Comment on attachment 8738344 [details] MozReview Request: Bug 1248861: P10. Ensure opus decoder channel layout is always in SMPTE order. r=rillian Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44471/diff/6-7/
Comment on attachment 8738386 [details] MozReview Request: Bug 1248861: P11. Fix current downmixer. r=rillian Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44485/diff/5-6/
Comment on attachment 8738340 [details] MozReview Request: Bug 1248861: P4. Add AudioConfig and introduce channel layout definition. r=cpearce Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44463/diff/4-5/
Comment on attachment 8738341 [details] MozReview Request: Bug 1248861: P5. Add AudioConverter class and relatives. r=cpearce Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44465/diff/6-7/
Comment on attachment 8738342 [details] MozReview Request: Bug 1248861: P6. Ensure AT decoder channel layout is always in SMPTE order. r=rillian Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44467/diff/7-8/
Comment on attachment 8738343 [details] MozReview Request: Bug 1248861: P7. Ensure vorbis decoder channel layout is always in SMPTE order. r=rillian Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44469/diff/7-8/
Comment on attachment 8738888 [details] MozReview Request: Bug 1248861: P8. Added ChannelLayout::MappingTable method. r=gerald Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44735/diff/4-5/
Comment on attachment 8738889 [details] MozReview Request: Bug 1248861: P9. Use ChannelLayout::MappingTable. r=gerald Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44737/diff/4-5/
Comment on attachment 8738344 [details] MozReview Request: Bug 1248861: P10. Ensure opus decoder channel layout is always in SMPTE order. r=rillian Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44471/diff/7-8/
Comment on attachment 8738386 [details] MozReview Request: Bug 1248861: P11. Fix current downmixer. r=rillian Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44485/diff/6-7/
Comment on attachment 8738341 [details] MozReview Request: Bug 1248861: P5. Add AudioConverter class and relatives. r=cpearce Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44465/diff/7-8/
Comment on attachment 8738342 [details] MozReview Request: Bug 1248861: P6. Ensure AT decoder channel layout is always in SMPTE order. r=rillian Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44467/diff/8-9/
Comment on attachment 8738343 [details] MozReview Request: Bug 1248861: P7. Ensure vorbis decoder channel layout is always in SMPTE order. r=rillian Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44469/diff/8-9/
Comment on attachment 8738888 [details] MozReview Request: Bug 1248861: P8. Added ChannelLayout::MappingTable method. r=gerald Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44735/diff/5-6/
Comment on attachment 8738889 [details] MozReview Request: Bug 1248861: P9. Use ChannelLayout::MappingTable. r=gerald Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44737/diff/5-6/
Comment on attachment 8738344 [details] MozReview Request: Bug 1248861: P10. Ensure opus decoder channel layout is always in SMPTE order. r=rillian Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44471/diff/8-9/
Comment on attachment 8738386 [details] MozReview Request: Bug 1248861: P11. Fix current downmixer. r=rillian Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44485/diff/7-8/
backedout for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=25400167&repo=mozilla-inbound
Flags: needinfo?(jyavenard)
issue is with the AudioCompactor which makes assumption that when creating an AudioData with X samples, the size allocated will always be X * sizeof(AudioDataValue). However, this assumption is no longer true as we have the extra alignement bytes. So when the AudioCompactor is assume it's about to allocate 4kB, it really allocated 8kB. I have disabled the alignement parameter for now, we don't need it for now as FFmpeg (which was the one really requiring it) allocates its own memory. I have incoming code that will make use of it and the AudioCompactor will have to be updated accordingly.
Flags: needinfo?(jyavenard)
backed out for failed asan tests like https://treeherder.mozilla.org/logviewer.html#?job_id=25415880&repo=mozilla-inbound
Flags: needinfo?(jyavenard)
None of the object leaking are related to AudioData! Sure it's a reproducible leak?
Flags: needinfo?(jyavenard)
Ok, was just a bad link to a failure not related to those changes. The Asan failure are those: SUMMARY: AddressSanitizer: heap-buffer-overflow /builds/slave/m-in-l64-asan-0000000000000000/build/src/media/ffvpx/libavcodec/bytestream.h:94 bytestream_get_be16 So ffmpeg while it doesn't need 32 bytes aligned data, does requires the 32 bytes padding at the end. Which makes sense as it uses SSE2 accelerated code that performs 32 bytes read at once.
MozReview-Commit-ID: 2e9oUthWW4C
Attachment #8739668 - Flags: review?(cpearce)
Review commit: https://reviewboard.mozilla.org/r/45341/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/45341/
Attachment #8738340 - Attachment description: MozReview Request: Bug 1248861: P4. Add AudioConfig and introduce channel layout definition. r?cpearce → MozReview Request: Bug 1248861: P4. Add AudioConfig and introduce channel layout definition. r=cpearce
Attachment #8738341 - Attachment description: MozReview Request: Bug 1248861: P5. Add AudioConverter class and relatives. r?cpearce → MozReview Request: Bug 1248861: P5. Add AudioConverter class and relatives. r=cpearce
Attachment #8738342 - Attachment description: MozReview Request: Bug 1248861: P6. Ensure AT decoder channel layout is always in SMPTE order. r?rillian → MozReview Request: Bug 1248861: P6. Ensure AT decoder channel layout is always in SMPTE order. r=rillian
Attachment #8738343 - Attachment description: MozReview Request: Bug 1248861: P7. Ensure vorbis decoder channel layout is always in SMPTE order. r?rillian → MozReview Request: Bug 1248861: P7. Ensure vorbis decoder channel layout is always in SMPTE order. r=rillian
Attachment #8738888 - Attachment description: MozReview Request: Bug 1248861: P8. Added ChannelLayout::MappingTable method. r?gerald → MozReview Request: Bug 1248861: P8. Added ChannelLayout::MappingTable method. r=gerald
Attachment #8738889 - Attachment description: MozReview Request: Bug 1248861: P9. Use ChannelLayout::MappingTable. r?gerald → MozReview Request: Bug 1248861: P9. Use ChannelLayout::MappingTable. r=gerald
Attachment #8738344 - Attachment description: MozReview Request: Bug 1248861: P10. Ensure opus decoder channel layout is always in SMPTE order. r?rillian → MozReview Request: Bug 1248861: P10. Ensure opus decoder channel layout is always in SMPTE order. r=rillian
Attachment #8738386 - Attachment description: MozReview Request: Bug 1248861: P11. Fix current downmixer. r?rillian → MozReview Request: Bug 1248861: P11. Fix current downmixer. r=rillian
Attachment #8739670 - Flags: review?(cpearce)
Comment on attachment 8738029 [details] MozReview Request: Bug 1248861: P1. Add AlignedBuffer template class. r=cpearce Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44259/diff/5-6/
Comment on attachment 8738030 [details] MozReview Request: Bug 1248861: P2. Use AlignedByteBuffer with MediaRawData. r=cpearce Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44261/diff/5-6/
Comment on attachment 8738031 [details] MozReview Request: Bug 1248861: P3. Use AlignedAudioBuffer object with AudioData. r=cpearce Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44263/diff/5-6/
Comment on attachment 8738340 [details] MozReview Request: Bug 1248861: P4. Add AudioConfig and introduce channel layout definition. r=cpearce Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44463/diff/5-6/
Comment on attachment 8738341 [details] MozReview Request: Bug 1248861: P5. Add AudioConverter class and relatives. r=cpearce Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44465/diff/8-9/
Comment on attachment 8738342 [details] MozReview Request: Bug 1248861: P6. Ensure AT decoder channel layout is always in SMPTE order. r=rillian Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44467/diff/9-10/
Comment on attachment 8738343 [details] MozReview Request: Bug 1248861: P7. Ensure vorbis decoder channel layout is always in SMPTE order. r=rillian Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44469/diff/9-10/
Comment on attachment 8738888 [details] MozReview Request: Bug 1248861: P8. Added ChannelLayout::MappingTable method. r=gerald Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44735/diff/6-7/
Comment on attachment 8738889 [details] MozReview Request: Bug 1248861: P9. Use ChannelLayout::MappingTable. r=gerald Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44737/diff/6-7/
Comment on attachment 8738344 [details] MozReview Request: Bug 1248861: P10. Ensure opus decoder channel layout is always in SMPTE order. r=rillian Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44471/diff/9-10/
Comment on attachment 8738386 [details] MozReview Request: Bug 1248861: P11. Fix current downmixer. r=rillian Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44485/diff/8-9/
Attachment #8739668 - Attachment is obsolete: true
Attachment #8739668 - Flags: review?(cpearce)
Comment on attachment 8738029 [details] MozReview Request: Bug 1248861: P1. Add AlignedBuffer template class. r=cpearce Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44259/diff/6-7/
Attachment #8739670 - Attachment description: MozReview Request: Bug 1248861: P12. Update AudioCompactor to account for alignment padding. r?cpearce → MozReview Request: Bug 1248861: P12. Update AudioCompactor to account for alignment padding. r?rillian
Attachment #8739670 - Flags: review?(cpearce) → review?(giles)
Comment on attachment 8738030 [details] MozReview Request: Bug 1248861: P2. Use AlignedByteBuffer with MediaRawData. r=cpearce Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44261/diff/6-7/
Comment on attachment 8738031 [details] MozReview Request: Bug 1248861: P3. Use AlignedAudioBuffer object with AudioData. r=cpearce Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44263/diff/6-7/
Comment on attachment 8738340 [details] MozReview Request: Bug 1248861: P4. Add AudioConfig and introduce channel layout definition. r=cpearce Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44463/diff/6-7/
Comment on attachment 8738341 [details] MozReview Request: Bug 1248861: P5. Add AudioConverter class and relatives. r=cpearce Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44465/diff/9-10/
Comment on attachment 8738342 [details] MozReview Request: Bug 1248861: P6. Ensure AT decoder channel layout is always in SMPTE order. r=rillian Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44467/diff/10-11/
Comment on attachment 8738343 [details] MozReview Request: Bug 1248861: P7. Ensure vorbis decoder channel layout is always in SMPTE order. r=rillian Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44469/diff/10-11/
Comment on attachment 8738888 [details] MozReview Request: Bug 1248861: P8. Added ChannelLayout::MappingTable method. r=gerald Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44735/diff/7-8/
Comment on attachment 8738889 [details] MozReview Request: Bug 1248861: P9. Use ChannelLayout::MappingTable. r=gerald Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44737/diff/7-8/
Comment on attachment 8738344 [details] MozReview Request: Bug 1248861: P10. Ensure opus decoder channel layout is always in SMPTE order. r=rillian Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44471/diff/10-11/
Comment on attachment 8738386 [details] MozReview Request: Bug 1248861: P11. Fix current downmixer. r=rillian Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44485/diff/9-10/
Comment on attachment 8739670 [details] MozReview Request: Bug 1248861: P12. Update AudioCompactor to account for alignment padding. r?rillian Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45341/diff/1-2/
Comment on attachment 8739670 [details] MozReview Request: Bug 1248861: P12. Update AudioCompactor to account for alignment padding. r?rillian https://reviewboard.mozilla.org/r/45341/#review42137 ::: dom/media/MediaData.h:224 (Diff revision 2) > // Returns false if memory couldn't be allocated. > bool EnsureCapacity(size_t aLength) > { > const CheckedInt<size_t> sizeNeeded = > CheckedInt<size_t>(aLength) * sizeof(Type) + AlignmentOffset() * 2; > `+ AlignmentPaddingSize()` here.
Attachment #8739670 - Flags: review?(giles) → review+
Comment on attachment 8739670 [details] MozReview Request: Bug 1248861: P12. Update AudioCompactor to account for alignment padding. r?rillian Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45341/diff/2-3/
Depends on: 1265794
Blocks: 1115967
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: