Closed Bug 1349658 Opened 4 years ago Closed 1 year ago

Support float32 WAV files in WebAudio decodeAudioData

Categories

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

52 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla74
Tracking Status
firefox74 --- fixed

People

(Reporter: toy.raymond, Assigned: martin)

Details

(Keywords: good-first-bug)

Attachments

(5 files, 3 obsolete files)

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.
Component: Untriaged → Web Audio
Product: Firefox → Core
Component: Web Audio → Audio/Video: Playback
(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
Priority: -- → P3
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.
Flags: needinfo?(ajones)
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.
Flags: needinfo?(ajones)
Attachment #8902156 - Flags: review?(louis) → review?(jwwang)
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) {
Attachment #8902156 - Flags: review?(jwwang)
(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?
Flags: needinfo?(nfroyd)
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.
(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.
Flags: needinfo?(nfroyd)
Please merge 2 patches.
Attachment #8902156 - Attachment is obsolete: true
Attachment #8902156 - Flags: review?(jwwang)
Attachment #8903051 - Attachment is obsolete: true
Attachment #8903051 - Flags: review?(jwwang)
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
Flags: needinfo?(jwwang)
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).
Flags: needinfo?(jwwang)
Attachment #8903079 - Flags: review?(louis) → review?(jwwang)
(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 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.
Attachment #8903079 - Flags: review?(jwwang) → review+
(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.
Thank you for the help! The :: notation is useful, that makes sense now. I amended the commit message.
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.
Flags: needinfo?(suro001)
Forward ni flag because JW had left Mozilla.
Flags: needinfo?(jyavenard)
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
Attachment #8903079 - Flags: review+
Flags: needinfo?(jyavenard)

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.

Flags: needinfo?(suro001)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → martin
Status: NEW → ASSIGNED

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.

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

Flags: needinfo?(jyavenard)
Attachment #9119778 - Attachment is obsolete: true
Pushed by malexandru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/22877b474839
Add support for IEEE Float encoded wav files. r=jya
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla74
Flags: needinfo?(jyavenard)

(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

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