Closed Bug 1231793 Opened 8 years ago Closed 8 years ago

Change Wave decoder to Media Data Decoder / Demuxer

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: lchristie, Assigned: lchristie)

References

Details

Attachments

(8 files, 17 obsolete files)

1.60 KB, patch
Details | Diff | Splinter Review
1.78 KB, patch
Details | Diff | Splinter Review
1.08 KB, patch
Details | Diff | Splinter Review
1.48 KB, patch
Details | Diff | Splinter Review
4.38 KB, patch
Details | Diff | Splinter Review
3.35 KB, patch
Details | Diff | Splinter Review
38.02 KB, patch
Details | Diff | Splinter Review
1.22 KB, patch
jya
: review+
Details | Diff | Splinter Review
Change Wave decoder to Media Data Decoder / Demuxer
Assignee: nobody → lchristie
Priority: -- → P2
Attached patch bug-1231793-fix.patch (obsolete) — Splinter Review
Attachment #8705476 - Flags: review?(jyavenard)
Blocks: 851530
Attachment #8705476 - Flags: review?(jyavenard)
Depends on: 1239165
Comment on attachment 8705476 [details] [diff] [review]
bug-1231793-fix.patch

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

For all the new stuff we have kept the original MediaDecoder (here WaveDecoder) and used a pref to decide on what to use: either the old code, or the new MediaFormatReader

I would do the same here, so we can flip off the new code, and allow a smooth transition, and should things come to worse, disable it easily.

So you don't need a new WAVDecoder ; you can simply add (typically two lines) to the existing WaveDecoder and avoid creating new files.

BTW, I don't really like the WAV vs Wave naming. It's not consistent. Use one or the other thanks.

LGTM otherwise

::: dom/media/MP3Decoder.cpp
@@ -1,1 @@
> -

out of scope. nothing to do with WAV

::: dom/media/WAVDemuxer.cpp
@@ +31,5 @@
> +} // namespace wav_header
> +
> +// WAVDemuxer
> +
> +WAVDemuxer::WAVDemuxer(MediaResource * aSource)

MediaResource*

@@ +163,5 @@
> +  if (!riffHeader) {
> +    return false;
> +  }
> +  ByteReader* RIFFReader = new ByteReader(riffHeader->Data(), 12);
> +  mRIFFParser.Parse(RIFFReader);

you're leaking the ByteReader here.

You don't need to create it on the heap either. Could change RIFFParser::Parse to take a reference instead.

@@ +174,5 @@
> +  if (!aHeader) {
> +    return false;
> +  }
> +  ByteReader* HeaderReader = new ByteReader(aHeader->Data(), 8);
> +  mHeaderParser.Parse(HeaderReader);

same as RIFFParserInit: you're leaking your ByteReader, also make the HeaderParser::Parse take a reference instead

@@ +730,5 @@
> +HeaderParser::ChunkHeader::Update(uint8_t c) {
> +  if (mPos < SIZE) {
> +    mRaw[mPos] = c;
> +  }
> +  mPos++;

shouldn't you only increment mPos if you actually stored something in mRaw?
e.g. do mRaw[mPos++] instead

@@ +805,5 @@
> +
> +bool
> +FormatParser::FormatChunk::IsValid(int aPos) const {
> +  if (aPos == 3) {
> +    return (Channels() == 1 || Channels() == 2);

do not reject a particular number of channels, sampling rate or anything else for that matter.
It isn't up to the MediaDataDemuxer to make assumption on what is or what isn't supported.

The only thing reported as invalid is a broken file, not because it's multi-channels etc.

@@ +835,5 @@
> +
> +bool
> +FormatParser::FormatChunk::Update(uint8_t c) {
> +  if (mPos < MIN_SIZE) {
> +    mRaw[mPos] = c;

same comment as above mRaw[mPos++].

@@ +853,5 @@
> +}
> +
> +const DataParser::DataChunk&
> +DataParser::CurrentChunk() const {
> +  return mChunk;

I would put all the code related to DataParser (including the header) inside this file, and have all code inline instead. Much easier than having to navigate back and forth between definition and code, especially when the implementation is so small and much better suited for inline.

::: dom/media/WAVDemuxer.h
@@ +32,5 @@
> +private:
> +  // Synchronous Initialization.
> +  bool InitInternal();
> +
> +  RefPtr<MediaResource> mSource;

use MediaResourceIndex instead.

It provides an easy wrapper that handles partial read from the MediaResource etc..

@@ +59,5 @@
> +  };
> +
> +  const RIFFHeader& RiffHeader() const;
> +
> +  uint32_t Parse(mp4_demuxer::ByteReader* aReader);

take a ByteReader& instead here, see why in the comment inside WAVDemuxer.cpp

@@ +65,5 @@
> +  void Reset();
> +
> +private:
> +  RIFFHeader mRiffHeader;
> +};

this isn't used outside WavDemuxer, so it should be a private class. Put it in WAVDecoder.cpp

@@ +98,5 @@
> +  void Reset();
> +
> +private:
> +  ChunkHeader mHeader;
> +};

same here...

@@ +127,5 @@
> +  };
> +
> +  const FormatChunk& FmtChunk() const;
> +
> +  uint32_t Parse(mp4_demuxer::ByteReader* aReader);

I would make that a typedef , so there's no mp4_demuxer reference in there.. just look too weird.

@@ +133,5 @@
> +  void Reset();
> +
> +private:
> +  FormatChunk mFmtChunk;
> +};

