1349658 - Support float32 WAV files in WebAudio decodeAudioData

Status: RESOLVED FIXED

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

Description

[toy.raymond]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20170303013005

Steps to reproduce:

Navigate to https://chinmay.audio/decodethis/ and find the entry "M1F1-float32-AFsp.wav". Press the play button. (Or just press the "Test all" button, and find that entry's result.)


Actual results:

 Firefox is unable to decode this 32-bit float WAV file.


Expected results:

It would be nice if Firefox could decode these files since WebAudio works with 32-bit floats.  This is useful for testing WebAudio itself.

Comment 1

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to toy.raymond from comment #0)
> User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:52.0) Gecko/20100101
> Firefox/52.0
> Build ID: 20170303013005
> 
> Steps to reproduce:
> 
> Navigate to https://chinmay.audio/decodethis/ and find the entry
> "M1F1-float32-AFsp.wav". Press the play button. (Or just press the "Test
> all" button, and find that entry's result.)
> 
> 
> Actual results:
> 
>  Firefox is unable to decode this 32-bit float WAV file.
> 
> 
> Expected results:
> 
> It would be nice if Firefox could decode these files since WebAudio works
> with 32-bit floats.  This is useful for testing WebAudio itself.

This is probably a relatively easy first bug if you're feeling motivated. The code is here:

http://searchfox.org/mozilla-central/source/dom/media/wave/WaveDemuxer.cpp

Comment 2

[Luna Phipps-Costin]

Attachment: Bug 1349658 - Use mozilla::BitwiseCast rather than reinterpret_cast

Review commit: https://reviewboard.mozilla.org/r/173612/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/173612/

Comment 3

[Luna Phipps-Costin]

I'm not quite savvy enough to be 100% certain who to call out for review (I put the person who changed most of the code on the file, according to hg, but he has not been active in over a year), so I'm ni'ing Anthony Jones, who is triage owner (? idk what that means) and appeared to do some initial research.

Comment 4

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Comment on attachment 8902156 [details]
Bug 1349658 - Use mozilla::BitwiseCast rather than reinterpret_cast

Thanks the the ni?. JW is the right person to review this code.

Comment 5

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8902156 [details]
Bug 1349658 - Use mozilla::BitwiseCast rather than reinterpret_cast

https://reviewboard.mozilla.org/r/173612/#review179834

::: dom/media/platforms/agnostic/WAVDecoder.cpp:95
(Diff revision 1)
>    }
>    for (int i = 0; i < frames; ++i) {
>      for (unsigned int j = 0; j < mInfo.mChannels; ++j) {
> +      if (mInfo.mProfile == 3) {                              //IEEE float Data
> +        uint32_t v = aReader.ReadU32();
> +        float decoded = reinterpret_cast<float&>(v);

Not sure if this will break strict-aliasing.

::: dom/media/platforms/agnostic/WAVDecoder.cpp:98
(Diff revision 1)
> +      if (mInfo.mProfile == 3) {                              //IEEE float Data
> +        uint32_t v = aReader.ReadU32();
> +        float decoded = reinterpret_cast<float&>(v);
> +        buffer[i * mInfo.mChannels + j] =
> +            FloatToAudioSample<AudioDataValue>(decoded);
> +      }

if (mInfo.mProfile == 3) {
} else if (mInfo.mProfile == 6) {

Comment 6

[JW Wang [:jwwang] [:jw_wang]]

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #5)
> Comment on attachment 8902156 [details]
> Bug 1349658 - Use WAV float audio encoding when appropriate
> 
> https://reviewboard.mozilla.org/r/173612/#review179834
> 
> ::: dom/media/platforms/agnostic/WAVDecoder.cpp:95
> (Diff revision 1)
> >    }
> >    for (int i = 0; i < frames; ++i) {
> >      for (unsigned int j = 0; j < mInfo.mChannels; ++j) {
> > +      if (mInfo.mProfile == 3) {                              //IEEE float Data
> > +        uint32_t v = aReader.ReadU32();
> > +        float decoded = reinterpret_cast<float&>(v);
> 
> Not sure if this will break strict-aliasing.

Hi Nathan,
Is this a correct way to reinterpret a 4-byte integer as a float? Or should we use a union to do the conversion?

Comment 7

[Luna Phipps-Costin]

We could also read it as a float, but the file already uses mp4_demuxer::ByteReader which doesn't support it, so I figured we'd just read it as an int and convert it.

Comment 8

[Nathan Froyd [:froydnj]]

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #6)
> (In reply to JW Wang [:jwwang] [:jw_wang] from comment #5)
> > https://reviewboard.mozilla.org/r/173612/#review179834
> > 
> > ::: dom/media/platforms/agnostic/WAVDecoder.cpp:95
> > (Diff revision 1)
> > >    }
> > >    for (int i = 0; i < frames; ++i) {
> > >      for (unsigned int j = 0; j < mInfo.mChannels; ++j) {
> > > +      if (mInfo.mProfile == 3) {                              //IEEE float Data
> > > +        uint32_t v = aReader.ReadU32();
> > > +        float decoded = reinterpret_cast<float&>(v);
> > 
> > Not sure if this will break strict-aliasing.
> 
> Hi Nathan,
> Is this a correct way to reinterpret a 4-byte integer as a float? Or should
> we use a union to do the conversion?

Yes, this would be undefined behavior.  Please don't do that.

We have mozilla::BitwiseCast to do what you want here:

https://dxr.mozilla.org/mozilla-central/source/mfbt/Casting.h#19-62

  float decoded = BitwiseCast<float>(v);

Please use that instead.

Comment 9

[Luna Phipps-Costin]

Attachment: Bug 1349658 - Use `else if` to not flow into final `else`

Previously a typo had no `else`, making the control flow into the default else
block. Also fix formatting to standard.

Review commit: https://reviewboard.mozilla.org/r/174826/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/174826/

Comment 10

[Luna Phipps-Costin]

Comment on attachment 8902156 [details]
Bug 1349658 - Use mozilla::BitwiseCast rather than reinterpret_cast

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/173612/diff/1-2/

Comment 11

[JW Wang [:jwwang] [:jw_wang]]

Please merge 2 patches.

Comment 12

[Luna Phipps-Costin]

Attachment: `hg export` of amended commits

I'm new to both Mercurial and MozReview, and I think I reeally messed up the MozReview. I couldn't figure it out, so I just did an `hg export` of all 3 commits and attached it. I hope that's ok.

Thank you

Comment 13

[JW Wang [:jwwang] [:jw_wang]]

https://docs.rhodecode.com/RhodeCode-Enterprise/tutorials/squash-commits.html
Use 'hg histedit' to fold multiple commits into one and then push the commit to the review server again (hg push -c REV review).

Comment 14

[Luna Phipps-Costin]

Attachment: Bug 1349658 - Use WAV float audio encoding when appropriate

This is a merge of the original commit and the following additional commits in
response to review:

Bug 1349658 - Use mozilla::BitwiseCast rather than reinterpret_cast

reinterpret_cast would be undefined behavior.

Bug 1349658 - Use `else if` to not flow into final `else` r?jwwang

Previously a typo had no `else`, making the control flow into the default else
block. Also fix formatting to standard.

Review commit: https://reviewboard.mozilla.org/r/174860/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/174860/

Comment 15

[Luna Phipps-Costin]

(Sorry, I forgot to change the old reviewer when I folded the old commits)

I hope this works, thank you for the help.

In the future (and I'm actually having this problem in another bug as well), how can I push for review a commit on top of the old one, like in response to review, rather than pushing a commit as an amendment of the old one? (As it looked like happened here.) Sorry if too off-topic.

Comment 16

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8903079 [details]
Bug 1349658 - Use WAV float audio encoding when appropriate

https://reviewboard.mozilla.org/r/174860/#review179928

::: commit-message-5b30f:1
(Diff revision 1)
> +Bug 1349658 - Use WAV float audio encoding when appropriate r?lchristie

Fix the commit message by removing unwanted lines and describing what this patch actually does.

Comment 17

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Christopher Phonis-Phine from comment #15)
> (Sorry, I forgot to change the old reviewer when I folded the old commits)
> 
> I hope this works, thank you for the help.
> 
> In the future (and I'm actually having this problem in another bug as well),
> how can I push for review a commit on top of the old one, like in response
> to review, rather than pushing a commit as an amendment of the old one? (As
> it looked like happened here.) Sorry if too off-topic.

Say you have r1 in your local repo and you `hg push -c r1 review` for review. Then you have some review comments to address. You can either:

1. amend r1 so you have r1' in your local repo. Do `hg push -c r1' review` for review again. You will still see one patch on the review server. Or

2. add a new commit r2 to address the issues. So you have r1 and r2 in your local repo. Do `hg push -r r1::r2 review` for review again. You will see 2 patches on the review server.

Comment 18

[Luna Phipps-Costin]

Comment on attachment 8903079 [details]
Bug 1349658 - Use WAV float audio encoding when appropriate

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174860/diff/1-2/

Comment 19

[Luna Phipps-Costin]

Thank you for the help! The :: notation is useful, that makes sense now. I amended the commit message.

Comment 20

[Luna Phipps-Costin]

Hi, sorry for the long hiatus. Any chance I could get a review on the request I pushed before? It doesn't look like there are any new merge conflicts since that time.

Comment 21

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

Forward ni flag because JW had left Mozilla.

Comment 22

[Jean-Yves Avenard [:jya]]

Comment on attachment 8903079 [details]
Bug 1349658 - Use WAV float audio encoding when appropriate

https://reviewboard.mozilla.org/r/174860/#review237494

::: dom/media/platforms/agnostic/WAVDecoder.cpp:159
(Diff revision 2)
>  {
>    // Some WebAudio uses "audio/x-wav",
>    // WAVdemuxer uses "audio/wave; codecs=aNum".
>    return aMimeType.EqualsLiteral("audio/x-wav") ||
>           aMimeType.EqualsLiteral("audio/wave; codecs=1") ||
> +         aMimeType.EqualsLiteral("audio/wave; codecs=3") ||

You'll need to also modify in AndroidDecoderModule.cpp:

AndroidDecoderModule::SupportsMimeType

thank you

Comment 23

[Luna Phipps-Costin]

Attachment: Bug 1349658 - Update to use Mozilla::Result

Comment 24

[Luna Phipps-Costin]

Attachment: Bug 1349658 - Use WAV float audio encoding when appropriate r?jya

Comment 25

[Luna Phipps-Costin]

Okay, so ReviewBoard has been decommissioned since I last worked on this patch. Added the android line and pushed it to Phabricator. Let me know if I did it wrong, I'm not sure if having two separate Phabricators is OK, not quite sure what the deal with that is.

Comment 26

[Martin McNickle]

Attachment: Bug 1349658 - Add support for IEEE Float encoded wav files. r=jya

Comment 27

[Martin McNickle]

I cleaned up the previous patch, and added a 32bit float wav file to the play test.

There's a question mark over whether the approach I took to read the float values from the stream is appropriate.

Comment 28

[Martin McNickle]

Attachment: Bug 1349658 - Add support for IEEE Float encoded wav files. r=jya

Comment 29

[Martin McNickle]

Would it be possible to land this for me jya? Or give me pointers on how to go about it?

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/22877b474839
Add support for IEEE Float encoded wav files. r=jya

Comment 31

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/22877b474839

Comment 32

[Jean-Yves Avenard [:jya]]

(In reply to Martin McNickle from comment #29)

Would it be possible to land this for me jya? Or give me pointers on how to go about it?

In the future, add the keyword checkin-needed to the keywords field