Closed Bug 805254 Opened 7 years ago Closed 7 years ago

Simplify audio format handling

Categories

(Core :: Audio/Video, defect)

18 Branch
x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: roc, Assigned: roc)

Details

Attachments

(13 files)

4.08 KB, patch
kinetik
: review+
Details | Diff | Splinter Review
5.39 KB, patch
kinetik
: review+
Details | Diff | Splinter Review
2.63 KB, patch
kinetik
: review+
Details | Diff | Splinter Review
7.89 KB, patch
kinetik
: review+
Details | Diff | Splinter Review
6.26 KB, patch
kinetik
: review+
Details | Diff | Splinter Review
7.05 KB, patch
kinetik
: review+
Details | Diff | Splinter Review
18.04 KB, patch
kinetik
: review+
Details | Diff | Splinter Review
21.35 KB, patch
kinetik
: review+
Details | Diff | Splinter Review
2.86 KB, patch
kinetik
: review+
Details | Diff | Splinter Review
2.65 KB, patch
kinetik
: review+
Details | Diff | Splinter Review
4.03 KB, patch
kinetik
: review+
Details | Diff | Splinter Review
7.46 KB, patch
kinetik
: review+
Details | Diff | Splinter Review
7.63 KB, patch
kinetik
: review+
Details | Diff | Splinter Review
Now that we hardcode the output audio sample format at compile time (to MOZ_AUDIO_DATA_FORMAT), we can simplify some media code.
We can eliminate support for FORMAT_U8 completely. AFAIK only WAVE files contain these and we already convert them to MOZ_AUDIO_DATA_FORMAT in the WAVE reader.

On desktop we could work with float32 at all times in AudioData and MediaStreams, so we wouldn't even need to support buffers with varying sample formats. Unfortunately on mobile we want to use S16 for media playback but we need to use float32 for Web Audio, so we'll still need to deal with sample formats and conversion in MediaStreams. We could have AudioData use S16 only and MediaStreams use float32 only, but I think it's good to allow MediaStreams to be used for media playback without converting samples, so let's continue to let them be S16 or floats. Also WebRTC wants S16.
I didn't do a particularly good job of equalizing patch sizes ... the other patches are all pretty small and focused, but this one snowballed a bit. Sorry.
Attachment #675050 - Flags: review?(kinetik)
Attachment #675047 - Attachment description: Bug 806254. Part 7: Move SampleFormat to mozilla::AudioSampleFormat → Part 7: Move SampleFormat to mozilla::AudioSampleFormat
Attachment #675041 - Flags: review?(kinetik) → review+
Attachment #675042 - Flags: review?(kinetik) → review+
Attachment #675043 - Flags: review?(kinetik) → review+
Attachment #675044 - Flags: review?(kinetik) → review+
Attachment #675045 - Flags: review?(kinetik) → review+
Attachment #675046 - Flags: review?(kinetik) → review+
Comment on attachment 675047 [details] [diff] [review]
Part 7: Move SampleFormat to mozilla::AudioSampleFormat

The SharedBuffer.h change doesn't seem to belong in this patch.
Attachment #675047 - Flags: review?(kinetik) → review+
Comment on attachment 675050 [details] [diff] [review]
Part 8: Consolidate audio sample processing code

GetSampleSize could be replaced with the trait mechanism, but it's not a big deal either way.
Attachment #675050 - Flags: review?(kinetik) → review+
There's a build failure due to the 16-bit InterleaveAndConvertBuffer not being used on desktop (and warnings-as-errors being set). I'm not sure of the best way to suppress that...
(In reply to Matthew Gregan [:kinetik] from comment #17)
> GetSampleSize could be replaced with the trait mechanism, but it's not a big
> deal either way.

It goes away in part 12 anyway.

(In reply to Matthew Gregan [:kinetik] from comment #16)
> The SharedBuffer.h change doesn't seem to belong in this patch.

It actually is needed because SharedBuffer.h uses nsCOMPtr/nsRefPtr and some #include change means that they're no longer defined in time for SharedBuffer to use them.
Attachment #675051 - Flags: review?(kinetik) → review+
Attachment #675052 - Flags: review?(kinetik) → review+
Attachment #675053 - Flags: review?(kinetik) → review+
Attachment #675054 - Flags: review?(kinetik) → review+
Attachment #675055 - Flags: review?(kinetik) → review+
You need to log in before you can comment on or make changes to this bug.