Add downmix capabilities to AudioConverter

RESOLVED FIXED in Firefox 48

Status

()

Core
Audio/Video: Playback
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jya, Assigned: jya)

Tracking

Trunk
mozilla48
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Assignee)

Description

2 years ago
This is to track the task of adding dowmixing capabilities to the downmixer.
(Assignee)

Updated

2 years ago
Blocks: 1247138
(Assignee)

Comment 1

2 years ago
Created attachment 8739327 [details]
MozReview Request: Bug 1262746: P1. Add downmixing capabilities to AudioConverter. r?rillian

This is using the same downmixer algorithm as found in the ogg reader.

Review commit: https://reviewboard.mozilla.org/r/45195/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45195/
Attachment #8739327 - Flags: review?(giles)
Attachment #8739328 - Flags: review?(jwwang)
Attachment #8739329 - Flags: review?(giles)
(Assignee)

Comment 2

2 years ago
Created attachment 8739328 [details]
MozReview Request: Bug 1262746: P2. Use AudioConvert in AudioStream to downmix samples. r?jwwang

Review commit: https://reviewboard.mozilla.org/r/45197/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45197/
(Assignee)

Comment 3

2 years ago
Created attachment 8739329 [details]
MozReview Request: Bug 1262746: P3. Remove DownmixAudioToStereo method. r?rillian

Functionality is now provided through AudioConverter class.

Review commit: https://reviewboard.mozilla.org/r/45199/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45199/
Comment on attachment 8739328 [details]
MozReview Request: Bug 1262746: P2. Use AudioConvert in AudioStream to downmix samples. r?jwwang

https://reviewboard.mozilla.org/r/45197/#review41707

::: dom/media/AudioStream.cpp:358
(Diff revision 1)
>    params.format = ToCubebFormat<AUDIO_OUTPUT_FORMAT>::value;
>    mAudioClock.Init();
>  
> +  mInConfig = MakeUnique<AudioConfig>(mChannels, mInRate);
> +  mOutConfig = MakeUnique<AudioConfig>(mOutChannels, mOutRate);
> +  mAudioConverter = MakeUnique<AudioConverter>(*mInConfig.get(), *mOutConfig.get());

You can say |MakeUnique<>(*mInConfig, *mOutConfig)|. UniquePtr has operation*.
Attachment #8739328 - Flags: review?(jwwang) → review+
Attachment #8739327 - Flags: review?(giles) → review+
Comment on attachment 8739327 [details]
MozReview Request: Bug 1262746: P1. Add downmixing capabilities to AudioConverter. r?rillian

https://reviewboard.mozilla.org/r/45195/#review42085

Sorry it took me so long to look at this. I didn't check the coefficients again. I assume they're cut-and-pasted from the updates in the earlier patches.
Attachment #8739329 - Flags: review?(giles) → review+
Comment on attachment 8739329 [details]
MozReview Request: Bug 1262746: P3. Remove DownmixAudioToStereo method. r?rillian

https://reviewboard.mozilla.org/r/45199/#review42087

...other times I just forget to hit the three-click publish button sequence.

If you export the clip function, you can remove MOZ_CLIP_TO_15() from VideoUtils.h and replace the other calls (OggReader, OpusDecoder) with AudioConverter::clipTo15().
(Assignee)

Comment 7

2 years ago
Comment on attachment 8739327 [details]
MozReview Request: Bug 1262746: P1. Add downmixing capabilities to AudioConverter. r?rillian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45195/diff/1-2/
(Assignee)

Comment 8

2 years ago
Comment on attachment 8739328 [details]
MozReview Request: Bug 1262746: P2. Use AudioConvert in AudioStream to downmix samples. r?jwwang

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45197/diff/1-2/
(Assignee)

Comment 9

2 years ago
Comment on attachment 8739329 [details]
MozReview Request: Bug 1262746: P3. Remove DownmixAudioToStereo method. r?rillian

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

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b77cdb6c0b6c
https://hg.mozilla.org/mozilla-central/rev/5b3a0fdc1e61
https://hg.mozilla.org/mozilla-central/rev/3955e76756ce
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.