Last Comment Bug 778053 - Support WAV tag metadata
: Support WAV tag metadata
Status: RESOLVED FIXED
[mentor=rillian] [lang=c++]
:
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla20
Assigned To: ChangZhuo Chen
:
Mentors:
Depends on: 763010 1010163 1080986
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-26 23:21 PDT by Ralph Giles (:rillian) needinfo me
Modified: 2014-10-12 16:16 PDT (History)
8 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
[patch] read wave metadata (13.55 KB, patch)
2012-09-25 07:13 PDT, ChangZhuo Chen
no flags Details | Diff | Splinter Review
[patch] mochitest for wave metadata (67.94 KB, patch)
2012-09-25 07:14 PDT, ChangZhuo Chen
no flags Details | Diff | Splinter Review
[patch] mochitest for wave metadata (81.25 KB, patch)
2012-09-26 04:23 PDT, ChangZhuo Chen
no flags Details | Diff | Splinter Review
[patch] mochitest for wave metadata (81.29 KB, patch)
2012-09-26 04:35 PDT, ChangZhuo Chen
no flags Details | Diff | Splinter Review
[patch] read wave metadata (94.61 KB, patch)
2012-09-26 05:16 PDT, ChangZhuo Chen
no flags Details | Diff | Splinter Review
[patch] read wave metadata (13.55 KB, patch)
2012-09-30 06:55 PDT, ChangZhuo Chen
no flags Details | Diff | Splinter Review
[patch] mochitest for wave metadata (81.28 KB, patch)
2012-09-30 06:55 PDT, ChangZhuo Chen
no flags Details | Diff | Splinter Review
666320: [patch] read wave metadata (13.56 KB, patch)
2012-10-06 04:24 PDT, ChangZhuo Chen
czchen: review+
Details | Diff | Splinter Review
[patch] read wave metadata (13.56 KB, patch)
2012-10-06 04:26 PDT, ChangZhuo Chen
no flags Details | Diff | Splinter Review
[patch] mochitest for wave metadata (81.29 KB, patch)
2012-10-06 04:27 PDT, ChangZhuo Chen
no flags Details | Diff | Splinter Review
[patch] read wave metadata (13.56 KB, patch)
2012-10-06 04:31 PDT, ChangZhuo Chen
kinetik: review-
Details | Diff | Splinter Review
[patch] mochitest for wave metadata (81.29 KB, patch)
2012-10-06 04:32 PDT, ChangZhuo Chen
kinetik: review+
Details | Diff | Splinter Review
[patch] read wave metadata (13.36 KB, patch)
2012-11-04 03:35 PST, ChangZhuo Chen
kinetik: review+
Details | Diff | Splinter Review
[patch] read wave metadata (13.39 KB, patch)
2012-11-25 07:04 PST, ChangZhuo Chen
no flags Details | Diff | Splinter Review
[patch] read wave metadata (13.39 KB, patch)
2012-11-25 07:06 PST, ChangZhuo Chen
kinetik: review+
Details | Diff | Splinter Review

Description Ralph Giles (:rillian) needinfo me 2012-07-26 23:21:28 PDT
WAV audio files can have metadata giving title, artist, etc.

