952326 - Address follow up issues from moving DownmixToStereo functionality into AudioData class

Status: NEW

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

Description

[Matthew Gregan [:kinetik]]

Opening a new bug for bug 911482 comment 24:

+++ This bug was initially created as a clone of Bug #911482 +++

(In reply to Matthew Gregan [:kinetik] from comment #24)
> diff --git a/content/media/MediaDecoderStateMachine.cpp
> b/content/media/MediaDecoderStateMachine.cpp
> @@ -1273,16 +1273,18 @@ uint32_t MediaDecoderStateMachine::PlayF
> +  aChannels = mAudioStream->GetOutChannels();
> [...]
>    mEventManager.QueueWrittenAudioData(audio->mAudioData.get(),
>                                        audio->mFrames * aChannels,
> 
> This can't be right.  mEventManager is initialized with the channel count of
> the media in MediaDecoderStateMachine::DecodeMetadata.  That also means the
> data in the AudioAvailable event doesn't match what's expected according to
> MediaElement.mozChannels.
> 
> Also, HTMLMediaElement::SetPlaybackRate rejects playbackRate changes for
> media with > 2 channels, but that condition will be hit if we're downmixing
> to stereo and could support playbackRate changes.
> 
> I'm not a fan of changing AudioData in place.  There's a reason
> AudioStream::Write is passed a const buffer; it should not be discarded
> lightly.  (See also bug 495040 comment 47, where this came up before.)

Comment 1

[Alex Chronopoulos [:achronop]]

> > diff --git a/content/media/MediaDecoderStateMachine.cpp
> > b/content/media/MediaDecoderStateMachine.cpp
> > @@ -1273,16 +1273,18 @@ uint32_t MediaDecoderStateMachine::PlayF
> > +  aChannels = mAudioStream->GetOutChannels();
> > [...]
> >    mEventManager.QueueWrittenAudioData(audio->mAudioData.get(),
> >                                        audio->mFrames * aChannels,
> > 
> > This can't be right.  mEventManager is initialized with the channel count of
> > the media in MediaDecoderStateMachine::DecodeMetadata.  That also means the
> > data in the AudioAvailable event doesn't match what's expected according to
> > MediaElement.mozChannels.

This is a problem indeed. It happens because the number of channles in metadata are set to the original value. We can provide a quick solution by setting directly the mChannels in metadata to 2 (when original mChannels >2 | <=8), on every implementation of MediaDecoderReader. We used to do that in OggReader (but removed from that patch).

> > Also, HTMLMediaElement::SetPlaybackRate rejects playbackRate changes for
> > media with > 2 channels, but that condition will be hit if we're downmixing
> > to stereo and could support playbackRate changes.

Same above.

> > I'm not a fan of changing AudioData in place.  There's a reason
> > AudioStream::Write is passed a const buffer; it should not be discarded
> > lightly.  (See also bug 495040 comment 47, where this came up before.)

I agree that the buffer is const for some reason. I do not like cast to unconst either. I think this patch took r+ because this is an intermediate step before transfer the downmix functionality in a more appropriate place, closer to the consumer platform. We can move to the final solution and undo this.