Implement WaveShaperNode.oversample

RESOLVED FIXED in Firefox 26

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Ehsan, Assigned: jwwang)

Tracking

(Blocks 1 bug, {dev-doc-complete})

Trunk
mozilla26
x86
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox25 affected, firefox26 fixed)

Details

Attachments

(2 attachments, 6 obsolete attachments)

Comment hidden (empty)
Mass moving Web Audio bugs to the Web Audio component.  Filter on duckityduck.
Component: Video/Audio → Web Audio
(Assignee)

Comment 2

6 years ago
The idea is to use libspeex for up/down sampling per chunk. However, speex_resampler_process_float() gives weird outputs in Resampler::UpSample/DownSample. Is there anything missing to use libspeex correctly?
Flags: needinfo?(jmvalin)
Pass 1 for channel count to _init and 0 for channel number in _process_float because the channels are already individual streams.
Actually, please ignore comment 3, sorry.
(Assignee)

Comment 5

6 years ago
Posted patch Part1 - add webidl enums (obsolete) — Splinter Review
Attachment #782479 - Flags: review?(ehsan)
(Assignee)

Comment 6

6 years ago
Attachment #782480 - Flags: review?(ehsan)
(Assignee)

Comment 7

6 years ago
Attachment #782481 - Flags: review?(ehsan)
Attachment #782479 - Flags: review?(ehsan) → review+
Comment on attachment 782480 [details] [diff] [review]
Part2 - add up/down samplers from Blink

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

Why should we not use libspeex here?  I don't think this is the right thing to do.
Attachment #782480 - Flags: review?(ehsan) → review-
Attachment #782481 - Flags: review?(ehsan)
(Assignee)

Comment 9

6 years ago
Attachment #780297 - Attachment is obsolete: true
Attachment #782480 - Attachment is obsolete: true
Attachment #782481 - Attachment is obsolete: true
Attachment #782972 - Flags: review?(ehsan)
(Assignee)

Updated

6 years ago
Attachment #782972 - Attachment description: implement based on libspeex → Part2 - implement based on libspeex
Comment on attachment 782972 [details] [diff] [review]
Part2 - implement based on libspeex

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

Looks great, thanks!

::: content/media/webaudio/WaveShaperNode.cpp
@@ +67,5 @@
> +  void Reset(uint32_t aChannels, uint32_t aSampleRate, OverSampleType aType)
> +  {
> +    if (aChannels == mChannels
> +        && aSampleRate == mSampleRate
> +        && aType == mType)

Nit: please put the ands at the end of lines.  Also, please wrap if's in curly brackets.

@@ +90,5 @@
> +    mDownSampler = speex_resampler_init(aChannels,
> +                                        aSampleRate * ValueOf(aType),
> +                                        aSampleRate,
> +                                        SPEEX_RESAMPLER_QUALITY_DEFAULT,
> +                                        nullptr);

Hmm, I think it would be slightly better to lazily allocate the resamplers, since I believe in most cases we will only end up using one of them.

@@ +104,5 @@
> +    MOZ_ASSERT(mBuffer.Length() == outSamples);
> +
> +    speex_resampler_process_float(mUpSampler, aChannel,
> +                                  aInputData, &inSamples,
> +                                  outputData, &outSamples);

After this line, please MOZ_ASSERT(inSamples == WEBAUDIO_BLOCK_SIZE && outSamples == WEBAUDIO_BLOCK_SIZE * aBlocks);.  Same in DownSample() below.

@@ +139,5 @@
> +  OverSampleType mType;
> +  SpeexResamplerState* mUpSampler;
> +  SpeexResamplerState* mDownSampler;
> +  uint32_t mChannels;
> +  uint32_t mSampleRate;

Nit: please use TrackRate as the type of the sample rate here and in the above functions.  (TrackRate is int32_t.)
Attachment #782972 - Flags: review?(ehsan) → review+
Assignee: nobody → jwwang
(Assignee)

Comment 11

6 years ago
Comment on attachment 782972 [details] [diff] [review]
Part2 - implement based on libspeex

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

::: content/media/webaudio/WaveShaperNode.cpp
@@ +90,5 @@
> +    mDownSampler = speex_resampler_init(aChannels,
> +                                        aSampleRate * ValueOf(aType),
> +                                        aSampleRate,
> +                                        SPEEX_RESAMPLER_QUALITY_DEFAULT,
> +                                        nullptr);

I don't get it. Do you mean we will only use either upsampler or downsampler in most cases? I think we will need both of them to complete the process (upsample -> apply curve -> downsample).
(In reply to jwwang from comment #11)
> Comment on attachment 782972 [details] [diff] [review]
> Part2 - implement based on libspeex
> 
> Review of attachment 782972 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/webaudio/WaveShaperNode.cpp
> @@ +90,5 @@
> > +    mDownSampler = speex_resampler_init(aChannels,
> > +                                        aSampleRate * ValueOf(aType),
> > +                                        aSampleRate,
> > +                                        SPEEX_RESAMPLER_QUALITY_DEFAULT,
> > +                                        nullptr);
> 
> I don't get it. Do you mean we will only use either upsampler or downsampler
> in most cases? I think we will need both of them to complete the process
> (upsample -> apply curve -> downsample).

No, sorry, this comment was a mistake of mine, and I meant to remove it before submitting the review!  Please ignore it.
(Assignee)

Comment 13

6 years ago
Attachment #782972 - Attachment is obsolete: true
Attachment #785583 - Flags: review+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
(Assignee)

Comment 16

6 years ago
"OverSampleType mType;" is missing from WaveShaperNode.h after merge and results in build fail. Does the merge fail?
Maybe. Please make sure your patches apply against m-c tip and rebase as necessary.
(Assignee)

Comment 18

6 years ago
Attachment #782479 - Attachment is obsolete: true
Attachment #786152 - Flags: review+
(Assignee)

Comment 19

6 years ago
Attachment #785583 - Attachment is obsolete: true
Attachment #786153 - Flags: review+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/802ef003bb08
https://hg.mozilla.org/mozilla-central/rev/1406e31167e2
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Comment on attachment 786152 [details] [diff] [review]
Part1 - add webidl enums

This is needed for Web Audio in Firefox 25.
Attachment #786152 - Flags: approval-mozilla-aurora?
checkin-needed for Aurora, a=webaudio.
Keywords: checkin-needed
Attachment #786152 - Flags: approval-mozilla-aurora?

Comment 26

6 years ago
Clear the needinfo
Flags: needinfo?(jmvalin)
Depends on: 912474
No longer depends on: 912474
Depends on: 921695
Blocks: 865251
Blocks: 961932
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.