same here...

::: dom/media/platforms/agnostic/WaveDecoder.cpp
@@ +16,5 @@
> +{
> +  uint32_t result = LittleEndian::readUint32(*aBuffer);
> +  *aBuffer += sizeof(uint32_t);
> +  return result;
> +}

Please use ByteReader instead, you can amend it to handle 24 bits int.

@@ +46,5 @@
> +{
> +  uint8_t result = uint8_t((*aBuffer)[0]);
> +  *aBuffer += sizeof(uint8_t);
> +  return result;
> +}

all of this need to be replaced with the appropriate ByteReader

@@ +77,5 @@
> +
> +template <> inline float
> +Signed24bIntToAudioSample<float>(int32_t aValue)
> +{
> +  return aValue / 8388608.0f;

please use static_cast<float>(2<<23);

@@ +83,5 @@
> +template <> inline int16_t
> +Signed24bIntToAudioSample<int16_t>(int32_t aValue)
> +{
> +  return aValue / 256;
> +}

a pity to have all those conversions here...
something I've been wanting to improve many times. I believe there are already conversion routines in AudioSampleFormat.h you could be using (anything with int16 and floats)

I wrote all of those with SSE2/SSE3: it's several order of magnitude faster, maybe time to import that
https://github.com/MythTV/mythtv/blob/master/mythtv/libs/libmyth/audio/audioconvert.cpp#L90

@@ +97,5 @@
> +{
> +  // Zero these member vars to avoid crashes in Wave clear functions when
> +  // destructor is called before |Init|.
> +  PodZero(&mPacketCount);
> +  PodZero(&mFrames);

uh what?

mPacketCount and mFrames are already initialised above. please remove

@@ +103,5 @@
> +
> +WaveDataDecoder::~WaveDataDecoder()
> +{
> +  PodZero(&mPacketCount);
> +  PodZero(&mFrames);

remove, unecessary and surely there's no advantage doing so over mPacketCount = 0 !

@@ +109,5 @@
> +
> +nsresult
> +WaveDataDecoder::Shutdown()
> +{
> +  //mReader = nullptr;

remove comment of dead code

@@ +142,5 @@
> +  }
> +}
> +
> +int
> +WaveDataDecoder::DoDecode(MediaRawData* aSample)

you only check the return value to check if it's -1.

So make this function return a bool, and return true upon success

@@ +156,5 @@
> +  for (int i = 0; i < frames; ++i) {
> +    for (unsigned int j = 0; j < mInfo.mChannels; ++j) {
> +      if (mInfo.mBitDepth == 8) {
> +        uint8_t v = ReadUint8(&aData);
> +        buffer[i * mInfo.mChannels + j] = 

trailing spaces

@@ +189,5 @@
> +                                  Move(buffer),
> +                                  mInfo.mChannels,
> +                                  mInfo.mRate));
> +  mFrames += frames;
> +    

trailing spaces

@@ +220,5 @@
> +/* static */
> +bool
> +WaveDataDecoder::IsWave(const nsACString& aMimeType)
> +{
> +  return aMimeType.EqualsLiteral("audio/x-wav");

that test is duplicated in other places.. I think it's clearer to simply check the mimetype where you need to check it, rather than define a sub-function for that

::: dom/media/platforms/agnostic/WaveDecoder.h
@@ +16,5 @@
> +public:
> +  WaveDataDecoder(const AudioInfo& aConfig,
> +                  FlushableTaskQueue* aTaskQueue,
> +                  MediaDataDecoderCallback* aCallback);
> +  ~WaveDataDecoder();

you don't need a destructor as you do nothing there (well you do, but really you shouldn't). default destructor is sufficient

::: dom/media/test/manifest.js
@@ +676,5 @@
>    },
>    { name:"wave_metadata_bad_len.wav", tags: {
>        name:"Track Title",
>        artist:"Artist Name",
> +      comments:"Comments",

this is out of scope, and need another patch
(In reply to Jean-Yves Avenard [:jya] from comment #13)
> Comment on attachment 8705476 [details] [diff] [review]
> bug-1231793-fix.patch
> 
> Review of attachment 8705476 [details] [diff] [review]:
> -----------------------------------------------------------------
[...]
> @@ +83,5 @@
> > +template <> inline int16_t
> > +Signed24bIntToAudioSample<int16_t>(int32_t aValue)
> > +{
> > +  return aValue / 256;
> > +}
> 
> a pity to have all those conversions here...
> something I've been wanting to improve many times. I believe there are
> already conversion routines in AudioSampleFormat.h you could be using
> (anything with int16 and floats)
> 
> I wrote all of those with SSE2/SSE3: it's several order of magnitude faster,
> maybe time to import that
> https://github.com/MythTV/mythtv/blob/master/mythtv/libs/libmyth/audio/
> audioconvert.cpp#L90

Reusing the routines in AudioSampleFormat.h makes sense. Optimizing them with SSE2/SSE3 is out of scope for this bug. We can follow up later on this in another bug.
(In reply to Jean-Yves Avenard [:jya] from comment #13)
> Comment on attachment 8705476 [details] [diff] [review]
> bug-1231793-fix.patch
> 
> Review of attachment 8705476 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> For all the new stuff we have kept the original MediaDecoder (here
> WaveDecoder) and used a pref to decide on what to use: either the old code,
> or the new MediaFormatReader
> 
> I would do the same here, so we can flip off the new code, and allow a
> smooth transition, and should things come to worse, disable it easily.
> 
> So you don't need a new WAVDecoder ; you can simply add (typically two
> lines) to the existing WaveDecoder and avoid creating new files.

Shouldn't the pref be with DecoderTraits.cpp? We should leave the content of media/wave unchanged to ensure that if we do disable the new code it would work exactly as before.

> @@ +65,5 @@
> > +  void Reset();
> > +
> > +private:
> > +  RIFFHeader mRiffHeader;
> > +};
> 
> this isn't used outside WavDemuxer, so it should be a private class. Put it
> in WAVDecoder.cpp
>
> @@ +98,5 @@
> > +  void Reset();
> > +
> > +private:
> > +  ChunkHeader mHeader;
> > +};
> 
> same here...
>
> @@ +133,5 @@
> > +  void Reset();
> > +
> > +private:
> > +  FormatChunk mFmtChunk;
> > +};
> 
> same here...

These classes are used in WAVTrackDemuxer::GetRIFFHeader() etc. We could put each SIZE outside each class, and then make each one private, but what would this add over leaving them public?
 
> @@ +77,5 @@
> > +
> > +template <> inline float
> > +Signed24bIntToAudioSample<float>(int32_t aValue)
> > +{
> > +  return aValue / 8388608.0f;
> 
> please use static_cast<float>(2<<23);

8388608 = 2 << 22

> @@ +220,5 @@
> > +/* static */
> > +bool
> > +WaveDataDecoder::IsWave(const nsACString& aMimeType)
> > +{
> > +  return aMimeType.EqualsLiteral("audio/x-wav");
> 
> that test is duplicated in other places.. I think it's clearer to simply
> check the mimetype where you need to check it, rather than define a
> sub-function for that

This was done for consistency in AgnosticDecoderModule.cpp:

>bool
>AgnosticDecoderModule::SupportsMimeType(const nsACString& aMimeType) const
>{
>  return VPXDecoder::IsVPX(aMimeType) ||
>    OpusDataDecoder::IsOpus(aMimeType) ||
>    VorbisDataDecoder::IsVorbis(aMimeType) ||
>    WaveDataDecoder::IsWave(aMimeType);
>}
(In reply to Louis Christie from comment #15)

> > So you don't need a new WAVDecoder ; you can simply add (typically two
> > lines) to the existing WaveDecoder and avoid creating new files.
> 
> Shouldn't the pref be with DecoderTraits.cpp? We should leave the content of
> media/wave unchanged to ensure that if we do disable the new code it would
> work exactly as before.
> 

No, DecoderTraits with all its static code is an abomination that needs to disappear: its impossible to maintain, everyone add its own style to it and there's been plenty of regression introduced whenever someone touched it.. (there's a bug to have classes self-register what they support and automatically being available without ever having to touch static code anywhere)

What I'm talking about is something like what the WebMDecoder used to do:
http://hg.mozilla.org/mozilla-central/file/3714873643124a5c65c938366db4eba3fc9cbcb4/dom/media/webm/WebMDecoder.cpp#l19

the choice to use the new media architecture or the older MediaDecoderReader one was really a matter of changing a single line in the MediaDecoder.

Here you're duplicating it entirely for no good reason. Once WaveReader will be removed, along with the now longer WaveDecoder you'll delete them all, losing all historical data while at it.

When you could instead simply remove a single line to remove the old code like this:
http://hg.mozilla.org/mozilla-central/diff/ce4623289f7f7fa28c7d005b61ba931190828f5b/dom/media/webm/WebMDecoder.cpp

> > 
> > same here...
> 
> These classes are used in WAVTrackDemuxer::GetRIFFHeader() etc. We could put
> each SIZE outside each class, and then make each one private, but what would
> this add over leaving them public?

Because it doesn't need to be public.
Easier to maintain, easier to read, easier for the next person to jump on the code, quicker to compile (as you're parsing the headers in all files including it)... The list is long.

Ultimately, it's just good practice and I'm the person reviewing your code :)

>  
> > @@ +77,5 @@
> > > +
> > > +template <> inline float
> > > +Signed24bIntToAudioSample<float>(int32_t aValue)
> > > +{
> > > +  return aValue / 8388608.0f;
> > 
> > please use static_cast<float>(2<<23);
> 
> 8388608 = 2 << 22

sorry I meant 1<<23 (2^23) it makes it much easier to understand where this value come from: you're converting a 24 bits *signed* integer. Just like above 32768 is 1<<15 because you're dealing with 16 bits signed integer.

It compiles the same, but anyone glancing at the code understand where that from.

> 
> > @@ +220,5 @@
> > > +/* static */
> > > +bool
> > > +WaveDataDecoder::IsWave(const nsACString& aMimeType)
> > > +{
> > > +  return aMimeType.EqualsLiteral("audio/x-wav");
> > 
> > that test is duplicated in other places.. I think it's clearer to simply
> > check the mimetype where you need to check it, rather than define a
> > sub-function for that
> 
> This was done for consistency in AgnosticDecoderModule.cpp:

I see..

okay then.
(In reply to Louis Christie from comment #15)

> 
> These classes are used in WAVTrackDemuxer::GetRIFFHeader() etc. We could put
> each SIZE outside each class, and then make each one private, but what would
> this add over leaving them public?

feel free to put that in a separate file.
Attached patch bug-1231793-fix-part1.patch (obsolete) — Splinter Review
Attachment #8715988 - Flags: review?(jyavenard)
Attached patch bug-1231793-fix-part2.patch (obsolete) — Splinter Review
Attachment #8715989 - Flags: review?(jyavenard)
Attached patch bug-1231793-fix-part3.patch (obsolete) — Splinter Review
Attachment #8715990 - Flags: review?(jyavenard)
Attached patch bug-1231793-fix-part4.patch (obsolete) — Splinter Review
Attachment #8715991 - Flags: review?(jyavenard)
Attached patch bug-1231793-fix-part5.patch (obsolete) — Splinter Review
Attachment #8715992 - Flags: review?(jyavenard)
Attached patch bug-1231793-fix-part6.patch (obsolete) — Splinter Review
Attachment #8705476 - Attachment is obsolete: true
Attachment #8715993 - Flags: review?(jyavenard)
Latest try run, with all six patches applied: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1a0b02a6e39
Attached patch bug-1231793-fix-part6.patch (obsolete) — Splinter Review
Added GetDescriptionName method to WaveDecoder.h
Attachment #8715993 - Attachment is obsolete: true
Attachment #8715993 - Flags: review?(jyavenard)
Attachment #8716011 - Flags: review?(jyavenard)
Comment on attachment 8715988 [details] [diff] [review]
bug-1231793-fix-part1.patch

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

LGTM, with the changes mentioned.

::: dom/media/test/test_decode_error.html
@@ +24,5 @@
>      el._sawError = true;
> +    // New behaviour means that mReadyState may not be set to HAVE_NOTHING
> +    // after the PDM shutsdown. In this case the "emptied" event is not fired
> +    // and so we must shutdown the test here.
> +    if (el.readyState != HTMLMediaElement.HAVE_NOTHING) {

why is that?

I'm not sure I see the relation between a new wav decoder and the HTMLMEdiaElement
This test for networkState is invalid to start with.

I would remove that entire test on networkState and related code.

calling manager.finished(token) at all time.
Attachment #8715988 - Flags: review?(jyavenard)
Comment on attachment 8715989 [details] [diff] [review]
bug-1231793-fix-part2.patch

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

::: media/libstagefright/binding/include/mp4_demuxer/ByteReader.h
@@ +120,5 @@
> +      return 0;
> +    }
> +    int32_t result = int32_t(ptr[2] << 16 | ptr[1] << 8 | ptr[0]);
> +    if ((ptr[2] & 0x80) == 0x80) {
> +      result = (result | 0xff000000);

This is undefined behaviour.

Unless it's something unique for WAV, but going by where this code is located and I would assume you only want to extend the sign from 24 bits to 32 bits.

I would do:
    if (result & 0x00800000u) {
        result -= 0x1000000
    }
Attachment #8715989 - Flags: review?(jyavenard)
(In reply to Jean-Yves Avenard [:jya] from comment #29)
> Comment on attachment 8715988 [details] [diff] [review]
> bug-1231793-fix-part1.patch
> 
> Review of attachment 8715988 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM, with the changes mentioned.
> 
> ::: dom/media/test/test_decode_error.html
> @@ +24,5 @@
> >      el._sawError = true;
> > +    // New behaviour means that mReadyState may not be set to HAVE_NOTHING
> > +    // after the PDM shutsdown. In this case the "emptied" event is not fired
> > +    // and so we must shutdown the test here.
> > +    if (el.readyState != HTMLMediaElement.HAVE_NOTHING) {
> 
> why is that?
> 
> I'm not sure I see the relation between a new wav decoder and the
> HTMLMEdiaElement
> This test for networkState is invalid to start with.
> 
> I would remove that entire test on networkState and related code.
> 
> calling manager.finished(token) at all time.

This isn't specific to the new Wav Decoder, it just happens to be that this test only tests Wave and Ogg files. I'll check with cpearce, we still might want the old test for the Ogg files.
Flags: needinfo?(cpearce)
Comment on attachment 8715990 [details] [diff] [review]
bug-1231793-fix-part3.patch

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

::: dom/media/AudioSampleFormat.h
@@ +105,5 @@
> +}
> +template <> inline int16_t
> +UInt8bitToAudioSample<int16_t>(uint8_t aValue)
> +{
> +  return int16_t(aValue * UINT16_MAX / UINT8_MAX + INT16_MIN);

This is also undefined C++ behaviour, as you may have an overflow when doing aValue * UINT16_MAX depending on how UINT16_MAX is defined. (Per Paragraph 5/4 of the C++11 Standard)

The other thing here is that you introduce noise (as for example 0xff will become 0x7f00 and not 0x7fff as expected)
a good trick is doing:
int16_t(aValue) << 8 + aValue

and to that you shift giving:
int16_t(aValue) << 8 + aValue + INT16_MIN
which will give you a nicer value (with higher SNR), we do lose the exact value of 0, but close to those ranges it's inaudible anyway.

Having said that, people listening to 8 bits audio probably don't expect much out.
Attachment #8715990 - Flags: review?(jyavenard)
Attachment #8715991 - Flags: review?(jyavenard) → review+
(In reply to Jean-Yves Avenard [:jya] from comment #29)
> Comment on attachment 8715988 [details] [diff] [review]
> bug-1231793-fix-part1.patch
> 
> Review of attachment 8715988 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM, with the changes mentioned.
> 
> ::: dom/media/test/test_decode_error.html
> @@ +24,5 @@
> >      el._sawError = true;
> > +    // New behaviour means that mReadyState may not be set to HAVE_NOTHING
> > +    // after the PDM shutsdown. In this case the "emptied" event is not fired
> > +    // and so we must shutdown the test here.
> > +    if (el.readyState != HTMLMediaElement.HAVE_NOTHING) {
> 
> why is that?

This is because the MediaFormatReader reaches HAVE_METADATA without decoding anything, and only after it's loaded metadata does it look for a PDM that can decode (which subsequently fails to decode). Whereas the old WaveReader would ensure it could decode before reporting loadedmetadata, so in this test would fail before advancing the readyState.

> 
> I'm not sure I see the relation between a new wav decoder and the
> HTMLMEdiaElement
> This test for networkState is invalid to start with.
> 
> I would remove that entire test on networkState and related code.
> 
> calling manager.finished(token) at all time.


I'd be fine with removing the lines that test readyState and networkState here. I don't think jya meant to remove the whole file, just the lines that test the readyState and networkState. I think there's still value in keeping the other tests here.
(In reply to Chris Pearce (:cpearce) from comment #33)

> This is because the MediaFormatReader reaches HAVE_METADATA without decoding
> anything, and only after it's loaded metadata does it look for a PDM that
> can decode (which subsequently fails to decode). Whereas the old WaveReader
> would ensure it could decode before reporting loadedmetadata, so in this
> test would fail before advancing the readyState.

And so it should...

readyState is a buffer monitoring issue.
assuming the value of readyState is to be HAVE_NOTHING when you have a decoding error sounds wrong to me

That may have well be the case with the old reader, but enforcing that rule is out of spec.

So I'd remove the test altogether. It can't regress the OggReader anyway, and hopefully, soon the ogg reader will be using MediaFormatReader and it will be exactly like wave :)
(In reply to Chris Pearce (:cpearce) from comment #33)
> I'd be fine with removing the lines that test readyState and networkState
> here. I don't think jya meant to remove the whole file, just the lines that
> test the readyState and networkState. I think there's still value in keeping
> the other tests here.

Oh, I didn't think it could have been interpreting that way.

Yes, I'm only talking about removing the check of readyState and the conditional state of the test completing if readyState have a particular value, everything else stay the same.
Comment on attachment 8716011 [details] [diff] [review]
bug-1231793-fix-part6.patch

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

why don't you re-use WaveDecoder instead of duplicating it into a new WAVDecoder?

Please split the change to DecoderTraits into another patch (which is about enabling it) and the new decoder/demuxer.

LGTM otherwise.

::: dom/media/WAVDecoder.cpp
@@ +31,5 @@
> +        new MediaFormatReader(this, new WAVDemuxer(GetResource()));
> +    return new MediaDecoderStateMachine(this, reader);
> +  } else {
> +    return new MediaDecoderStateMachine(this, new WaveReader(this));
> +  } 

trailing space

@@ +33,5 @@
> +  } else {
> +    return new MediaDecoderStateMachine(this, new WaveReader(this));
> +  } 
> +}
> +  

trailing spaces

::: dom/media/WAVDemuxer.cpp
@@ +178,5 @@
> +  RefPtr<MediaRawData> fmtChunk(GetFileHeader(FindFmtChunk()));
> +  if (!fmtChunk) {
> +    return false;
> +  }
> +  ByteReader fmtReader = ByteReader(fmtChunk->Data(), 

trailing space

@@ +190,5 @@
> +  uint32_t bytesRead = 0;
> +
> +  RefPtr<MediaRawData> infoTag(GetFileHeader(FindInfoTag()));
> +  ByteReader infoTagReader = ByteReader(infoTag->Data(), 4);
> +  if (infoTagReader.ReadU32() != 0x494E464F) {

can't you use a constant here?

GetFileHeader can return nullptr, but you are never testing the return values. please do.
You should also check that infoTag does contains more than 4 bytes.

As a rule, always check that there's enough bytes to read in the ByteReader before calling ReadXXX. This problem has bitten us many times with bad mp4 generated by the fuzzing team.

@@ +212,5 @@
> +      length = aChunkSize - bytesRead;
> +    }
> +
> +    MediaByteRange mRange = { mOffset, mOffset + length };
> +    RefPtr<MediaRawData> mChunkData(GetFileHeader(mRange));

need to test that mChunkData is not nullptr ; or assert if you are certain it's always true

@@ +374,5 @@
> +
> +TimeUnit
> +WAVTrackDemuxer::Duration() const {
> +  if (!mDataLength) {
> +    return TimeUnit::FromMicroseconds(-1);

I think this is a bad init value, as you are passing the content of Duration() directly to the MediaInfo.
So potentially you could return a negative duration which will cause various assert.

0 (or default TimeUnit()) seems more appropriate. Especially as everywhere you end up using it you test if it's > 0

@@ +377,5 @@
> +  if (!mDataLength) {
> +    return TimeUnit::FromMicroseconds(-1);
> +  }
> +
> +  uint32_t numSamples = mDataLength * 8 / mChannels / mSampleFormat;

could this overflow?

Should use CheckedInt32 instead.

@@ +385,5 @@
> +  if (USECS_PER_S * numSamples % mSamplesPerSecond > mSamplesPerSecond / 2) {
> +    numUSeconds++;
> +  }
> +
> +  return TimeUnit::FromMicroseconds(numUSeconds);

from Microseconds takes an int64_t, so you have potentially an overflow here.
It won't ever happen here, but for clarity, numUSeconds should be a int64_t.

BTW, you have some routines to do all of this in VideoUtils, should try to reuse those.

@@ +391,5 @@
> +
> +TimeUnit
> +WAVTrackDemuxer::Duration(int64_t aNumDataChunks) const {
> +  if (!mSamplesPerSecond) {
> +    return TimeUnit::FromMicroseconds(-1);

same as above, I would use 0 as default value

@@ +489,5 @@
> +    datachunk->mDuration = Duration(1).ToMicroseconds();
> +  } else {
> +    uint32_t mBytesRemaining = mDataLength -
> +                               mChunkIndex *
> +                               wav_header::DATA_CHUNK_SIZE;

not a fan of this formatting, 
typically we do:
uint32_t mBytesRemaining =
  mDataLength ....

@@ +545,5 @@
> +WAVTrackDemuxer::ChunkIndexFromTime(const media::TimeUnit& aTime) const {
> +  int64_t chunkIndex = (aTime.ToSeconds() *
> +                        mSamplesPerSecond /
> +                        mSamplesPerChunk) -
> +                       1;

that is weird formatting really

@@ +570,5 @@
> +    aSize = std::min<int64_t>(aSize, streamLen - aOffset);
> +  }
> +
> +  uint32_t read = 0;
> +

remove empty line

@@ +758,5 @@
> +}
> +
> +uint16_t
> +FormatParser::FormatChunk::WaveFormat() const {
> +  return ((mRaw[1] << 8) | (mRaw[0]));

do you need the extra parenthesis? seem redundant to me.

::: dom/media/WAVDemuxer.h
@@ +14,5 @@
> +using mp4_demuxer::ByteReader;
> +
> +namespace mozilla {
> +
> +namespace wav_header {

do you need a new namespace?

@@ +42,5 @@
> +      TrackInfo::TrackType aType, uint32_t aTrackNumber) override;
> +  bool IsSeekable() const override;
> +
> +  // Unsure if needed
> +  bool ShouldComputeStartTime() const override { return false; }

you don't need that. the default implementation returns false.

@@ +194,5 @@
> +
> +  // MediaTrackDemuxer interface.
> +  UniquePtr<TrackInfo> GetInfo() const override;
> +  RefPtr<SeekPromise> Seek(media::TimeUnit aTime) override;
> +  RefPtr<SamplesPromise> GetSamples(int32_t aNumSamples = 1) override;

you don't need to set the default value here

::: dom/media/platforms/agnostic/AgnosticDecoderModule.cpp
@@ +62,2 @@
>    }
> +  

trailing spaces

::: dom/media/platforms/agnostic/WaveDecoder.cpp
@@ +73,5 @@
> +    for (unsigned int j = 0; j < mInfo.mChannels; ++j) {
> +      if (mInfo.mBitDepth == 8) {
> +        uint8_t v = aReader.ReadU8();
> +        buffer[i * mInfo.mChannels + j] =
> +            UInt8bitToAudioSample<AudioDataValue>(v);

A ToAudioSample<int, AudioDataValue> format would have been much more elegant IMHO

@@ +88,5 @@
> +  }
> +
> +  aReader.DiscardRemaining();
> +
> +  CheckedInt64 duration = frames / mInfo.mRate;

How could you ever overflow here? this is unnecessary precaution.

@@ +95,5 @@
> +    return false;
> +  }
> +  CheckedInt64 time = aTstampUsecs;
> +  if (!time.isValid()) {
> +    NS_WARNING("Int overflow adding total_duration and aTstampUsecs");

that's impossible, you peform no calculation on time: it's unecessary

::: dom/media/platforms/agnostic/WaveDecoder.h
@@ +32,5 @@
> +  // Return true if mimetype is Wave
> +  static bool IsWave(const nsACString& aMimeType);
> +
> +private:
> +  void Decode (MediaRawData* aSample);

style: no extra space here.

@@ +33,5 @@
> +  static bool IsWave(const nsACString& aMimeType);
> +
> +private:
> +  void Decode (MediaRawData* aSample);
> +  bool DoDecode (MediaRawData* aSample);

same

@@ +34,5 @@
> +
> +private:
> +  void Decode (MediaRawData* aSample);
> +  bool DoDecode (MediaRawData* aSample);
> +  void DoDrain ();

and same
Attachment #8716011 - Flags: review?(jyavenard)
Comment on attachment 8715992 [details] [diff] [review]
bug-1231793-fix-part5.patch

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

::: dom/media/platforms/android/AndroidDecoderModule.cpp
@@ +259,5 @@
>    }
>  
> +  // When checking "audio/x-wav", CreateDecoder can cause a JNI ERROR by
> +  // Accessing a stale local reference leading to a SIGSEGV crash.
> +  // To avoid this we check for wav types here.

not sure I follow here, is this a bug in CreateDecoder?

@@ +264,5 @@
> +  if (aMimeType.EqualsLiteral("audio/x-wav") ||
> +      aMimeType.EqualsLiteral("audio/wave; codecs=1") ||
> +      aMimeType.EqualsLiteral("audio/wave; codecs=65534")) {
> +    return false;
> +  }  

trailing space.
Attachment #8715992 - Flags: review?(jyavenard) → review+
(In reply to Louis Christie from comment #31)
> This isn't specific to the new Wav Decoder, it just happens to be that this
> test only tests Wave and Ogg files. I'll check with cpearce, we still might
> want the old test for the Ogg files.

It's still worth having the Ogg files tested.
Flags: needinfo?(cpearce)
Attached patch bug-1231793-fix-part1.patch (obsolete) — Splinter Review
Attachment #8715988 - Attachment is obsolete: true
Attachment #8718176 - Flags: review?(jyavenard)
Attached patch bug-1231793-fix-part2.patch (obsolete) — Splinter Review
Attachment #8715989 - Attachment is obsolete: true
Attachment #8718177 - Flags: review?(jyavenard)
Attached patch bug-1231793-fix-part3.patch (obsolete) — Splinter Review
Attachment #8715990 - Attachment is obsolete: true
Attachment #8718178 - Flags: review?(jyavenard)
Attached patch bug-1231793-fix-part6.patch (obsolete) — Splinter Review
Attachment #8716011 - Attachment is obsolete: true
Attachment #8718193 - Flags: review?(jyavenard)
Attached patch bug-1231793-fix-part7.patch (obsolete) — Splinter Review
Attachment #8718194 - Flags: review?(jyavenard)
Attached patch bug-1231793-fix-part6.patch (obsolete) — Splinter Review
Fixed the old WaveDecoder.cpp to ensure Wave playback without patch 7 applied.
Attachment #8718193 - Attachment is obsolete: true
Attachment #8718193 - Flags: review?(jyavenard)
Attachment #8718505 - Flags: review?(jyavenard)
Attached patch bug-1231793-fix-part7.patch (obsolete) — Splinter Review
Amended to reflect the changes in part 6.
Attachment #8718194 - Attachment is obsolete: true
Attachment #8718194 - Flags: review?(jyavenard)
Attachment #8718508 - Flags: review?(jyavenard)
Comment on attachment 8718176 [details] [diff] [review]
bug-1231793-fix-part1.patch

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

r=me.

This shouldn't be part1 though as it will only work after applying the new demuxer.

I like to have the patches in logical order, and each on their own should pass.
If you were to bisect the code and stop at this commit, the mochitest would fail.
Attachment #8718176 - Flags: review?(jyavenard) → review+
Attachment #8718177 - Flags: review?(jyavenard) → review+
Attachment #8718178 - Flags: review?(jyavenard) → review+
Comment on attachment 8718508 [details] [diff] [review]
bug-1231793-fix-part7.patch

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

r=me with the following changes applied:

define the media.wave.decoder.enabled preference in the modules/libpref/init/all.js and have it there set to true by default.

no need to modify WaveDecoder.cpp
Attachment #8718508 - Flags: review?(jyavenard) → review+
(In reply to Jean-Yves Avenard [:jya] from comment #46)

> This shouldn't be part1 though as it will only work after applying the new
> demuxer.
> 
> I like to have the patches in logical order, and each on their own should
> pass.
> If you were to bisect the code and stop at this commit, the mochitest would
> fail.

True, it could be folded into part 7 (because the tests would fail after part 7 without the changes here).
(In reply to Jean-Yves Avenard [:jya] from comment #47)
> Comment on attachment 8718508 [details] [diff] [review]
> bug-1231793-fix-part7.patch
> 
> Review of attachment 8718508 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with the following changes applied:
> 
> define the media.wave.decoder.enabled preference in the
> modules/libpref/init/all.js and have it there set to true by default.
> 
> no need to modify WaveDecoder.cpp

I will add it there, as part of patch 6, but set it to false (so that it uses the WaveReader and would pass tests without the last patch applied), and set it to true as part of this one.
Comment on attachment 8718505 [details] [diff] [review]
bug-1231793-fix-part6.patch

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

r=me with style changes addressed.

I know the MP3 demuxer did it, but that was wrong. So let's start properly for this one:
extracted from: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

"For small functions, constructors, or other braced constructs, it's okay to collapse the definition to one line as shown for TinyFunction above. For larger ones use something similar to method declarations below.
Methods and functions
C/C++

In C/C++, method names should be capitalized and use CamelCase.

template<typename T>  // Templates on own line.
static int            // Return type on own line for top-level functions.
MyFunction(...)
{
  ...
}

int
MyClass::Method(...)
{
  ...
}
|

not:
int
MyClass::Method(...) {
  ...
}

so please change all braces in WAVDemuer.cpp for function declaration.

Also, I argue the name WAVDemuxer/WAVDecoder

It used to be wave, even in WAVDecoder you say "wave audio decoder"

so WAV vs WAVE , I prefer WAVE as that's what was there to start with for better or worse.
currently it's inconsistent: WAVDemuxer going with WaveDecoder etc... same for the casing.

And why moved the code away from the wave folder? I liked it there :)

That the MP3 demuxer isn't in its own mp3 folder is the wrong thing IMHO.

::: dom/media/WAVDemuxer.cpp
@@ +188,5 @@
> +bool
> +WAVTrackDemuxer::ListChunkParserInit(uint32_t aChunkSize) {
> +  uint32_t bytesRead = 0;
> +
> +  RefPtr<MediaRawData> infoTag(GetFileHeader(FindInfoTag()));

use this format instead.

RefPtr<MediaRawData> infoTag = GetFileHeader(FindInfoTag())

same comment applies wherever you're declating RefPtr<MediaRawData>

@@ +189,5 @@
> +WAVTrackDemuxer::ListChunkParserInit(uint32_t aChunkSize) {
> +  uint32_t bytesRead = 0;
> +
> +  RefPtr<MediaRawData> infoTag(GetFileHeader(FindInfoTag()));
> +  if (infoTag == nullptr) {

if (!infoTag)

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style
"When testing a pointer, use (!myPtr) or (myPtr); don't use myPtr != nullptr or myPtr == nullptr."

@@ +227,5 @@
> +    if (length > 0 && val[length - 1] == '\0') {
> +      val.SetLength(length - 1);
> +    }
> +
> +    if (length % 2 != 0) {

if (length %2)

@@ +287,5 @@
> +}
> +
> +TimeUnit
> +WAVTrackDemuxer::FastSeek(const TimeUnit& aTime) {
> +  if (!aTime.ToMicroseconds()) {

if (aTime.ToMicroseconds) {
and reverse the two blocks. save an extra not

@@ +552,5 @@
> +}
> +
> +void
> +WAVTrackDemuxer::UpdateState(const MediaByteRange& aRange) {
> +  if (aRange.Length() < 0) {

how could that ever be true?

@@ +654,5 @@
> +// HeaderParser
> +
> +uint32_t
> +HeaderParser::Parse(ByteReader& aReader) {
> +  MOZ_ASSERT(&aReader);

this can never be false as a reference can't point to nullptr.
please remove

or if you want to use pointer instead and test for nullptr take ByteReader*
Attachment #8718505 - Flags: review?(jyavenard) → review+
Attachment #8718177 - Attachment is obsolete: true
Attachment #8718178 - Attachment is obsolete: true
Attachment #8715991 - Attachment is obsolete: true
Attachment #8715992 - Attachment is obsolete: true
Attached patch bug-1231793-fix-part-5.patch (obsolete) — Splinter Review
Attachment #8718505 - Attachment is obsolete: true
Attachment #8718508 - Attachment is obsolete: true
Attachment #8718176 - Attachment is obsolete: true
Attached patch bug-1231793-fix-part-5.patch (obsolete) — Splinter Review
Attachment #8718663 - Attachment is obsolete: true
Blocks: 1245546
Attachment #8718667 - Attachment is obsolete: true
Keywords: checkin-needed
Attachment #8720011 - Flags: review+
Depends on: 1287397
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: