Experimental: Unmute Audio During Tab Sourced Desktop Share

Experimental: Unmute Audio During Tab Sourced Desktop Share

This CL is used to illustrate my design idea. 
For real implementation, we need to split the stuff into 
several CLs.

BUG=595428

[qiangchen, 2016-03-28 18:39:36]

Description was changed from

==========
Experimental: Unmute Audio During Tab Sourced Desktop Share

BUG=
==========

to

==========
Experimental: Unmute Audio During Tab Sourced Desktop Share

This CL is used to illustrate my design idea. 
For real implementation, we need to split the stuff into 
several CLs.

BUG=
==========

[qiangchen, 2016-03-28 18:39:36]

qiangchen@chromium.org changed reviewers:
+ dalecurtis@chromium.org, miu@chromium.org

[qiangchen, 2016-03-28 22:36:14]

Description was changed from

==========
Experimental: Unmute Audio During Tab Sourced Desktop Share

This CL is used to illustrate my design idea. 
For real implementation, we need to split the stuff into 
several CLs.

BUG=
==========

to

==========
Experimental: Unmute Audio During Tab Sourced Desktop Share

This CL is used to illustrate my design idea. 
For real implementation, we need to split the stuff into 
several CLs.

BUG=595428
==========

[qiangchen, 2016-03-28 22:37:48]

The experimental CL illustating design idea of unmuting source audio during
capturing.

[miu, 2016-03-31 20:11:53]

On 2016/03/28 22:37:48, qiangchenC wrote:
> The experimental CL illustating design idea of unmuting source audio during
> capturing.

Took a look.  This jogged my old memories, and I think I have a simpler solution
to all this.  First, let's revisit a few things to make sure we're on the same
page:

To re-state your requirements: Today, tab capture of audio will halt audio
delivery to the local audio output (speakers).  You'd like to introduce an
option that continues local audio output while capturing.

How audio output works today: The platform provides an AudioOutputStream impl,
which will call AudioOutputController::OnMoreData() at regular intervals to
fetch audio from the renderer.

How tab audio capture works today: WCAIS spawns a VirtualAudioInputStream.  This
is a loopback mechanism.  It'll launch a thread (via FakeAudioWorker) to pull
the audio at regular intervals (just as a normal AudioOutputStream would),
ultimately calling AudioOutputController::OnMoreData() to fetch audio from the
renderer.  However, this scheme only allows audio flow to travel to one
destination, because only one destination may "pull" at a time.

What I think you should do for duplicating audio: Instead of introducing an
AudioShifter to duplicate/stretch the audio between two "pullers" with different
clocks, you should instead provide an alternate "piggyback" implementation for
tab capture so that tab capture gets audio pushed to it instead.  Meaning, when
you start tab audio capture, allow the platform AudioOutputStream impl to
continue to make AOC::OnMoreData() calls to pull the audio as it normally would.
 Then, add some simple code in AOC::OnMoreData() to *push* a copy of each
AudioBus into the loopback pipeline.  This would only happen for tab capture in
this new "duplication mode."

The Start/StopDuplicating() methods you proposed adding to AudioOutputController
could instead take a pointer to an AudioInputStream::AudioInputCallback.  A
better name for these methods might be AddLoopbackSink().  Then, WCAIS (via the
MirroringDestination interface) could provide a CreateLoopbackSink() method that
returns an AIS::AudioInputCallback for receiving the pushed audio data.  In this
scheme, VirtualAudioInput/OutputStream would not be involved.

WDYT?  If this is confusing, we could chat more over VC (I'm in your time zone,
IIRC).

[qiangchen, 2016-03-31 23:34:33]

On 2016/03/31 20:11:53, miu wrote:
> On 2016/03/28 22:37:48, qiangchenC wrote:
> > The experimental CL illustating design idea of unmuting source audio during
> > capturing.
> 
> Took a look.  This jogged my old memories, and I think I have a simpler
solution
> to all this.  First, let's revisit a few things to make sure we're on the same
> page:
> 
> To re-state your requirements: Today, tab capture of audio will halt audio
> delivery to the local audio output (speakers).  You'd like to introduce an
> option that continues local audio output while capturing.
> 
> How audio output works today: The platform provides an AudioOutputStream impl,
> which will call AudioOutputController::OnMoreData() at regular intervals to
> fetch audio from the renderer.
> 
> How tab audio capture works today: WCAIS spawns a VirtualAudioInputStream. 
This
> is a loopback mechanism.  It'll launch a thread (via FakeAudioWorker) to pull
> the audio at regular intervals (just as a normal AudioOutputStream would),
> ultimately calling AudioOutputController::OnMoreData() to fetch audio from the
> renderer.  However, this scheme only allows audio flow to travel to one
> destination, because only one destination may "pull" at a time.
> 
> What I think you should do for duplicating audio: Instead of introducing an
> AudioShifter to duplicate/stretch the audio between two "pullers" with
different
> clocks, you should instead provide an alternate "piggyback" implementation for
> tab capture so that tab capture gets audio pushed to it instead.  Meaning,
when
> you start tab audio capture, allow the platform AudioOutputStream impl to
> continue to make AOC::OnMoreData() calls to pull the audio as it normally
would.
>  Then, add some simple code in AOC::OnMoreData() to *push* a copy of each
> AudioBus into the loopback pipeline.  This would only happen for tab capture
in
> this new "duplication mode."
> 
> The Start/StopDuplicating() methods you proposed adding to
AudioOutputController
> could instead take a pointer to an AudioInputStream::AudioInputCallback.  A
> better name for these methods might be AddLoopbackSink().  Then, WCAIS (via
the
> MirroringDestination interface) could provide a CreateLoopbackSink() method
that
> returns an AIS::AudioInputCallback for receiving the pushed audio data.  In
this
> scheme, VirtualAudioInput/OutputStream would not be involved.
> 
> WDYT?  If this is confusing, we could chat more over VC (I'm in your time
zone,
> IIRC).

Thanks a lot.

I'll investigate the detail tomorrow.

So roughly speaking WCAIS could take data using push model. The
VirtualAudioOutputStream it spawns is kind of a connector to turn WCAIS to use
pull model for data.

If I understand correctly, your suggestion is to push data directly to WCAIS,
rather than using VAOS to turn it to pull model, then using AudioShifter to turn
back to push model.

[miu, 2016-04-01 22:48:53]

On 2016/03/31 23:34:33, qiangchenC wrote:
> So roughly speaking WCAIS could take data using push model. The
> VirtualAudioOutputStream it spawns is kind of a connector to turn WCAIS to use
> pull model for data.

Yes, that's a good way to summarize my proposal.

> If I understand correctly, your suggestion is to push data directly to WCAIS,
> rather than using VAOS to turn it to pull model, then using AudioShifter to
turn
> back to push model.

No need to use AudioShifter at all.  First, keep the "pull" implementation
(using VAOS) in WCAIS for the normal, non-duplicated-audio use case.  There's no
need to remove or modify that.  Then, for the duplication use case, add the
ability to create a WCAIS in a mode where it will have audio pushed to it
(instead of using VAOS).

[qiangchen, 2016-04-04 17:53:01]

On 2016/04/01 22:48:53, miu wrote:
> On 2016/03/31 23:34:33, qiangchenC wrote:
> > So roughly speaking WCAIS could take data using push model. The
> > VirtualAudioOutputStream it spawns is kind of a connector to turn WCAIS to
use
> > pull model for data.
> 
> Yes, that's a good way to summarize my proposal.
> 
> > If I understand correctly, your suggestion is to push data directly to
WCAIS,
> > rather than using VAOS to turn it to pull model, then using AudioShifter to
> turn
> > back to push model.
> 
> No need to use AudioShifter at all.  First, keep the "pull" implementation
> (using VAOS) in WCAIS for the normal, non-duplicated-audio use case.  There's
no
> need to remove or modify that.  Then, for the duplication use case, add the
> ability to create a WCAIS in a mode where it will have audio pushed to it
> (instead of using VAOS).

Thanks.

Update of my investigation:
An AudioConverter will be necessary to resolve the parameter difference between
WebContentsAudioInputStream and AudioOutputController (namely the speaker).

P.S. Previously, VirtualAudioOutputStream is created with same parameter as
AudioOutputController. But its connection to VirtualAudioInputStream
(WCAIS::mixer_stream_) is through an AudioConverter, so the parameter
discrepancy is automatically resolved.

[qiangchen, 2016-04-06 21:37:49]

Patchset #3 (id:40001) has been deleted

[qiangchen, 2016-04-06 21:44:13]