This bug proposes to modify nsWaveReader so as to provide those tags to webcontent via ::ReadMetadata() and media.mozGetMetadata().
Comment 1 Thomasy 2012-09-16 06:03:24 PDT
(In reply to Ralph Giles (:rillian) from comment #0)
> WAV audio files can have metadata giving title, artist, etc.
> 
> This bug proposes to modify nsWaveReader so as to provide those tags to
> webcontent via ::ReadMetadata() and media.mozGetMetadata().

rillian ChangZhuo is
Comment 2 Thomasy 2012-09-16 06:04:26 PDT
rillian,
 ChangZhuo is interested in this bug. And I think he can help.
Comment 3 Ralph Giles (:rillian) needinfo me 2012-09-17 14:47:19 PDT
Thanks, Thomasy, for the introduction. Hi ChangZhuo, thanks for taking an interest in this bug!

Do you know how you want to proceed? In general idea is to replace the *aTags = nullptr assignment in nsWaveReader::ReadMetadata() with the output of a new function. That function, called something like nsWaveReader::GetTags(), creates and returns a pointer to a new nsHTMLMediaElement::MetadataTags hash table. The example in nsOggReader.cpp may be a helpful guide.

I know less about the WAVE metadata chunk, and what we should support there, but I now the free Audacity programme can create .wav files with embedded metadata. That's a good place to start.

Please let me know right away if you have any questions. I'm happy to help.
Comment 4 ChangZhuo Chen 2012-09-18 03:57:02 PDT
Hi Ralph,

For wave format, I found a link[1] that describe how wave store the metadate. The format is simple tag-length-value structure with ASCII tag name, and we only need to extract the INFO list chunk in LIST chunk to get the metadata. However, there are some problems I need advice:

1. There is no chunk order

In current design, it assumes fmt chunk always appears before data chunk. It is reasonable because we need fmt chunk to decode data chunk. However, the order of chunk is not specified in spec, so we cannot use nsWaveReader::ScanForwardUntil() to find the LIST chunk.

My plan for this is adding a new function nsWaveReader::GetNextChunk() to get the next chunk and parse it if necessary. However, in this case, we need to use mDecoder->GetResource()->Seek() to adjust resource pointer, and I am not sure if it causes other side effect.

2. ReentrantMonitorAutoEnter

ReentrantMonitorAutoEnter looks like a RAII class, but I do not know the purpose if this. Please tell me if anyone know about this class, thanks.


ref:
[1] http://www-mmsp.ece.mcgill.ca/Documents/AudioFormats/WAVE/WAVE.html

Regards,
ChangZhuo
Comment 5 Paul Adenot (:padenot) 2012-09-18 10:09:06 PDT
ReentrantMonitorAutoEnter is indeed an RAII class that enter and exits the monitor that is passed during its construction. A monitor is a synchronization object, I'm not sure how familiar you are with this concept, see [1] ([4] if you want to read the relevant code, click on the links to navigate). In your context (the nsWaveReader), you enter the decoder monitor (see [2]) when you want to call certain methods of the state machine, that change its state.

For example in [2], the call informs the state machine of the new duration you found while decoding the media. Because the duration can be accessed by the state machine thread and the decoder thread, we need some kind of synchronization. At that point, you should probably read [5] to have an overview of how the media code is architectured in Gecko so the last paragraph makes sense.

Anyway. Usually, if you wonder whether you should call some method while being in a monitor, check the definition of the method you want to call, and if it contains |mDecoder->GetReentrantMonitor().AssertCurrentThreadIn();|, that means this will Assert that the Current Thread is In the monitor, and blow up (in a debug build), if you forgot to use a monitor.

[1]: http://en.wikipedia.org/wiki/Monitor_%28synchronization%29
[2]: http://mxr.mozilla.org/mozilla-central/source/content/media/wave/nsWaveReader.cpp#141
[3]: http://mxr.mozilla.org/mozilla-central/source/content/media/nsBuiltinDecoderStateMachine.cpp#1349
[4]: http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/ReentrantMonitor.h#168
[5]: http://blog.pearce.org.nz/2011/02/firefox-4-video-decoder-architecture.html
Comment 6 Ralph Giles (:rillian) needinfo me 2012-09-18 16:38:01 PDT
(In reply to ChangZhuo Chen from comment #4)

> My plan for this is adding a new function nsWaveReader::GetNextChunk() to
> get the next chunk and parse it if necessary. However, in this case, we need
> to use mDecoder->GetResource()->Seek() to adjust resource pointer, and I am
> not sure if it causes other side effect.

Calling ::GetNextChunk and dispatching to the various parsers sounds reasonable, but we want to be careful not to scan the whole file looking for metadata. It should probably start decoding as soon as it's seen both the fmt and data chunk headers.

Are there files with the LIST::INFO chunk at the end? We could seek past the data chunk (and any other large chunks) to find that, but over the network this always adds delay.
Comment 7 ChangZhuo Chen 2012-09-19 05:32:26 PDT
(In reply to Paul ADENOT (:padenot) from comment #5)

Hi Paul,

Thank for your explanation. So the reason why ReentrantMonitorAutoEnter is needed in [1] and [2] is because nsWaveReader tries to update the data like sample rate, channel, frame size, ... that will affect the decoder. If I do not update anything related to decode, ReentrantMonitorAutoEnter is not needed, right?


[1] http://mxr.mozilla.org/mozilla-central/source/content/media/wave/nsWaveReader.cpp#449
[2] http://mxr.mozilla.org/mozilla-central/source/content/media/wave/nsWaveReader.cpp#481
Comment 8 ChangZhuo Chen 2012-09-19 06:38:25 PDT
(In reply to Ralph Giles (:rillian) from comment #6)

According to [1], "There are no restrictions upon the order of the chunks within a WAVE file, with the exception that the Format chunk must precede the Data chunk.", so we cannot know where the LIST::INFO chunk is until finding it. However, because putting LIST::INFO chunk after data chunk is not streaming friendly, I think we can just stop parsing after data chunk is found. If there is no LIST::INFO before data chunk, there is possible no LIST::INFO chunk at all.

[1] http://www.blitter.com/~russtopia/MIDI/~jglatt/tech/wave.htm
Comment 9 Ralph Giles (:rillian) needinfo me 2012-09-19 13:16:13 PDT
(In reply to ChangZhuo Chen from comment #8)
> I think we can just stop parsing after data chunk is found.

I think that's the best plan for now. Once that's working we can look at whether seeking past the data chunk makes sense.

 -r
Comment 10 ChangZhuo Chen 2012-09-24 07:30:40 PDT
The [1] is full list of metadata stored in LIST::INFO. Currently I just implement the following tags:

IART as artist
ICMT as comments
IGNR as genre
INAM as name
IPRD as product

Do we need to implement other tags? or these tags are enough?

For the `name' and `product', the names used in ogv and ogg are `title' and `album'. Do we need to synchronize tag names between wav and ogg?


[1] http://www.warpspeed.com.au/cgi-bin/inf2html.cmd?..%5Chtml%5Cbook%5CToolkt40%5CMMREF3.INF+2218
[2] http://dxr.mozilla.org/mozilla-central/content/media/test/manifest.js.html#l323
Comment 11 Ralph Giles (:rillian) needinfo me 2012-09-24 09:49:29 PDT
(In reply to ChangZhuo Chen from comment #10)
> The [1] is full list of metadata stored in LIST::INFO. Currently I just
> implement the following tags:
> 
> IART as artist
> ICMT as comments
> IGNR as genre
> INAM as name
> IPRD as product
> 
> Do we need to implement other tags? or these tags are enough?

I'd like ICRD as date as well.

> For the `name' and `product', the names used in ogv and ogg are `title' and
> `album'. Do we need to synchronize tag names between wav and ogg?

Ah! I wondered what people used for the album tag.

Just use the native tag names. The plan is to provide another call that maps tags from every format to a shared vocabulary, in bug 793737. So for the mozGetMetadata interface it's best to just return them in whatever the native tag set is. Either the fourcc, or your mapping to english above.

Does that sound good to you?
Comment 12 Ralph Giles (:rillian) needinfo me 2012-09-24 10:02:56 PDT
BTW, if you go with the fourcc codes, you could just return everything in the LIST::INFO without having to guess which would be important. That might be better.
Comment 13 ChangZhuo Chen 2012-09-25 07:13:28 PDT
Created attachment 664494 [details] [diff] [review]
[patch] read wave metadata
Comment 14 ChangZhuo Chen 2012-09-25 07:14:34 PDT
Created attachment 664495 [details] [diff] [review]
[patch] mochitest for wave metadata
Comment 15 ChangZhuo Chen 2012-09-25 07:27:59 PDT
Can someone help me to submit the test on try server? My account is not ready yet.
Comment 16 Thomasy 2012-09-25 09:17:30 PDT
(In reply to ChangZhuo Chen from comment #15)
> Can someone help me to submit the test on try server? My account is not
> ready yet.
Please check

https://tbpl.mozilla.org/?tree=Try&rev=588c42a8ef5d

If you have any question do not hesitate to ping me on IRC.
Comment 17 Mozilla RelEng Bot 2012-09-25 10:15:35 PDT
Try run for 588c42a8ef5d is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=588c42a8ef5d
Results (out of 14 total builds):
    failure: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/thomas@thomasy.tw-588c42a8ef5d
Comment 18 Ralph Giles (:rillian) needinfo me 2012-09-25 11:39:14 PDT
You need to 'git add' or 'hg add' the new test files, I guess.

BTW, it might be useful to have test files with zero and one comment, and an invalid comment the reader should not pass on, as well.
Comment 19 ChangZhuo Chen 2012-09-26 04:23:16 PDT
Created attachment 664887 [details] [diff] [review]
[patch] mochitest for wave metadata
Comment 20 ChangZhuo Chen 2012-09-26 04:35:33 PDT
Created attachment 664890 [details] [diff] [review]
[patch] mochitest for wave metadata
Comment 21 ChangZhuo Chen 2012-09-26 04:37:33 PDT
I updated the test cases. Now the following scenarios are tested:

- No INFO tag tag (wavedata_u8.wav)
- The INFO tag contains ASCII string (wave_metadata.wav)
- The INFO tag contains UTF-8 string (wave_metadata_utf-8.wav)

- The INFO tag contains bad length (wave_metadata_bad_len.wav)
- The INFO tag string does not null-terminated (wave_metadata_bad_no_null.wav)
- The INFO tag contains invalid UTF-8 character (wave_metadata_bad_utf8.wav)
- The INFO tag contains unknown tag (wave_metadata_unknown_tag.wav)
Comment 22 ChangZhuo Chen 2012-09-26 05:16:04 PDT
Created attachment 664902 [details] [diff] [review]
[patch] read wave metadata
Comment 23 Mozilla RelEng Bot 2012-09-26 06:00:32 PDT
Try run for 51de811b210e is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=51de811b210e
Results (out of 14 total builds):
    failure: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/thomas@thomasy.tw-51de811b210e
Comment 24 Mozilla RelEng Bot 2012-09-28 23:00:36 PDT
Try run for 744dad3c2468 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=744dad3c2468
Results (out of 16 total builds):
    exception: 16
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/czchen@gmail.com-744dad3c2468
Comment 25 Mozilla RelEng Bot 2012-09-29 00:00:35 PDT
Try run for b3fb0cbe6ff1 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=b3fb0cbe6ff1
Results (out of 321 total builds):
    exception: 1
    success: 299
    warnings: 21
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/thomas@thomasy.tw-b3fb0cbe6ff1
Comment 26 Ralph Giles (:rillian) needinfo me 2012-09-29 07:40:24 PDT
Don't forget to update the attached bug too. :)
Comment 27 Ralph Giles (:rillian) needinfo me 2012-09-29 07:40:57 PDT
Attached patch, rather. Too early in the morning.
Comment 28 ChangZhuo Chen 2012-09-30 06:55:25 PDT
Created attachment 666320 [details] [diff] [review]
[patch] read wave metadata
Comment 29 ChangZhuo Chen 2012-09-30 06:55:57 PDT
Created attachment 666321 [details] [diff] [review]
[patch] mochitest for wave metadata
Comment 30 ChangZhuo Chen 2012-09-30 06:57:15 PDT
I updated the attached patches. Please help to check if there is any problem, thanks.
Comment 31 Mozilla RelEng Bot 2012-09-30 07:30:30 PDT
Try run for 1c3d8fdcbdc8 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=1c3d8fdcbdc8
Results (out of 290 total builds):
    success: 274
    warnings: 16
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/czchen@gmail.com-1c3d8fdcbdc8
Comment 32 Thomasy 2012-09-30 07:47:06 PDT
(In reply to ChangZhuo Chen from comment #30)
> I updated the attached patches. Please help to check if there is any
> problem, thanks.

As a starter, if you want to get a formal review. You should set the review flag to r? and the reviewer to kinetik(kinetik@flim.org).
Comment 33 ChangZhuo Chen 2012-09-30 08:50:03 PDT
(In reply to Thomasy from comment #32)
> As a starter, if you want to get a formal review. You should set the review
> flag to r? and the reviewer to kinetik(kinetik@flim.org).

I updated the review flag. Thanks for the help.
Comment 34 ChangZhuo Chen 2012-10-06 04:24:18 PDT
Created attachment 668758 [details] [diff] [review]
666320: [patch] read wave metadata
Comment 35 ChangZhuo Chen 2012-10-06 04:26:34 PDT
Created attachment 668759 [details] [diff] [review]
[patch] read wave metadata
Comment 36 ChangZhuo Chen 2012-10-06 04:27:24 PDT
Created attachment 668760 [details] [diff] [review]
[patch] mochitest for wave metadata
Comment 37 ChangZhuo Chen 2012-10-06 04:31:26 PDT
Created attachment 668761 [details] [diff] [review]
[patch] read wave metadata
Comment 38 ChangZhuo Chen 2012-10-06 04:32:23 PDT
Created attachment 668762 [details] [diff] [review]
[patch] mochitest for wave metadata
Comment 39 Matthew Gregan [:kinetik] 2012-10-22 19:04:57 PDT
Comment on attachment 668761 [details] [diff] [review]
[patch] read wave metadata

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

Thanks for the patch!  I apologize for taking so long to review it.  I meant to get to it last week, but it slipped off of my radar.

::: content/media/wave/nsWaveReader.cpp
@@ +134,5 @@
> +  bool findDataOffset = false;
> +  nsHTMLMediaElement::MetadataTags *tag = nullptr;
> +
> +  // The spec does not mention abort the order of of fmt, data, LIST chunks, so
> +  // we try to find them one by one.

IBM/MS RIFF/MCI Specification 1.0 states:

    The WAVE form is defined as follows. Programs must expect (and ignore) any unknown chunks encountered, as with all RIFF forms. However, <fmt-ck> must always occur before <wave-data>, and both of these chunks are mandatory in a WAVE file.

And the INFO chunk is under the LIST chunk at a higher level in the tree, so the only valid orders are:

RIFF
WAVE
  FMT
  DATA
LIST
  INFO

or

RIFF
LIST
  INFO
WAVE
  FMT
  DATA

So please restrict the code to accept only those forms, as the previous code did.

@@ +161,5 @@
> +        break;
> +
> +      case LIST_CHUNK_MAGIC:
> +        // LIST chunk is optional, so it is okay if we cannot read it.
> +        LoadListChunk(chunkSize, &tag);

This will leak tag when the FRMT or DATA branch return an error.

@@ +173,5 @@
> +
> +    // RIFF chunks are two-byte aligned, so round up if necessary.
> +    chunkSize += chunkSize % 2;
> +
> +    mDecoder->GetResource()->Seek(nsISeekableStream::NS_SEEK_SET, offset + CHUNK_HEADER_SIZE + chunkSize);

You need to test for an error and deal with it.  It's not always possible to seek in streams, so that case needs to be handled.

The old code had the nice attribute that it parsed the stream linearly, which these changes breaks by expecting to seek between chunks.  I'm not *that* happy about that, because it could make loading some files significantly slower... and as I mentioned above, we still need to handle the case where the stream isn't seekable.

Can we delay loading this metadata until later in some cases?  Or perhaps for a first-pass at this, we could only load the metadata here it it appears before the WAVE chunk... but that's going to exclude an awful lot of files, I suspect.

@@ +178,5 @@
> +  }
> +
> +  // Ensure mandatory information are read.
> +  if (!(loadFormatChunk && findDataOffset)) {
> +    return NS_ERROR_FAILURE;

This will leak tag too.

@@ +181,5 @@
> +  if (!(loadFormatChunk && findDataOffset)) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  mDecoder->GetResource()->Seek(nsISeekableStream::NS_SEEK_SET, mWavePCMOffset);

Test for error.

@@ +556,5 @@
> +  // List chunks are always word (two byte) aligned.
> +  NS_ABORT_IF_FALSE(mDecoder->GetResource()->Tell() % 2 == 0,
> +                    "LoadListChunk called with unaligned resource");
> +
> +  nsAutoArrayPtr<char> chunk(new char[aChunkSize + 1]);

aChunkSize comes directly from the file, so this could be a huge value, including UINT32_MAX, which will overflow.

@@ +570,5 @@
> +
> +  // If there are multiple LIST::INFO in wave, we only parse the first one, the
> +  // others are ignored.
> +  if (*aTags) {
> +    return false;

I'd rather this check was removed and the caller avoided calling this function if the tags have already been read.

@@ +600,5 @@
> +    { 0x49534850, NS_LITERAL_CSTRING("sharpness") },         // ISHP
> +    { 0x49535243, NS_LITERAL_CSTRING("source") },            // ISRC
> +    { 0x49535246, NS_LITERAL_CSTRING("source_form") },       // ISRF
> +    { 0x49544348, NS_LITERAL_CSTRING("technician") },        // ITCH
> +  };

Do we want to read and expose all of these? Some of them seem nonsensical for audio, e.g. ICRP, IDIM, IDPI, ILGT, IPLT.

@@ +605,5 @@
> +
> +  // Put a null at the end of chunk to ensure the nsCString does not read
> +  // beyond chunk.
> +  chunk[aChunkSize] = 0;
> +  const char * const end = chunk.get() + aChunkSize + 1;

What if aChunkSize is large enough that the pointer wraps?

Any time you're reading data from a file, you need to validate it *very* carefully.

@@ +617,5 @@
> +    // subchunk shall not larger than parent chunk.
> +    if (length > aChunkSize) {
> +      break;
> +    }
> +    nsCString val(p);

Use the nsCString constructor that takes a pointer and a length, and pass in the length from the chunk size.

@@ +621,5 @@
> +    nsCString val(p);
> +    // Chunks in List::INFO are always word (two byte) aligned. So round up if
> +    // necessary.
> +    p += length;
> +    p += length % 2;

Make the second line:

p += p % 2;

So that it matches the other rounding up code.

@@ +627,5 @@
> +    if (!IsUTF8(val)) {
> +      continue;
> +    }
> +
> +    for (size_t i = 0; i < sizeof(ID_TO_NAME) / sizeof(ID_TO_NAME[0]); ++i) {

Use mozilla::ArrayLength.
Comment 40 ChangZhuo Chen 2012-10-24 06:36:16 PDT
Hi,

Thank for the review. I will fix the problems and update the patch. However, I
still have some questions about the code, please help to check them, thanks.

(In reply to Matthew Gregan [:kinetik] from comment #39)
> 
> IBM/MS RIFF/MCI Specification 1.0 states:
> 
> RIFF
> WAVE
>   FMT
>   DATA
> LIST
>   INFO
> 
> or
> 
> RIFF
> LIST
>   INFO
> WAVE
>   FMT
>   DATA

For the wave format, could you double confirm it? It looks like the INFO list
chunk is in the same level as data and fmt chunk. The following example is
from IBM/MS RIFF/MCI Specification 1.0, around p71:

Example of a PCM WAVE file with 44.1 kHz sampling rate, mono, 20 bits per
sample:
RIFF(   'WAVE'          INFO(INAM("O Canada"Z))
                                        fmt(1, 1, 44100, 132300, 3, 20)
                                        data( <wave-data> ) )

> @@ +161,5 @@
> > +        break;
> > +
> > +      case LIST_CHUNK_MAGIC:
> > +        // LIST chunk is optional, so it is okay if we cannot read it.
> > +        LoadListChunk(chunkSize, &tag);
> 
> This will leak tag when the FRMT or DATA branch return an error.

For the leak, is there any thing like nsAutoArrayPtr() for the raw pointer? or
I need to check every possible branch?

> 
> @@ +173,5 @@
> > +
> > +    // RIFF chunks are two-byte aligned, so round up if necessary.
> > +    chunkSize += chunkSize % 2;
> > +
> > +    mDecoder->GetResource()->Seek(nsISeekableStream::NS_SEEK_SET, offset + CHUNK_HEADER_SIZE + chunkSize);
> 
> You need to test for an error and deal with it.  It's not always possible to
> seek in streams, so that case needs to be handled.
> 
> The old code had the nice attribute that it parsed the stream linearly,
> which these changes breaks by expecting to seek between chunks.  I'm not
> *that* happy about that, because it could make loading some files
> significantly slower... and as I mentioned above, we still need to handle
> the case where the stream isn't seekable.

I will following the original design to parse the stream linearly.

> 
> Can we delay loading this metadata until later in some cases?  Or perhaps
> for a first-pass at this, we could only load the metadata here it it appears
> before the WAVE chunk... but that's going to exclude an awful lot of files,
> I suspect.

I think we can assume that INFO list chunk always appers before data chunk
because it is not stream friendly to put INFO list chunk after data chunk. I
will try to download some wave files from Internet to see if there are lots of
wave having INFO list chunk after data chunk.

> 
> @@ +556,5 @@
> > +  // List chunks are always word (two byte) aligned.
> > +  NS_ABORT_IF_FALSE(mDecoder->GetResource()->Tell() % 2 == 0,
> > +                    "LoadListChunk called with unaligned resource");
> > +
> > +  nsAutoArrayPtr<char> chunk(new char[aChunkSize + 1]);
> 
> aChunkSize comes directly from the file, so this could be a huge value,
> including UINT32_MAX, which will overflow.

For this part, I will set an upper bound for the chunk size and field size in
case that we do not use too many memory.

> 
> @@ +570,5 @@
> > +
> > +  // If there are multiple LIST::INFO in wave, we only parse the first one, the
> > +  // others are ignored.
> > +  if (*aTags) {
> > +    return false;
> 
> I'd rather this check was removed and the caller avoided calling this
> function if the tags have already been read.

Ok. I will fix it.

> 
> @@ +600,5 @@
> > +    { 0x49534850, NS_LITERAL_CSTRING("sharpness") },         // ISHP
> > +    { 0x49535243, NS_LITERAL_CSTRING("source") },            // ISRC
> > +    { 0x49535246, NS_LITERAL_CSTRING("source_form") },       // ISRF
> > +    { 0x49544348, NS_LITERAL_CSTRING("technician") },        // ITCH
> > +  };
> 
> Do we want to read and expose all of these? Some of them seem nonsensical
> for audio, e.g. ICRP, IDIM, IDPI, ILGT, IPLT.

I just copy every tag defined in specification. For this part, maybe I just
use four characters as tag.

> 
> @@ +605,5 @@
> > +
> > +  // Put a null at the end of chunk to ensure the nsCString does not read
> > +  // beyond chunk.
> > +  chunk[aChunkSize] = 0;
> > +  const char * const end = chunk.get() + aChunkSize + 1;
> 
> What if aChunkSize is large enough that the pointer wraps?
> 
> Any time you're reading data from a file, you need to validate it *very*
> carefully.

I will set a upper bound for the aChunkSize.

> 
> @@ +617,5 @@
> > +    // subchunk shall not larger than parent chunk.
> > +    if (length > aChunkSize) {
> > +      break;
> > +    }
> > +    nsCString val(p);
> 
> Use the nsCString constructor that takes a pointer and a length, and pass in
> the length from the chunk size.

You mean using nsCString val(p, length)?

> 
> @@ +621,5 @@
> > +    nsCString val(p);
> > +    // Chunks in List::INFO are always word (two byte) aligned. So round up if
> > +    // necessary.
> > +    p += length;
> > +    p += length % 2;
> 
> Make the second line:
> 
> p += p % 2;
> 
> So that it matches the other rounding up code.

Ok. I will fix it.

> 
> @@ +627,5 @@
> > +    if (!IsUTF8(val)) {
> > +      continue;
> > +    }
> > +
> > +    for (size_t i = 0; i < sizeof(ID_TO_NAME) / sizeof(ID_TO_NAME[0]); ++i) {
> 
> Use mozilla::ArrayLength.

Ok. I will fix it.

Regards,
ChangZhuo
Comment 41 Matthew Gregan [:kinetik] 2012-10-24 15:54:16 PDT
(In reply to ChangZhuo Chen from comment #40)
> For the wave format, could you double confirm it? It looks like the INFO list
> chunk is in the same level as data and fmt chunk. The following example is
> from IBM/MS RIFF/MCI Specification 1.0, around p71:

I think that example in the spec is wrong (or it really means a LIST INFO chunk and it's written in a confusing way), so let's ignore it for now.  Of course, since it's in the spec, it may mean that files exist with that layout.  If so, we'll deal with it when we come to it.

Looking at gstreamer's RIFF and WAVE parsers (gst-plugins-base/gst-libs/gst/riff and gst-plugins-good/gst/wavparse), it only expects INFO chunks inside LIST chunks, so I think we're safe to stick to what I said.

Also in the gst code, it also looks like the tag names may appear in some files in lower case (see gst_riff_parse_info where it upcases the tag ID read out of the RIFF).  I don't know how common that is, but we might want to handle that.

> For the leak, is there any thing like nsAutoArrayPtr() for the raw pointer?
> or
> I need to check every possible branch?

Yeah, you can use nsAutoRef.  Sorry, I had forgotten about that, otherwise I would've pointed you to it when I first commented.

> I think we can assume that INFO list chunk always appers before data chunk
> because it is not stream friendly to put INFO list chunk after data chunk. I
> will try to download some wave files from Internet to see if there are lots
> of
> wave having INFO list chunk after data chunk.

Wikipedia made it sound fairly common for them to occur at the end of the file: http://en.wikipedia.org/wiki/Resource_Interchange_File_Format#INFO_chunk_placement_problems

...and it makes sense, given that metadata is often added later and it's painful to rewrite entire files to add a small bit of data.

You could test whether the stream is seekable, and if not, only read the INFO if it's at the start of the file.  That's similar to what the gstreamer code mentioned above does.

> I just copy every tag defined in specification. For this part, maybe I just
> use four characters as tag.

Ralph might have an opinion.  My preference is to only include things that are obviously useful for audio, at least for now.  We can always add more later, but it's difficult to remove things once they're exposed to web content.

> You mean using nsCString val(p, length)?

That's it.

Thanks!
Comment 42 Matthew Gregan [:kinetik] 2012-10-24 15:57:35 PDT
(In reply to Matthew Gregan [:kinetik] from comment #41)
> Yeah, you can use nsAutoRef.  Sorry, I had forgotten about that, otherwise I
> would've pointed you to it when I first commented.

Actually, plain old nsAutoPtr will also work and it's slightly simpler to use.
Comment 43 ChangZhuo Chen 2012-10-29 05:25:28 PDT
(In reply to Matthew Gregan [:kinetik] from comment #41)
> Looking at gstreamer's RIFF and WAVE parsers
> (gst-plugins-base/gst-libs/gst/riff and gst-plugins-good/gst/wavparse), it
> only expects INFO chunks inside LIST chunks, so I think we're safe to stick
> to what I said.

In gst_wavparse_stream_headers in gstreamer, the GST_RIFF_TAG_data and
GST_RIFF_TAG_LIST are in the same switch-case statement, so FMT, LIST, and DATA
are all in the same level, like the following right?

FMT
LIST
  INFO
DATA

> Also in the gst code, it also looks like the tag names may appear in some
> files in lower case (see gst_riff_parse_info where it upcases the tag ID
> read out of the RIFF).  I don't know how common that is, but we might want
> to handle that.

For this case, I think I can use the same way to handle it as gstreamer does.

> You could test whether the stream is seekable, and if not, only read the
> INFO if it's at the start of the file.  That's similar to what the gstreamer
> code mentioned above does.

For the seekable test, is calling Seek and check for return is enough?

> > You mean using nsCString val(p, length)?

The length here contains the NULL in the string, shall I strip the NULL by
using SetLength? or is there any other API I can use?

Regards,
ChangZhuo
Comment 44 ChangZhuo Chen 2012-11-04 03:35:43 PST
Created attachment 678103 [details] [diff] [review]
[patch] read wave metadata

The patch fix the following items:

- Only allow the following wave layout:

  FMT
  LIST
    INFO
  DATA

  LIST
    INFO
  FMT
  DATA

- Avoid using Seek() for nonseekable resource
- Use nsAutoPtr to prevent tag leak
- Limit the LIST::INFO to 64k
- Read only artist, comments, genre, name in LIST::INFO
- Support lower case ID in LIST::INFO
- Use mozilla::ArrayLength for array length

Please help to review it, thanks.
Comment 45 Matthew Gregan [:kinetik] 2012-11-18 18:04:30 PST
Comment on attachment 678103 [details] [diff] [review]
[patch] read wave metadata

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

Sorry for the delay in reviewing this. :-(  Just a few minor comments, r+ with those fixed.

There have been a number of refactoring changes landed in the media code recently, so you will need to rebase your patch before landing it.  The changes should all be straightforward renames, e.g. nsWaveDecoder has been renamed to WaveDecoder.

::: content/media/wave/nsWaveReader.cpp
@@ +135,5 @@
>    if (!loaded) {
>      return NS_ERROR_FAILURE;
>    }
>  
> +  nsAutoPtr<nsHTMLMediaElement::MetadataTags> tag;

Call this "tags".

@@ +541,5 @@
> +    { 0x49474e52, NS_LITERAL_CSTRING("genre") },    // IGNR
> +    { 0x494e414d, NS_LITERAL_CSTRING("name") },     // INAM
> +  };
> +
> +  const char * const end = chunk.get() + aChunkSize;

No space between char and *, please.

@@ +548,5 @@
> +  aTags->Init();
> +
> +  while (p + 8 < end) {
> +    uint32_t id = ReadUint32BE(&p);
> +    /* make uppercase. from gstreamer */

Use // for comments, and make the comment slightly more explicit, something like:

  // Uppercase tag id, inspired by GStreamer's Wave parser.

@@ +553,5 @@
> +    id &= 0xDFDFDFDF;
> +
> +    uint32_t length = ReadUint32LE(&p);
> +
> +    // subchunk shall not exceed parent chunk.

Use a capital at start of comment.

@@ +560,5 @@
> +    }
> +
> +    nsCString val(p, length);
> +    if (val[length - 1] == '\0')
> +      val.SetLength(length - 1);

Use braces here please.

@@ +635,5 @@
> +
> +    // Move forward to next chunk
> +    int64_t forward = chunkStart + chunkSize - GetPosition();
> +
> +    if (forward < 0) {

chunkSize is unsigned and the results of GetPosition should be positive, so this can't be negative.  Signed overflow is undefined, so using a less-than-zero test is unsafe.  Please use CheckedInt64 instead.
Comment 46 ChangZhuo Chen 2012-11-25 07:04:13 PST
Created attachment 684955 [details] [diff] [review]
[patch] read wave metadata

Refactoring as requested. Please help to see if there is any problem, thanks.

https://tbpl.mozilla.org/?tree=Try&rev=cc2cb12bed9d
Comment 47 ChangZhuo Chen 2012-11-25 07:06:20 PST
Created attachment 684957 [details] [diff] [review]
[patch] read wave metadata
Comment 48 Matthew Gregan [:kinetik] 2012-11-28 01:14:48 PST
Thank you!
Comment 50 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-11-28 20:21:40 PST
(In reply to Ralph Giles (:rillian) from comment #12)
> BTW, if you go with the fourcc codes, you could just return everything in
> the LIST::INFO without having to guess which would be important. That might
> be better.

I agree, I think this approach would be better. Simpler code for us, and gives the Web developer access even to metadata we don't recognize.

Note You need to log in before you can comment on or make changes to this bug.