On 2016/04/04 17:53:01, qiangchenC wrote:
> On 2016/04/01 22:48:53, miu wrote:
> > On 2016/03/31 23:34:33, qiangchenC wrote:
> > > So roughly speaking WCAIS could take data using push model. The
> > > VirtualAudioOutputStream it spawns is kind of a connector to turn WCAIS to
> use
> > > pull model for data.
> > 
> > Yes, that's a good way to summarize my proposal.
> > 
> > > If I understand correctly, your suggestion is to push data directly to
> WCAIS,
> > > rather than using VAOS to turn it to pull model, then using AudioShifter
to
> > turn
> > > back to push model.
> > 
> > No need to use AudioShifter at all.  First, keep the "pull" implementation
> > (using VAOS) in WCAIS for the normal, non-duplicated-audio use case. 
There's
> no
> > need to remove or modify that.  Then, for the duplication use case, add the
> > ability to create a WCAIS in a mode where it will have audio pushed to it
> > (instead of using VAOS).
> 
> Thanks.
> 
> Update of my investigation:
> An AudioConverter will be necessary to resolve the parameter difference
between
> WebContentsAudioInputStream and AudioOutputController (namely the speaker).
> 
> P.S. Previously, VirtualAudioOutputStream is created with same parameter as
> AudioOutputController. But its connection to VirtualAudioInputStream
> (WCAIS::mixer_stream_) is through an AudioConverter, so the parameter
> discrepancy is automatically resolved.

Hi, miu@

    Can you take a look at the update? I encountered a crucial problem when
trying to implement push model for duplication, namely the audio parameter
discrepancy.

    I marked "NOT WORK" in the code, and explain the reason. I also updated the
design describing this. Are we going to implement the way using AudioShifter, or
any better way?

    Thanks

Qiang

[miu, 2016-04-12 02:09:46]

On 2016/04/06 21:44:13, qiangchenC wrote:
>     Can you take a look at the update? I encountered a crucial problem when
> trying to implement push model for duplication, namely the audio parameter
> discrepancy.

Seems like the existing interfaces don't support what you need.  Meaning, the
media::AudioInputStream::AudioInputCallback interface only provides a callback
method for the audio data under the assumption that the format never changes.

My suggestion: You'll need to design an interface that provides methods both for
delivering the audio data *and* to report format changes.  Something like the
following:

  namespace media {
    class LoopbackSink {
      virtual void OnFormatChanged(const media::AudioParams&) = 0;
      virtual void OnData(const media::AudioBus&, delay, ...) = 0;
      virtual void Close() = 0;
    };
  }

Then, in AOC, you can replace the Start/StopDuplicating methods with:

  void AddLoopbackSink(media::LoopbackSink* sink);
  void RemoveLoopbackSink(media::LoopbackSink* sink);

AOC would call sink->OnData() to deliver a copy of the audio data, and call
sink->OnFormatChanged() whenever the format changes.  Close() would have similar
semantics to AudioOutputStream::Close().

Then, in the MirroringDestination interface, rename AddInput() to
AddPulledInput(), and then add a second method:

  virtual media::AudioOutputStream* AddPulledInput(
      const media::AudioParameters& params) = 0;
  virtual media::LoopbackSink* AddPushedInput() = 0;

The AddPushedInput() method would return an implementation of the LoopbackSink
interface that hooks into into VirtualAudioInputStream's AudioConverters.

Not sure if all of the above will "fit" perfectly.  Use it as guidance, and try
to adjust as necessary.  The real hard part in all of this is going to be the
thread-safety concerns and the start/stop/tear-down sequence.

[qiangchen, 2016-04-12 16:36:13]

On 2016/04/12 02:09:46, miu wrote:
> On 2016/04/06 21:44:13, qiangchenC wrote:
> >     Can you take a look at the update? I encountered a crucial problem when
> > trying to implement push model for duplication, namely the audio parameter
> > discrepancy.
> 
> Seems like the existing interfaces don't support what you need.  Meaning, the
> media::AudioInputStream::AudioInputCallback interface only provides a callback
> method for the audio data under the assumption that the format never changes.
> 
> My suggestion: You'll need to design an interface that provides methods both
for
> delivering the audio data *and* to report format changes.  Something like the
> following:
> 
>   namespace media {
>     class LoopbackSink {
>       virtual void OnFormatChanged(const media::AudioParams&) = 0;
>       virtual void OnData(const media::AudioBus&, delay, ...) = 0;
>       virtual void Close() = 0;
>     };
>   }
> 
> Then, in AOC, you can replace the Start/StopDuplicating methods with:
> 
>   void AddLoopbackSink(media::LoopbackSink* sink);
>   void RemoveLoopbackSink(media::LoopbackSink* sink);
> 
> AOC would call sink->OnData() to deliver a copy of the audio data, and call
> sink->OnFormatChanged() whenever the format changes.  Close() would have
similar
> semantics to AudioOutputStream::Close().
> 
> Then, in the MirroringDestination interface, rename AddInput() to
> AddPulledInput(), and then add a second method:
> 
>   virtual media::AudioOutputStream* AddPulledInput(
>       const media::AudioParameters& params) = 0;
>   virtual media::LoopbackSink* AddPushedInput() = 0;
> 
> The AddPushedInput() method would return an implementation of the LoopbackSink
> interface that hooks into into VirtualAudioInputStream's AudioConverters.
> 
> Not sure if all of the above will "fit" perfectly.  Use it as guidance, and
try
> to adjust as necessary.  The real hard part in all of this is going to be the
> thread-safety concerns and the start/stop/tear-down sequence.

I think the essential difficulty is how to do the format conversion.
Namely, how can we implement OnFormatChanged?

Now the input format is determined by AOC, but the output format is determined
by WebContentsAudioInputStream, and they may not be the same.
We cannot simply force the audio format in WCAIS to be the same as AOC, because
it is determined by WCAIS' consumer, and the following code path is built based
on that format.
Do we have a correspondence class of AudioConverter, which works for push model?
(P.S. AudioConverter is for pull model)
If not, do you think possible and worth to write one?

[miu, 2016-04-12 23:34:46]

On 2016/04/12 16:36:13, qiangchenC wrote:
> I think the essential difficulty is how to do the format conversion.
> Namely, how can we implement OnFormatChanged?
> 
> Now the input format is determined by AOC, but the output format is determined
> by WebContentsAudioInputStream, and they may not be the same.
> We cannot simply force the audio format in WCAIS to be the same as AOC,
because
> it is determined by WCAIS' consumer, and the following code path is built
based
> on that format.
> Do we have a correspondence class of AudioConverter, which works for push
model?
> (P.S. AudioConverter is for pull model)
> If not, do you think possible and worth to write one?

WCAIS uses media::VirtualAudioInputStream (and VAIOutputStream) to do the format
conversion from the *multiple* AOCs to a single mixed audio data stream provided
by VAIS.  I recommend you read and understand all the code in
virtual_audio_input_stream.cc AND virtual_audio_output_stream.cc to see how the
data flow with format conversion happens (hint: there is a thread that calls
PumpAudio() at regular intervals).

Push model to Pull model: So, now I think I may have been wrong earlier where I
said you wouldn't need AudioShifter.  I was thinking about the case where there
is only a single AudioOutputStream for a tab.  However, there can be more than
one, and VAIS will mix multiple audio streams into one.  Thus, your
implementation of the LoopbackSink interface should:

1. Own an AudioShifter: Calls to the LoopbackSink::OnData() implementation
should push audio into an AudioShifter.  You re-create the AudioShifter each
time LoopbackSink::OnFormatChanged() is called with new params.

2. Own a VirtualAudioOutputStream: You would create one of these each time
LoopbackSink::OnFormatChanged() is called with new params, and call
VAOS::Start().  Then, VAIS will run OnMoreData() callbacks to pull audio out of
the AudioShifter at regular intervals.  The unavoidable mismatch in clock rates
will be resolved by AudioShifter automatically stretching the audio to
compensate.

In fact, I've been talking about LoopbackSink as an interface that needs an impl
class.  Perhaps the interface isn't needed and you can just have a single
concrete class named something like VirtualAudioLoopbackSink.

[qiangchen, 2016-04-13 21:15:00]

> WCAIS uses media::VirtualAudioInputStream (and VAIOutputStream) to do the
format
> conversion from the *multiple* AOCs to a single mixed audio data stream
provided
> by VAIS.  I recommend you read and understand all the code in
> virtual_audio_input_stream.cc AND virtual_audio_output_stream.cc to see how
the
> data flow with format conversion happens (hint: there is a thread that calls
> PumpAudio() at regular intervals).

Yeah, I have read that. The essential format conversion happens in
AudioConverter, which is in the pulling part.

My understanding is that PumpAudio() is the separation point between pulling
part and pushing part.
The code path before PumpAudio() is the pulling part, and after that is the
pushing part.
AudioConverter converts audio format and mix multiple VAOS into one stream.

Probably we will still utilize this part to resolve provider-consumer audio
format difference.

Thanks.

[qiangchen, 2016-04-15 16:35:42]

In the fourth patch, I created LoopbackSink, which is used to replace VAOS in
the duplication code path.

LoopbackSink uses the same way as VAOS to attach to WebContentsAudioInputStream,
but unlike VAOS, LoopbackSink accepts a push-model input.

Is this close to what you mentioned?

P.S. in this experimental CL, I did not create files for new class, but just
squeeze it in virtual_audio_output_stream.

[qiangchen, 2016-04-28 16:53:36]

qiangchen@chromium.org changed reviewers:
- dalecurtis@chromium.org, miu@chromium.org