Exposes a shlib interface for media/audio path.

Introduces media interface for third-party Cast clients.

Adds interfaces to chromecast/public for output audio streams. These
interfaces are intended to be implemented by third-party clients,
without dependencies on other parts of Chromium. Introduces a number of
classes which translate chromecast interfaces to their media::
counterparts.

BUG=None

[slan, 2015-04-24 01:40:26]

slan@chromium.org changed reviewers:
+ byungchul@chromium.org, cleichner@chromium.org, gunsch@chromium.org,
halliwell@chromium.org

[slan, 2015-04-24 01:40:27]

Initial commit for breaking static dependencies for audio playout.

Please review the two public interfaces introduced:
chromecast/public/cast_audio_stream.h
chromecast/public/cast_audio_stream_provider.h

These are the interfaces that vendors will implement (the entry points to our
shared libraries). I am not as familiar as all of you on what is needed by each
of them, what is common, etc. They currently use the design in media:: If
modifications need to be made to those interfaces, please give feedback, then
review the rest of the patch once the dust has settled. Most of the rest is
framework to pipe the newly exposed interfaces back to internal media:: code.

Coming soon: unittests for CastAudioOutputStream, as well as integration tests
to verify data transport from media:: down to the platform. But I want to shake
out the interfaces first.

[lcwu1, 2015-04-24 01:44:27]

lcwu@chromium.org changed reviewers:
+ erickung@chromium.org, lcwu@chromium.org, servolk@chromium.org

[lcwu1, 2015-04-24 02:18:25]

https://codereview.chromium.org/1105803002/diff/1/chromecast/media/audio/cast...
File chromecast/media/audio/cast_audio_manager.cc (right):

https://codereview.chromium.org/1105803002/diff/1/chromecast/media/audio/cast...
chromecast/media/audio/cast_audio_manager.cc:43:
CastAudioManager::CastAudioManager(
Leave a vertical space above this line.

https://codereview.chromium.org/1105803002/diff/1/chromecast/media/audio/cast...
File chromecast/media/audio/cast_audio_manager.h (right):

https://codereview.chromium.org/1105803002/diff/1/chromecast/media/audio/cast...
chromecast/media/audio/cast_audio_manager.h:10: #include "base/macros.h"
Keep a vertical space between C++ system headers and project's headers.

https://codereview.chromium.org/1105803002/diff/1/chromecast/media/audio/cast...
chromecast/media/audio/cast_audio_manager.h:18: namespace media {
Any reason why you want this to be outside of chromecast namespace?

https://codereview.chromium.org/1105803002/diff/1/chromecast/media/audio/cast...
chromecast/media/audio/cast_audio_manager.h:35: // AudioManager overrides.
Our convention is to use

// AudioManager implementation:

https://codereview.chromium.org/1105803002/diff/1/chromecast/public/cast_audi...
File chromecast/public/cast_audio_stream_provider.h (right):

https://codereview.chromium.org/1105803002/diff/1/chromecast/public/cast_audi...
chromecast/public/cast_audio_stream_provider.h:22: class CHROMECAST_EXPORT
CastAudioStreamProvider {
This name is actually a bit misleading as it seems to suggest this is a class
that 'provides' the audio stream, while it is actually a class that we can query
the output device's capability, parameters, etc. I would probably rename it to
something like 'CastAudioOutputProvider' or 'CastAudioOutputDevice'.

[slan, 2015-04-24 05:34:26]

Thanks for the comments, lcwu.

I cleaned up some formatting and moved everything into the chromecast::
namespace. Please take a look at Patch 2, especially the audio interfaces in
chromecast/public/. These are what the vendors will implement, and they will
drive the rest of the code in this patch.

https://codereview.chromium.org/1105803002/diff/1/chromecast/media/audio/cast...
File chromecast/media/audio/cast_audio_manager.cc (right):

https://codereview.chromium.org/1105803002/diff/1/chromecast/media/audio/cast...
chromecast/media/audio/cast_audio_manager.cc:43:
CastAudioManager::CastAudioManager(
On 2015/04/24 02:18:24, lcwu1 wrote:
> Leave a vertical space above this line.

Done.

https://codereview.chromium.org/1105803002/diff/1/chromecast/media/audio/cast...
File chromecast/media/audio/cast_audio_manager.h (right):

https://codereview.chromium.org/1105803002/diff/1/chromecast/media/audio/cast...
chromecast/media/audio/cast_audio_manager.h:10: #include "base/macros.h"
On 2015/04/24 02:18:24, lcwu1 wrote:
> Keep a vertical space between C++ system headers and project's headers.

Done.

https://codereview.chromium.org/1105803002/diff/1/chromecast/media/audio/cast...
chromecast/media/audio/cast_audio_manager.h:18: namespace media {
On 2015/04/24 02:18:24, lcwu1 wrote:
> Any reason why you want this to be outside of chromecast namespace?

I did this to add readability and avoid name collisions between chromecast
interfaces that mirror media interfaces, and implementations of media interfaces
(i.e. CastAudioOutputStream could be seen as either, without context).

This is probably an abuse of the feature. I think that I can work out a better
naming scheme for the next patch, putting all of this in the chromecast
namespace.

https://codereview.chromium.org/1105803002/diff/1/chromecast/media/audio/cast...
chromecast/media/audio/cast_audio_manager.h:35: // AudioManager overrides.
On 2015/04/24 02:18:24, lcwu1 wrote:
> Our convention is to use
> 
> // AudioManager implementation:

Done.

https://codereview.chromium.org/1105803002/diff/1/chromecast/public/cast_audi...
File chromecast/public/cast_audio_stream_provider.h (right):

https://codereview.chromium.org/1105803002/diff/1/chromecast/public/cast_audi...
chromecast/public/cast_audio_stream_provider.h:22: class CHROMECAST_EXPORT
CastAudioStreamProvider {
On 2015/04/24 02:18:24, lcwu1 wrote:
> This name is actually a bit misleading as it seems to suggest this is a class
> that 'provides' the audio stream, while it is actually a class that we can
query
> the output device's capability, parameters, etc. I would probably rename it to
> something like 'CastAudioOutputProvider' or 'CastAudioOutputDevice'.

I like the second one. Done.

[halliwell, 2015-04-24 15:04:48]

https://codereview.chromium.org/1105803002/diff/20001/chromecast/media/audio/...
File chromecast/media/audio/cast_audio_manager_factory_test.cc (right):

https://codereview.chromium.org/1105803002/diff/20001/chromecast/media/audio/...
chromecast/media/audio/cast_audio_manager_factory_test.cc:5: #include <string>
insert blank line

https://codereview.chromium.org/1105803002/diff/20001/chromecast/media/audio/...
File chromecast/media/audio/cast_audio_manager_unittest.cc (right):

https://codereview.chromium.org/1105803002/diff/20001/chromecast/media/audio/...
chromecast/media/audio/cast_audio_manager_unittest.cc:5: #include <string>
insert blank line

https://codereview.chromium.org/1105803002/diff/20001/chromecast/media/audio/...
File chromecast/media/audio/chromecast_device_audio_output_stream.cc (right):

https://codereview.chromium.org/1105803002/diff/20001/chromecast/media/audio/...
chromecast/media/audio/chromecast_device_audio_output_stream.cc:53: frames *
params_.channels * params_.bits_per_sample / 8;
Other comment on bits/8 applies here (actually looking a few lines below,
AudioBus takes bytes per sample, makes me think we're always 0 mod 8)?

https://codereview.chromium.org/1105803002/diff/20001/chromecast/public/cast_...
File chromecast/public/cast_audio_output_device.h (right):

https://codereview.chromium.org/1105803002/diff/20001/chromecast/public/cast_...
chromecast/public/cast_audio_output_device.h:8: #include <string>
Is <string> used?

https://codereview.chromium.org/1105803002/diff/20001/chromecast/public/cast_...
chromecast/public/cast_audio_output_device.h:22: 
How do implementations get instantiated ... is that a follow-up CL?

https://codereview.chromium.org/1105803002/diff/20001/chromecast/public/cast_...
chromecast/public/cast_audio_output_device.h:37: // does not take ownership of
the returned stream.
So what's the lifetime of these streams ... they live till process exits?

https://codereview.chromium.org/1105803002/diff/20001/chromecast/public/cast_...
File chromecast/public/cast_audio_stream.h (right):

https://codereview.chromium.org/1105803002/diff/20001/chromecast/public/cast_...
chromecast/public/cast_audio_stream.h:15: // media::AudioParmameters.
typo: Parameters

https://codereview.chromium.org/1105803002/diff/20001/chromecast/public/cast_...
chromecast/public/cast_audio_stream.h:43: //   |len| >= |frames| * (num
channels) * (bits per sample) / 8
If we ever had e.g. 20 bit audio, then bits/8 needs rounding up not down.  And
if we explicitly don't support non-multiple-of-8 bit depths, consider enforcing
that by using bytes per sample not bits??

https://codereview.chromium.org/1105803002/diff/20001/chromecast/public/cast_...
chromecast/public/cast_audio_stream.h:86: virtual void GetVolume(double* volume)
= 0;
Any reason why we don't just return double here?

[slan, 2015-04-24 16:19:39]

Thanks for the comments, halliwell@. All nits were addressed. Please see my
questions in comments:
 - Do we need to support non-byte-aligned bit depths?
 - Is the ownership model for audio streams appropriate?

Thanks!

https://codereview.chromium.org/1105803002/diff/20001/chromecast/media/audio/...
File chromecast/media/audio/cast_audio_manager_factory_test.cc (right):

https://codereview.chromium.org/1105803002/diff/20001/chromecast/media/audio/...
chromecast/media/audio/cast_audio_manager_factory_test.cc:5: #include <string>
On 2015/04/24 15:04:47, halliwell wrote:
> insert blank line

Done.

https://codereview.chromium.org/1105803002/diff/20001/chromecast/media/audio/...
File chromecast/media/audio/cast_audio_manager_unittest.cc (right):

https://codereview.chromium.org/1105803002/diff/20001/chromecast/media/audio/...
chromecast/media/audio/cast_audio_manager_unittest.cc:5: #include <string>
On 2015/04/24 15:04:47, halliwell wrote:
> insert blank line

Done.

https://codereview.chromium.org/1105803002/diff/20001/chromecast/media/audio/...
File chromecast/media/audio/chromecast_device_audio_output_stream.cc (right):

https://codereview.chromium.org/1105803002/diff/20001/chromecast/media/audio/...
chromecast/media/audio/chromecast_device_audio_output_stream.cc:53: frames *
params_.channels * params_.bits_per_sample / 8;
On 2015/04/24 15:04:47, halliwell wrote:
> Other comment on bits/8 applies here (actually looking a few lines below,
> AudioBus takes bytes per sample, makes me think we're always 0 mod 8)?

Yup, please see my response to your other comment, and let me know about typical
use cases :)

https://codereview.chromium.org/1105803002/diff/20001/chromecast/public/cast_...
File chromecast/public/cast_audio_output_device.h (right):

https://codereview.chromium.org/1105803002/diff/20001/chromecast/public/cast_...
chromecast/public/cast_audio_output_device.h:8: #include <string>
On 2015/04/24 15:04:48, halliwell wrote:
> Is <string> used?

Nope. Artifact of re-factoring. Removed.

https://codereview.chromium.org/1105803002/diff/20001/chromecast/public/cast_...
chromecast/public/cast_audio_output_device.h:22: 
On 2015/04/24 15:04:48, halliwell wrote:
> How do implementations get instantiated ... is that a follow-up CL?

Yes, follow-up CL. This merits a discussion about when the implementation is
injected. It needs to happen sometime before the main browser loop, but exactly
when and where is TBD.

I've opened a bug to track this: b/20551606

https://codereview.chromium.org/1105803002/diff/20001/chromecast/public/cast_...
chromecast/public/cast_audio_output_device.h:37: // does not take ownership of
the returned stream.
On 2015/04/24 15:04:48, halliwell wrote:
> So what's the lifetime of these streams ... they live till process exits?

Good question. For now, that is left up to the implementation of the stream and
the output device. Two assumptions which must hold:
 1) The pointer to the stream must remain valid until Close() is called or an
OnError() event is detected.
 2) The stream should not be freed, except by the output device, who owns it. 

Beyond that, it is left to implementer of the output device to decide when to
delete the stream (if at all).

However, I like the approach of the stream alerting the output device when it
has been Close()d. The output device (who owns the stream) then deletes the
stream. 

WDYT? Is this a robust approach?

https://codereview.chromium.org/1105803002/diff/20001/chromecast/public/cast_...
File chromecast/public/cast_audio_stream.h (right):

https://codereview.chromium.org/1105803002/diff/20001/chromecast/public/cast_...
chromecast/public/cast_audio_stream.h:15: // media::AudioParmameters.
On 2015/04/24 15:04:48, halliwell wrote:
> typo: Parameters

Done.

https://codereview.chromium.org/1105803002/diff/20001/chromecast/public/cast_...
chromecast/public/cast_audio_stream.h:43: //   |len| >= |frames| * (num
channels) * (bits per sample) / 8
On 2015/04/24 15:04:48, halliwell wrote:
> If we ever had e.g. 20 bit audio, then bits/8 needs rounding up not down.  And
> if we explicitly don't support non-multiple-of-8 bit depths, consider
enforcing
> that by using bytes per sample not bits??

Good catch. I was making the blind assumption that only multiples of 8 would be
supported. media::AudioBus only natively supports uint8_t, int16_t, and int32_t.

Is there a reasonable case for supporting other bit depths? If not, I will
enforce via bytes_per_sample (which will map [1,2,3] to the respective types
above, as media::AudioBus does).

Please give feedback about use cases.

https://codereview.chromium.org/1105803002/diff/20001/chromecast/public/cast_...
chromecast/public/cast_audio_stream.h:86: virtual void GetVolume(double* volume)
= 0;
On 2015/04/24 15:04:48, halliwell wrote:
> Any reason why we don't just return double here?

No reason. Changed.

[halliwell, 2015-04-25 01:34:10]

https://codereview.chromium.org/1105803002/diff/20001/chromecast/public/cast_...
File chromecast/public/cast_audio_output_device.h (right):

https://codereview.chromium.org/1105803002/diff/20001/chromecast/public/cast_...
chromecast/public/cast_audio_output_device.h:37: // does not take ownership of
the returned stream.
On 2015/04/24 16:19:39, slan wrote:
> On 2015/04/24 15:04:48, halliwell wrote:
> > So what's the lifetime of these streams ... they live till process exits?
> 
> Good question. For now, that is left up to the implementation of the stream
and
> the output device. Two assumptions which must hold:
>  1) The pointer to the stream must remain valid until Close() is called or an
> OnError() event is detected.
>  2) The stream should not be freed, except by the output device, who owns it. 
> 
> Beyond that, it is left to implementer of the output device to decide when to
> delete the stream (if at all).
> 
> However, I like the approach of the stream alerting the output device when it
> has been Close()d. The output device (who owns the stream) then deletes the
> stream. 
> 
> WDYT? Is this a robust approach?

I'm not familiar with practical usage of these classes, so someone else should
comment, but it doesn't quite sound robust to me.  Considering that the
'OnClose' and 'OnError' callbacks are documented as happening on a random
thread, and if the stream will be deleted after them, it's not clear how the
client code can safely call methods on the stream and track its lifetime.  I
would have thought you either need some kind of threadsafe refcounting pointer
type for the client (could be weak), or you need some kind of stronger/clearer
threading model, e.g. if the notification that a stream is about to be deleted
comes on the same thread that it's used from, that would be safe.

https://codereview.chromium.org/1105803002/diff/20001/chromecast/public/cast_...
File chromecast/public/cast_audio_stream.h (right):

https://codereview.chromium.org/1105803002/diff/20001/chromecast/public/cast_...
chromecast/public/cast_audio_stream.h:43: //   |len| >= |frames| * (num
channels) * (bits per sample) / 8
On 2015/04/24 16:19:39, slan wrote:
> On 2015/04/24 15:04:48, halliwell wrote:
> > If we ever had e.g. 20 bit audio, then bits/8 needs rounding up not down. 
And
> > if we explicitly don't support non-multiple-of-8 bit depths, consider
> enforcing
> > that by using bytes per sample not bits??
> 
> Good catch. I was making the blind assumption that only multiples of 8 would
be
> supported. media::AudioBus only natively supports uint8_t, int16_t, and
int32_t.
> 
> Is there a reasonable case for supporting other bit depths? If not, I will
> enforce via bytes_per_sample (which will map [1,2,3] to the respective types
> above, as media::AudioBus does).
> 
> Please give feedback about use cases.

Someone else probably knows better than me, but based on AudioBus, I'm guessing
Chromium doesn't deal with non-multiple-of-8 bitrates?  Another possibility, if
people like working with bitrates rather than bytes, would just be to have an
assert that bitrate % 8 == 0 so that's documented?

[erickung1, 2015-04-25 08:38:31]

(just take a look the CL very quickly, will look into details later on)

1. files under chromecast/ use like ::media::AudiorManager to indicates the
class/structure is from media/. This is because there is chromecast::media
namespace and we don't want to mess it up. I wonder if we should do that here
for code consistency (example
https://code.google.com/p/chromium/codesearch#chromium/src/chromecast/browser...)

2. (Please correct me if I'm wrong) Is this the path to push audio data to 3rd
party/OEM vendor? For Cast, I think it's really confusing that we have two
interfaces, one is CMA to support A/V and this is for web audio. Since there is
a plan to implement CMA shim to access ALSA, should we consolidate the code like
CastAudioManager to call CMA? Or is there any restriction that I may miss?? ?

https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
File chromecast/media/audio/cast_audio_manager.cc (right):

https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
chromecast/media/audio/cast_audio_manager.cc:77: // TODO(damienv): Providing the
default AudioParameters
This can be someone else name who will make the change. :)

https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
File chromecast/media/audio/chromecast_device_audio_output_stream.cc (right):

https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
chromecast/media/audio/chromecast_device_audio_output_stream.cc:14: :
output_stream_(output_stream), params_(params) {
Add DCHECK(output_stream) ?
Also set |callback_| to NULL

https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
chromecast/media/audio/chromecast_device_audio_output_stream.cc:26: callback_ =
callback;
We may want to add DCHECK on |callback| here.
Otherwise, OnError() we should only call the callback function if |callback_| is
not NULL

[gunsch, 2015-04-27 16:09:11]

A few comments included, mostly style conventions/nits. Others already on the CL
know the interface itself better than I do.

One comment I'd like to add for a follow-up (not necessarily this CL): it'd be
good to end up with a platform-level test that verifies the platform does the
right thing---sort of a verification test. I think we do something similar in
audio_video_pipeline_device_unittests.cc, for example (load a few sample A/V
files and run them through the platform-provided implementation).

https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
File chromecast/media/audio/cast_audio_manager.h (right):

https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
chromecast/media/audio/cast_audio_manager.h:24: class CastAudioManager : public
media::AudioManagerBase {
+1 to Eric's comment about namespacing, prefer ::media::AudioManagerBase (and
other references throughout this file). This will break if you add an include
from "chromecast/media/base/", for an example.

https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
chromecast/media/audio/cast_audio_manager.h:26: // This class acts as a proxy
for chromecast::CastAudioOutputDevice,
Documentation nit: separate the "class" docs from the "ctor" docs, put those
just above the class name

https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
File chromecast/media/audio/cast_audio_manager_unittest.cc (right):

https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
chromecast/media/audio/cast_audio_manager_unittest.cc:199: EXPECT_CALL(log,
Log(logging::LOG_WARNING, _, _, _, _)).Times(2);
verifying number of WARNING statements logged seems really inflexible over time,
do you think this is an important part of the test? (here and above)

https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
File chromecast/media/audio/chromecast_device_audio_output_stream.cc (right):

https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
chromecast/media/audio/chromecast_device_audio_output_stream.cc:32: }
Is Stop() parallel/opposite to Start()? If so, should it reset callback_?

https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
File chromecast/media/audio/chromecast_device_audio_output_stream.h (right):

https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
chromecast/media/audio/chromecast_device_audio_output_stream.h:22: : public
chromecast::CastAudioOutputStream::AudioSourceCallback,
style nit: don't fully-qualify chromecast-namespaced things within the
chromecast namespace

https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
chromecast/media/audio/chromecast_device_audio_output_stream.h:26:
chromecast::CastAudioOutputStream* output_stream,
Describe the ownership model here, since you're passing a raw pointer. Can you
do this without a raw pointer?

https://codereview.chromium.org/1105803002/diff/40001/chromecast/public/cast_...
File chromecast/public/cast_audio_stream.h (right):

https://codereview.chromium.org/1105803002/diff/40001/chromecast/public/cast_...
chromecast/public/cast_audio_stream.h:5: #ifndef
CHROMECAST_PUBLIC_CAST_AUDIO_STREAM_H_
filename should match class name

[byungchul, 2015-04-27 17:38:06]

https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
File chromecast/media/audio/cast_audio_manager.cc (right):

https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
chromecast/media/audio/cast_audio_manager.cc:41: }
a blank line below here please.

https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
chromecast/media/audio/cast_audio_manager.cc:71: DCHECK(device_names->empty());
Is this necessary?

https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
chromecast/media/audio/cast_audio_manager.cc:84: kDefaultSampleRate,
kDefaultBitsPerSample,
wrap line after , then one argument per line

https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
chromecast/media/audio/cast_audio_manager.cc:138: new
ChromecastDeviceAudioOutputStream(stream, chromecast_params)));
It is not correct since this function returns a raw pointer. You cannot prevent
the object being deleted. What is streams_ for? It is not used for now.

https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
File chromecast/media/audio/cast_audio_manager.h (right):

https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
chromecast/media/audio/cast_audio_manager.h:33: CastAudioOutputDevice*
audio_output_device);
who instantiate CastAudioOutputDevice?

https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
chromecast/media/audio/cast_audio_manager.h:70:
chromecast::CastAudioOutputDevice* audio_output_device_;
* const

https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
File chromecast/media/audio/cast_audio_manager_factory.h (right):

https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
chromecast/media/audio/cast_audio_manager_factory.h:32:
chromecast::CastAudioOutputDevice* audio_stream_output_device_;
* const

https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
File chromecast/media/audio/chromecast_device_audio_output_stream.cc (right):

https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
chromecast/media/audio/chromecast_device_audio_output_stream.cc:39:
CHECK(volume);
DCHECK() should be enough.

https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
chromecast/media/audio/chromecast_device_audio_output_stream.cc:76: DVLOG(1) <<
"Ouput stream has been closed.";
Why not just VLOG(1)?

https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
File chromecast/media/audio/chromecast_device_audio_output_stream.h (right):

https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
chromecast/media/audio/chromecast_device_audio_output_stream.h:22: : public
chromecast::CastAudioOutputStream::AudioSourceCallback,
On 2015/04/27 16:09:11, gunsch wrote:
> style nit: don't fully-qualify chromecast-namespaced things within the
> chromecast namespace

And, it doesn't have to be public.

https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
chromecast/media/audio/chromecast_device_audio_output_stream.h:48:
chromecast::CastAudioOutputStream* output_stream_;
* const

https://codereview.chromium.org/1105803002/diff/40001/chromecast/public/cast_...
File chromecast/public/cast_audio_output_device.h (right):

https://codereview.chromium.org/1105803002/diff/40001/chromecast/public/cast_...
chromecast/public/cast_audio_output_device.h:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
You should add these 2 public header files in cast_public_api target in
chromecast.gyp

https://codereview.chromium.org/1105803002/diff/40001/chromecast/public/cast_...
File chromecast/public/cast_audio_stream.h (right):

https://codereview.chromium.org/1105803002/diff/40001/chromecast/public/cast_...
chromecast/public/cast_audio_stream.h:15: // media::AudioParameters.
This file is for oem partners, and supposed to be harder to change api once it
is released. The comment should be self-contained.

https://codereview.chromium.org/1105803002/diff/40001/chromecast/public/cast_...
chromecast/public/cast_audio_stream.h:17: struct CHROMECAST_EXPORT
AudioParameters {
CHROMECAST_EXPORT not necessary.

https://codereview.chromium.org/1105803002/diff/40001/chromecast/public/cast_...
chromecast/public/cast_audio_stream.h:28: class CHROMECAST_EXPORT
CastAudioOutputStream {
CHROMECAST_EXPORT not necessary.

https://codereview.chromium.org/1105803002/diff/40001/chromecast/public/cast_...
chromecast/public/cast_audio_stream.h:34: class AudioSourceCallback {
What is thread model here? Do we allow callback functions called on a different
thread? Please specify what is allowed and what is not.

https://codereview.chromium.org/1105803002/diff/40001/chromecast/public/cast_...
chromecast/public/cast_audio_stream.h:50: virtual int OnMoreData(void* dest,
is void* enough to explain how dest will be filled?

https://codereview.chromium.org/1105803002/diff/40001/chromecast/public/cast_...
chromecast/public/cast_audio_stream.h:69: virtual bool Open() = 0;
Do we allow this function block the thread?

https://codereview.chromium.org/1105803002/diff/40001/chromecast/public/cast_...
chromecast/public/cast_audio_stream.h:85: // Gets the relative volume, with
range [0.0, 1.0] inclusive.
Do we allow this function blocking the thread?

[cleichner, 2015-04-27 22:15:34]

On 2015/04/25 08:38:31, erickung1 wrote:
> (just take a look the CL very quickly, will look into details later on)
> 
> 1. files under chromecast/ use like ::media::AudiorManager to indicates the
> class/structure is from media/. This is because there is chromecast::media
> namespace and we don't want to mess it up. I wonder if we should do that here
> for code consistency (example
>
https://code.google.com/p/chromium/codesearch#chromium/src/chromecast/browser...)
> 
> 2. (Please correct me if I'm wrong) Is this the path to push audio data to 3rd
> party/OEM vendor? For Cast, I think it's really confusing that we have two
> interfaces, one is CMA to support A/V and this is for web audio. Since there
is
> a plan to implement CMA shim to access ALSA, should we consolidate the code
like
> CastAudioManager to call CMA? Or is there any restriction that I may miss?? ?
> 
>
https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
> File chromecast/media/audio/cast_audio_manager.cc (right):
> 
>
https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
> chromecast/media/audio/cast_audio_manager.cc:77: // TODO(damienv): Providing
the
> default AudioParameters
> This can be someone else name who will make the change. :)
> 
>
https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
> File chromecast/media/audio/chromecast_device_audio_output_stream.cc (right):
> 
>
https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
> chromecast/media/audio/chromecast_device_audio_output_stream.cc:14: :
> output_stream_(output_stream), params_(params) {
> Add DCHECK(output_stream) ?
> Also set |callback_| to NULL
> 
>
https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
> chromecast/media/audio/chromecast_device_audio_output_stream.cc:26: callback_
=
> callback;
> We may want to add DCHECK on |callback| here.
> Otherwise, OnError() we should only call the callback function if |callback_|
is
> not NULL

I agree with you that the having two paths into the Chromecast media stack is
confusing. I talked about CMA with Stephen and he is looking into simplifying
this interface.

[gunsch-google, 2015-04-27 23:25:56]

On 2015/04/27 22:15:34, cleichner wrote:
> On 2015/04/25 08:38:31, erickung1 wrote:
> > (just take a look the CL very quickly, will look into details later on)
> > 
> > 1. files under chromecast/ use like ::media::AudiorManager to indicates the
> > class/structure is from media/. This is because there is chromecast::media
> > namespace and we don't want to mess it up. I wonder if we should do that
here
> > for code consistency (example
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chromecast/browser...)
> > 
> > 2. (Please correct me if I'm wrong) Is this the path to push audio data to
3rd
> > party/OEM vendor? For Cast, I think it's really confusing that we have two
> > interfaces, one is CMA to support A/V and this is for web audio. Since there
> is
> > a plan to implement CMA shim to access ALSA, should we consolidate the code
> like
> > CastAudioManager to call CMA? Or is there any restriction that I may miss??
?
> > 
> >
>
https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
> > File chromecast/media/audio/cast_audio_manager.cc (right):
> > 
> >
>
https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
> > chromecast/media/audio/cast_audio_manager.cc:77: // TODO(damienv): Providing
> the
> > default AudioParameters
> > This can be someone else name who will make the change. :)
> > 
> >
>
https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
> > File chromecast/media/audio/chromecast_device_audio_output_stream.cc
(right):
> > 
> >
>
https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
> > chromecast/media/audio/chromecast_device_audio_output_stream.cc:14: :
> > output_stream_(output_stream), params_(params) {
> > Add DCHECK(output_stream) ?
> > Also set |callback_| to NULL
> > 
> >
>
https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
> > chromecast/media/audio/chromecast_device_audio_output_stream.cc:26:
callback_
> =
> > callback;
> > We may want to add DCHECK on |callback| here.
> > Otherwise, OnError() we should only call the callback function if
|callback_|
> is
> > not NULL
> 
> I agree with you that the having two paths into the Chromecast media stack is
> confusing. I talked about CMA with Stephen and he is looking into simplifying
> this interface.

Chas: what's the intention with this comment? I don't know that this is an
appropriate time to try to merge WebAudio output interfaces with the CMA
interfaces. Note that this complexity isn't introduced by us, but by the fact
that Chrome already has 2+ completely different paths to push A/V.

[slan, 2015-04-28 00:09:31]

This discussion does in fact impact this CL. If we decide to pipe web audio
through CMA, to the end that vendors would only have to implement one audio
pipeline, then exposing the ::chromecast::CastAudioOutputDevice interface
is no longer useful. I just uploaded a patch with changes to the comments
already posted. Please take a look if you wish, but do no further review on
that interface.

I am about to explore a solution which delivers low latency audio using
CMA. If we can implement a ::media::AudioOutputStream which streams to CMA,
then we will drop the interface listed above. Most of the rest of the CL
should not change. Please hold off reviewing until I can guage the
feasability of this idea. Hopefully will be sometime tomorrow.

Thanks!

On Mon, Apr 27, 2015 at 4:25 PM, <gunsch@google.com> wrote:

> On 2015/04/27 22:15:34, cleichner wrote:
>
>> On 2015/04/25 08:38:31, erickung1 wrote:
>> > (just take a look the CL very quickly, will look into details later on)
>> >
>> > 1. files under chromecast/ use like ::media::AudiorManager to indicates
>> the
>> > class/structure is from media/. This is because there is
>> chromecast::media
>> > namespace and we don't want to mess it up. I wonder if we should do that
>>
> here
>
>> > for code consistency (example
>> >
>>
>
>
>
https://code.google.com/p/chromium/codesearch#chromium/src/chromecast/browser...
> )
>
>> >
>> > 2. (Please correct me if I'm wrong) Is this the path to push audio data
>> to
>>
> 3rd
>
>> > party/OEM vendor? For Cast, I think it's really confusing that we have
>> two
>> > interfaces, one is CMA to support A/V and this is for web audio. Since
>> there
>> is
>> > a plan to implement CMA shim to access ALSA, should we consolidate the
>> code
>> like
>> > CastAudioManager to call CMA? Or is there any restriction that I may
>> miss??
>>
> ?
>
>> >
>> >
>>
>
>
>
https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
>
>> > File chromecast/media/audio/cast_audio_manager.cc (right):
>> >
>> >
>>
>
>
>
https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
>
>> > chromecast/media/audio/cast_audio_manager.cc:77: // TODO(damienv):
>> Providing
>> the
>> > default AudioParameters
>> > This can be someone else name who will make the change. :)
>> >
>> >
>>
>
>
>
https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
>
>> > File chromecast/media/audio/chromecast_device_audio_output_stream.cc
>>
> (right):
>
>> >
>> >
>>
>
>
>
https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
>
>> > chromecast/media/audio/chromecast_device_audio_output_stream.cc:14: :
>> > output_stream_(output_stream), params_(params) {
>> > Add DCHECK(output_stream) ?
>> > Also set |callback_| to NULL
>> >
>> >
>>
>
>
>
https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
>
>> > chromecast/media/audio/chromecast_device_audio_output_stream.cc:26:
>>
> callback_
>
>> =
>> > callback;
>> > We may want to add DCHECK on |callback| here.
>> > Otherwise, OnError() we should only call the callback function if
>>
> |callback_|
>
>> is
>> > not NULL
>>
>
>  I agree with you that the having two paths into the Chromecast media
>> stack is
>> confusing. I talked about CMA with Stephen and he is looking into
>> simplifying
>> this interface.
>>
>
> Chas: what's the intention with this comment? I don't know that this is an
> appropriate time to try to merge WebAudio output interfaces with the CMA
> interfaces. Note that this complexity isn't introduced by us, but by the
> fact
> that Chrome already has 2+ completely different paths to push A/V.
>
> https://codereview.chromium.org/1105803002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[slan, 2015-04-28 00:11:39]

This patch addresses all of the issues raised in the last few reviews. Please
take a look if you wish, but please ***hold off on any further review***.

I am about to explore a solution which delivers low latency audio using CMA. If
we can implement a ::media::AudioOutputStream which streams to CMA, then we will
drop the CaastAudioOutputStream interface. Most of the rest of the CL should not
change. Please hold off reviewing until I can guage the feasability of this
idea.

Thanks for all of the review!!

https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
File chromecast/media/audio/cast_audio_manager.cc (right):

https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
chromecast/media/audio/cast_audio_manager.cc:41: }
On 2015/04/27 17:38:05, byungchul wrote:
> a blank line below here please.

Done.

https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
chromecast/media/audio/cast_audio_manager.cc:71: DCHECK(device_names->empty());
On 2015/04/27 17:38:05, byungchul wrote:
> Is this necessary?

No. Removed.

https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
chromecast/media/audio/cast_audio_manager.cc:77: // TODO(damienv): Providing the
default AudioParameters
On 2015/04/25 08:38:31, erickung1 wrote:
> This can be someone else name who will make the change. :)

Done. + bug assigned b/20630187

https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
chromecast/media/audio/cast_audio_manager.cc:84: kDefaultSampleRate,
kDefaultBitsPerSample,
On 2015/04/27 17:38:05, byungchul wrote:
> wrap line after , then one argument per line

Done.

https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
chromecast/media/audio/cast_audio_manager.cc:138: new
ChromecastDeviceAudioOutputStream(stream, chromecast_params)));
On 2015/04/27 17:38:05, byungchul wrote:
> It is not correct since this function returns a raw pointer. You cannot
prevent
> the object being deleted. What is streams_ for? It is not used for now.

We need to return a raw pointer to satisfy the ::media::AudioManager interface.
This does not transfer ownership of the returned pointer to the caller.
|streams_| is the container for the implementations of
::media::AudioOutputStream. These are owned by this class.

https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
File chromecast/media/audio/cast_audio_manager.h (right):

https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
chromecast/media/audio/cast_audio_manager.h:24: class CastAudioManager : public
media::AudioManagerBase {
On 2015/04/27 16:09:11, gunsch wrote:
> +1 to Eric's comment about namespacing, prefer ::media::AudioManagerBase (and
> other references throughout this file). This will break if you add an include
> from "chromecast/media/base/", for an example.

Done for entire chromecast/media/audio directory.

https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
chromecast/media/audio/cast_audio_manager.h:26: // This class acts as a proxy
for chromecast::CastAudioOutputDevice,
On 2015/04/27 16:09:11, gunsch wrote:
> Documentation nit: separate the "class" docs from the "ctor" docs, put those
> just above the class name

Done.

https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
chromecast/media/audio/cast_audio_manager.h:33: CastAudioOutputDevice*
audio_output_device);
On 2015/04/27 17:38:06, byungchul wrote:
> who instantiate CastAudioOutputDevice?

|audio_output_device| should be a global instance instatiated by the thirdparty
platform code. We will likely need to expose a public method with the following
signature:

static void CastAudioOutputDevice::SetDevice(CastAudioOutputDevice* device); 

This will come in a following CL: b/20551606

https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
chromecast/media/audio/cast_audio_manager.h:70:
chromecast::CastAudioOutputDevice* audio_output_device_;
On 2015/04/27 17:38:06, byungchul wrote:
> * const

Done.

https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
File chromecast/media/audio/cast_audio_manager_factory.h (right):

https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
chromecast/media/audio/cast_audio_manager_factory.h:32:
chromecast::CastAudioOutputDevice* audio_stream_output_device_;
On 2015/04/27 17:38:06, byungchul wrote:
> * const

Done.

https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
File chromecast/media/audio/cast_audio_manager_unittest.cc (right):

https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
chromecast/media/audio/cast_audio_manager_unittest.cc:199: EXPECT_CALL(log,
Log(logging::LOG_WARNING, _, _, _, _)).Times(2);
On 2015/04/27 16:09:11, gunsch wrote:
> verifying number of WARNING statements logged seems really inflexible over
time,
> do you think this is an important part of the test? (here and above)

There are two calls, so two warnings should pop up. Refactored for clarity.

https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
File chromecast/media/audio/chromecast_device_audio_output_stream.cc (right):

https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
chromecast/media/audio/chromecast_device_audio_output_stream.cc:14: :
output_stream_(output_stream), params_(params) {
On 2015/04/25 08:38:31, erickung1 wrote:
> Add DCHECK(output_stream) ?
> Also set |callback_| to NULL

Done.

https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
chromecast/media/audio/chromecast_device_audio_output_stream.cc:26: callback_ =
callback;
On 2015/04/25 08:38:31, erickung1 wrote:
> We may want to add DCHECK on |callback| here.
> Otherwise, OnError() we should only call the callback function if |callback_|
is
> not NULL

Done.

https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
chromecast/media/audio/chromecast_device_audio_output_stream.cc:32: }
On 2015/04/27 16:09:11, gunsch wrote:
> Is Stop() parallel/opposite to Start()? If so, should it reset callback_?

Really good catch. Done. We now ignore OnError and OnMoreData events if callback
is null.

https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
chromecast/media/audio/chromecast_device_audio_output_stream.cc:39:
CHECK(volume);
On 2015/04/27 17:38:06, byungchul wrote:
> DCHECK() should be enough.

Done.

https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
chromecast/media/audio/chromecast_device_audio_output_stream.cc:76: DVLOG(1) <<
"Ouput stream has been closed.";
On 2015/04/27 17:38:06, byungchul wrote:
> Why not just VLOG(1)?

Done.

https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
File chromecast/media/audio/chromecast_device_audio_output_stream.h (right):

https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
chromecast/media/audio/chromecast_device_audio_output_stream.h:22: : public
chromecast::CastAudioOutputStream::AudioSourceCallback,
On 2015/04/27 16:09:11, gunsch wrote:
> style nit: don't fully-qualify chromecast-namespaced things within the
> chromecast namespace

Done.

https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
chromecast/media/audio/chromecast_device_audio_output_stream.h:22: : public
chromecast::CastAudioOutputStream::AudioSourceCallback,
On 2015/04/27 17:38:06, byungchul wrote:
> On 2015/04/27 16:09:11, gunsch wrote:
> > style nit: don't fully-qualify chromecast-namespaced things within the
> > chromecast namespace
> 
> And, it doesn't have to be public.

Done.

https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
chromecast/media/audio/chromecast_device_audio_output_stream.h:26:
chromecast::CastAudioOutputStream* output_stream,
On 2015/04/27 16:09:11, gunsch wrote:
> Describe the ownership model here, since you're passing a raw pointer. Can you
> do this without a raw pointer?

Documentation added clarifying the ownership model.

https://codereview.chromium.org/1105803002/diff/40001/chromecast/media/audio/...
chromecast/media/audio/chromecast_device_audio_output_stream.h:48:
chromecast::CastAudioOutputStream* output_stream_;
On 2015/04/27 17:38:06, byungchul wrote:
> * const

Done.

https://codereview.chromium.org/1105803002/diff/40001/chromecast/public/cast_...
File chromecast/public/cast_audio_output_device.h (right):

https://codereview.chromium.org/1105803002/diff/40001/chromecast/public/cast_...
chromecast/public/cast_audio_output_device.h:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
On 2015/04/27 17:38:06, byungchul wrote:
> You should add these 2 public header files in cast_public_api target in
> chromecast.gyp

Done.

https://codereview.chromium.org/1105803002/diff/40001/chromecast/public/cast_...
File chromecast/public/cast_audio_stream.h (right):

https://codereview.chromium.org/1105803002/diff/40001/chromecast/public/cast_...
chromecast/public/cast_audio_stream.h:5: #ifndef
CHROMECAST_PUBLIC_CAST_AUDIO_STREAM_H_
On 2015/04/27 16:09:11, gunsch wrote:
> filename should match class name

Done.

https://codereview.chromium.org/1105803002/diff/40001/chromecast/public/cast_...
chromecast/public/cast_audio_stream.h:15: // media::AudioParameters.
On 2015/04/27 17:38:06, byungchul wrote:
> This file is for oem partners, and supposed to be harder to change api once it
> is released. The comment should be self-contained.

Done.

https://codereview.chromium.org/1105803002/diff/40001/chromecast/public/cast_...
chromecast/public/cast_audio_stream.h:17: struct CHROMECAST_EXPORT
AudioParameters {
On 2015/04/27 17:38:06, byungchul wrote:
> CHROMECAST_EXPORT not necessary.

Done.

https://codereview.chromium.org/1105803002/diff/40001/chromecast/public/cast_...
chromecast/public/cast_audio_stream.h:28: class CHROMECAST_EXPORT
CastAudioOutputStream {
On 2015/04/27 17:38:06, byungchul wrote:
> CHROMECAST_EXPORT not necessary.

Done.

https://codereview.chromium.org/1105803002/diff/40001/chromecast/public/cast_...
chromecast/public/cast_audio_stream.h:34: class AudioSourceCallback {
On 2015/04/27 17:38:06, byungchul wrote:
> What is thread model here? Do we allow callback functions called on a
different
> thread? Please specify what is allowed and what is not.

Done.

https://codereview.chromium.org/1105803002/diff/40001/chromecast/public/cast_...
chromecast/public/cast_audio_stream.h:50: virtual int OnMoreData(void* dest,
On 2015/04/27 17:38:06, byungchul wrote:
> is void* enough to explain how dest will be filled?

Documentation added explaining interleaved format.

https://codereview.chromium.org/1105803002/diff/40001/chromecast/public/cast_...
chromecast/public/cast_audio_stream.h:69: virtual bool Open() = 0;
On 2015/04/27 17:38:06, byungchul wrote:
> Do we allow this function block the thread?

I think this call should be able to block the audio thread. Otherwise, the
control flow is misleading. Documentation added.

https://codereview.chromium.org/1105803002/diff/40001/chromecast/public/cast_...
chromecast/public/cast_audio_stream.h:85: // Gets the relative volume, with
range [0.0, 1.0] inclusive.
On 2015/04/27 17:38:06, byungchul wrote:
> Do we allow this function blocking the thread?

This one should block as well. Documentation added.

[slan, 2015-07-22 22:05:28]

slan@chromium.org changed reviewers:
+ alokp@chromium.org

[alokp, 2015-07-23 22:56:28]

> > I agree with you that the having two paths into the Chromecast media stack
is
> > confusing. I talked about CMA with Stephen and he is looking into
simplifying
> > this interface.
> 
> Chas: what's the intention with this comment? I don't know that this is an
> appropriate time to try to merge WebAudio output interfaces with the CMA
> interfaces. Note that this complexity isn't introduced by us, but by the fact
> that Chrome already has 2+ completely different paths to push A/V.

Jesse: Are you referring to the audio pipeline for WebAudio and media::Renderer
here?

I looked at this closely today and discovered that in the default chromium
implementation both paths do converge at media::AudioOutputIPC, right before the
audio data is sent to the browser process. It bypasses the media::Renderer
because it does not need to for raw audio stream. Since the audio paths do
converge, I would not call it completely different paths.

Were you referring to any other AV pipeline?

[gunsch-google, 2015-07-24 04:02:12]

On 2015/07/23 22:56:28, Alok Priyadarshi wrote:
> > > I agree with you that the having two paths into the Chromecast media stack
> is
> > > confusing. I talked about CMA with Stephen and he is looking into
> simplifying
> > > this interface.
> > 
> > Chas: what's the intention with this comment? I don't know that this is an
> > appropriate time to try to merge WebAudio output interfaces with the CMA
> > interfaces. Note that this complexity isn't introduced by us, but by the
fact
> > that Chrome already has 2+ completely different paths to push A/V.
> 
> Jesse: Are you referring to the audio pipeline for WebAudio and
media::Renderer
> here?

Yes, those are the two paths I'm referring to.

> 
> I looked at this closely today and discovered that in the default chromium
> implementation both paths do converge at media::AudioOutputIPC, right before
the
> audio data is sent to the browser process. It bypasses the media::Renderer
> because it does not need to for raw audio stream. Since the audio paths do
> converge, I would not call it completely different paths.

True, audio does converge in the Chrome Linux paths before being output (which
makes sense). A couple thoughts:

(1) For better or worse, media::Renderer interface was designed to splinter off
one of those paths from the other. Note that WebRTC is still yet another
separate rendering path.

(2) I'm not 100% sure, but my initial skimming of the media::AudioOutputIPC path
seems to assume it's using decoded audio (rules out AC3/E-AC3)

(3) Does MojoMediaRenderer address (1)? I was under the impression it still only
handles the equivalent cases as media::Renderer.

We should probably sync up with the Chrome media folks at some point, likely
after our pipeline discussion on Tuesday.

> 
> Were you referring to any other AV pipeline?

[alokp, 2015-07-24 06:12:52]

On 2015/07/24 04:02:12, gunsch-google wrote:
> On 2015/07/23 22:56:28, Alok Priyadarshi wrote:
> > > > I agree with you that the having two paths into the Chromecast media
stack
> > is
> > > > confusing. I talked about CMA with Stephen and he is looking into
> > simplifying
> > > > this interface.
> > > 
> > > Chas: what's the intention with this comment? I don't know that this is an
> > > appropriate time to try to merge WebAudio output interfaces with the CMA
> > > interfaces. Note that this complexity isn't introduced by us, but by the
> fact
> > > that Chrome already has 2+ completely different paths to push A/V.
> > 
> > Jesse: Are you referring to the audio pipeline for WebAudio and
> media::Renderer
> > here?
> 
> Yes, those are the two paths I'm referring to.
> 
> > 
> > I looked at this closely today and discovered that in the default chromium
> > implementation both paths do converge at media::AudioOutputIPC, right before
> the
> > audio data is sent to the browser process. It bypasses the media::Renderer
> > because it does not need to for raw audio stream. Since the audio paths do
> > converge, I would not call it completely different paths.
> 
> True, audio does converge in the Chrome Linux paths before being output (which
> makes sense). A couple thoughts:
> 
> (1) For better or worse, media::Renderer interface was designed to splinter
off
> one of those paths from the other. Note that WebRTC is still yet another
> separate rendering path.

WebRTC audio path is still the same. It converges at media::AudioOutputDevice -
one step before media::AudioOutputIPC.

> (2) I'm not 100% sure, but my initial skimming of the media::AudioOutputIPC
path
> seems to assume it's using decoded audio (rules out AC3/E-AC3)

You are right. AFAICT media::AudioOutputIPC is for raw audio only. Coded audio
needs to be passed through media::Renderer. Raw audio bypasses the
media::Renderer but the two paths converge at media::AudioOutputIPC.
 
> (3) Does MojoMediaRenderer address (1)? I was under the impression it still
only
> handles the equivalent cases as media::Renderer.

Not at present. I had added a comment about this in the design doc. Dale replied
that it will be added in future. Mojo data pipe currently lacks the primitives
required to implement it in an efficient manner.

> We should probably sync up with the Chrome media folks at some point, likely
> after our pipeline discussion on Tuesday.
> 
> > 
> > Were you referring to any other AV pipeline?

[slan, 2015-07-29 08:23:38]

Hey all, 

This CL is being unearthed for the upcoming release. Please take a close look at
the new interfaces defined in public/.

the code has been updated to leverage some code used by halliwell@ here:
https://codereview.chromium.org/1257013003. I have verified it on production
hardware with corresponding internal CL's.

[halliwell, 2015-07-29 15:06:57]

https://codereview.chromium.org/1105803002/diff/100001/chromecast/media/audio...
File chromecast/media/audio/cast_audio_manager.cc (right):

https://codereview.chromium.org/1105803002/diff/100001/chromecast/media/audio...
chromecast/media/audio/cast_audio_manager.cc:112: chromecast_params, new
chromecast::TaskRunnerImpl());
nit: chromecast:: unnecessary.

https://codereview.chromium.org/1105803002/diff/100001/chromecast/public/cast...
File chromecast/public/cast_audio_output_device.h (right):

https://codereview.chromium.org/1105803002/diff/100001/chromecast/public/cast...
chromecast/public/cast_audio_output_device.h:15: namespace media {
we've been putting the other chromecast::media interfaces in
chromecast/public/media directory.  VIdeoPlane is an exception, it was the first
class added ... we should probably move it.  But let's move these as they're
new.

https://codereview.chromium.org/1105803002/diff/100001/chromecast/public/cast...
chromecast/public/cast_audio_output_device.h:42: // as a media rendering thread
and should be non-null.
The task_runner part seems very vague/unclear to me.  As background, in the CMA
case, I added it because:
* We have a clear threading model where we make all calls into vendor code on a
specific, designated "media thread"
* In order to simplify thread safety in implementations, it's highly desirable
for vendors to have the ability to schedule other work on that same thread. 
Hence the task runner.

This comment seems less clear to me.  What thread(s) are we calling the APIs in
this interface on, to start with?

I should probably go find the internal review as well.

[gunsch, 2015-07-29 17:22:37]

Overall the structure looks good (and I see there's overlap with Luke's CL),
handful of comments mostly on the public API docs.

https://codereview.chromium.org/1105803002/diff/100001/chromecast/base/task_r...
File chromecast/base/task_runner_impl.cc (right):

https://codereview.chromium.org/1105803002/diff/100001/chromecast/base/task_r...
chromecast/base/task_runner_impl.cc:1: // Copyright (c) 2015 The Chromium
Authors. All rights reserved.
I like how this is the same file as Luke's, but with formatting differences

https://codereview.chromium.org/1105803002/diff/100001/chromecast/public/cast...
File chromecast/public/cast_audio_output_device.h (right):

https://codereview.chromium.org/1105803002/diff/100001/chromecast/public/cast...
chromecast/public/cast_audio_output_device.h:45: chromecast::TaskRunner*
task_runner) = 0;
chromecast:: not necessary

https://codereview.chromium.org/1105803002/diff/100001/chromecast/public/cast...
File chromecast/public/cast_audio_output_stream.h (right):

https://codereview.chromium.org/1105803002/diff/100001/chromecast/public/cast...
chromecast/public/cast_audio_output_stream.h:30: // is not owned by this class,
and which must outlive it.
Let's keep guidance in public APIs to vendor implementations. "Must outlive it"
is our responsibility, not vendor's.

https://codereview.chromium.org/1105803002/diff/100001/chromecast/public/cast...
chromecast/public/cast_audio_output_stream.h:33: // Audio sources must implement
AudioSourceCallback. Calls to this interface
Same here, I think this is guidance to us, not the vendor.

https://codereview.chromium.org/1105803002/diff/100001/chromecast/public/cast...
chromecast/public/cast_audio_output_stream.h:34: // must be made on the same
thread on which the stream object is created.
Note: Byungchul recently rewrote the other shlibs such that vendor shlib can
invoke our callbacks from any thread. Can we keep that property here?

https://codereview.chromium.org/1105803002/diff/100001/chromecast/public/cast...
chromecast/public/cast_audio_output_stream.h:54: virtual int OnMoreData(void*
dest,
naming: GetMoreData?

[slan, 2015-07-29 19:51:55]

Thanks for the comments so far.

https://codereview.chromium.org/1105803002/diff/100001/chromecast/base/task_r...
File chromecast/base/task_runner_impl.cc (right):

https://codereview.chromium.org/1105803002/diff/100001/chromecast/base/task_r...
chromecast/base/task_runner_impl.cc:1: // Copyright (c) 2015 The Chromium
Authors. All rights reserved.
On 2015/07/29 17:22:36, gunsch wrote:
> I like how this is the same file as Luke's, but with formatting differences

:) Added a NOTE for review purposes.

https://codereview.chromium.org/1105803002/diff/100001/chromecast/media/audio...
File chromecast/media/audio/cast_audio_manager.cc (right):

https://codereview.chromium.org/1105803002/diff/100001/chromecast/media/audio...
chromecast/media/audio/cast_audio_manager.cc:112: chromecast_params, new
chromecast::TaskRunnerImpl());
On 2015/07/29 15:06:57, halliwell wrote:
> nit: chromecast:: unnecessary.

Removed.

https://codereview.chromium.org/1105803002/diff/100001/chromecast/public/cast...
File chromecast/public/cast_audio_output_device.h (right):

https://codereview.chromium.org/1105803002/diff/100001/chromecast/public/cast...
chromecast/public/cast_audio_output_device.h:15: namespace media {
On 2015/07/29 15:06:57, halliwell wrote:
> we've been putting the other chromecast::media interfaces in
> chromecast/public/media directory.  VIdeoPlane is an exception, it was the
first
> class added ... we should probably move it.  But let's move these as they're
> new.

Done.

https://codereview.chromium.org/1105803002/diff/100001/chromecast/public/cast...
chromecast/public/cast_audio_output_device.h:42: // as a media rendering thread
and should be non-null.
On 2015/07/29 15:06:57, halliwell wrote:
> The task_runner part seems very vague/unclear to me.  As background, in the
CMA
> case, I added it because:
> * We have a clear threading model where we make all calls into vendor code on
a
> specific, designated "media thread"
> * In order to simplify thread safety in implementations, it's highly desirable
> for vendors to have the ability to schedule other work on that same thread. 
> Hence the task runner.
> 
> This comment seems less clear to me.  What thread(s) are we calling the APIs
in
> this interface on, to start with?
> 
> I should probably go find the internal review as well.

Clarified, based on our offline conversation.

https://codereview.chromium.org/1105803002/diff/100001/chromecast/public/cast...
chromecast/public/cast_audio_output_device.h:45: chromecast::TaskRunner*
task_runner) = 0;
On 2015/07/29 17:22:36, gunsch wrote:
> chromecast:: not necessary

Done.

https://codereview.chromium.org/1105803002/diff/100001/chromecast/public/cast...
File chromecast/public/cast_audio_output_stream.h (right):

https://codereview.chromium.org/1105803002/diff/100001/chromecast/public/cast...
chromecast/public/cast_audio_output_stream.h:30: // is not owned by this class,
and which must outlive it.
On 2015/07/29 17:22:37, gunsch wrote:
> Let's keep guidance in public APIs to vendor implementations. "Must outlive
it"
> is our responsibility, not vendor's.

Done.

https://codereview.chromium.org/1105803002/diff/100001/chromecast/public/cast...
chromecast/public/cast_audio_output_stream.h:33: // Audio sources must implement
AudioSourceCallback. Calls to this interface
On 2015/07/29 17:22:36, gunsch wrote:
> Same here, I think this is guidance to us, not the vendor.

The vendor code is calling the interface from their implementation. (For
example, when their stream impl needs more data, it should know if it is safe to
call this interface on any thread).

https://codereview.chromium.org/1105803002/diff/100001/chromecast/public/cast...
chromecast/public/cast_audio_output_stream.h:34: // must be made on the same
thread on which the stream object is created.
On 2015/07/29 17:22:37, gunsch wrote:
> Note: Byungchul recently rewrote the other shlibs such that vendor shlib can
> invoke our callbacks from any thread. Can we keep that property here?

Discussed this offline.

Because |dest| is an "out" param, allowing any thread to call this would require
changing this to something like:

class CastAudioOutputStream {
 public:
  class AudioSourceCallback {
   public:
    ...
    void RequestMoreData(int frames, int delay) = 0;
  }
  ...

  void PushData(void* src, int actual_frames) = 0;
}

(Or we could pass in a function pointer as a callback to RequestMoreData())

It seems easier and cleaner to clearly define the threading model, and verify
that it is being used correctly via DCHECKs on the cast_shell side and
compliance tests.

https://codereview.chromium.org/1105803002/diff/100001/chromecast/public/cast...
chromecast/public/cast_audio_output_stream.h:54: virtual int OnMoreData(void*
dest,
On 2015/07/29 17:22:37, gunsch wrote:
> naming: GetMoreData?

Done.

[gunsch-google, 2015-07-29 20:10:13]

gunsch@google.com changed reviewers:
+ gunsch@google.com

[gunsch-google, 2015-07-29 20:10:14]

lgtm

[byungchul, 2015-07-29 20:18:49]

https://codereview.chromium.org/1105803002/diff/140001/chromecast/public/medi...
File chromecast/public/media/cast_audio_output_stream.h (right):

https://codereview.chromium.org/1105803002/diff/140001/chromecast/public/medi...
chromecast/public/media/cast_audio_output_stream.h:34: virtual
~AudioSourceCallback() {}
It should be protected since CastAudioOutputStream doesn't take the ownership.

https://codereview.chromium.org/1105803002/diff/140001/chromecast/public/medi...
chromecast/public/media/cast_audio_output_stream.h:66: virtual const
AudioParameters GetParameters() const = 0;
const function would be too restricted as a shlib api.

[halliwell, 2015-07-29 22:10:14]

lgtm % nits

https://codereview.chromium.org/1105803002/diff/140001/chromecast/public/cast...
File chromecast/public/cast_media_shlib.h (right):

https://codereview.chromium.org/1105803002/diff/140001/chromecast/public/cast...
chromecast/public/cast_media_shlib.h:46: // may be destroyed in Finalize().
May be or must be?  Check comment above about fact that we can go through
multiple init/finalize flows.  VideoPlane has to be created/destroyed for each
such cycle.  I don't know what the constraints are on audio, but we should be
clear on expectations.

https://codereview.chromium.org/1105803002/diff/140001/chromecast/public/medi...
File chromecast/public/media/cast_audio_output_device.h (right):

https://codereview.chromium.org/1105803002/diff/140001/chromecast/public/medi...
chromecast/public/media/cast_audio_output_device.h:29: virtual int
GetMaximumOutputStreamsAllowed() const = 0;
In general, don't we stay away from const on objects that vendors must
implement?

https://codereview.chromium.org/1105803002/diff/140001/chromecast/public/medi...
File chromecast/public/media/cast_audio_output_stream.h (right):

https://codereview.chromium.org/1105803002/diff/140001/chromecast/public/medi...
chromecast/public/media/cast_audio_output_stream.h:68: // Open the stream. A
call to this method must always be followed by a call to
This comment is written from the POV of us (clients of the API).  I think we try
to phrase vendor interfaces from their POV.  i.e. "this method will always be
followed by a call to Close".

[slan, 2015-07-30 20:59:18]

The CQ bit was checked by slan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-07-30 21:00:40]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1105803002/170001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1105803002/170001

[slan, 2015-07-30 21:00:42]

Since halliwell@ and I share code, I will block on his CL:
https://codereview.chromium.org/1257013003/

Performing dry-run.

https://codereview.chromium.org/1105803002/diff/140001/chromecast/public/medi...
File chromecast/public/media/cast_audio_output_device.h (right):

https://codereview.chromium.org/1105803002/diff/140001/chromecast/public/medi...
chromecast/public/media/cast_audio_output_device.h:29: virtual int
GetMaximumOutputStreamsAllowed() const = 0;
On 2015/07/29 22:10:14, halliwell wrote:
> In general, don't we stay away from const on objects that vendors must
> implement?

Done.

https://codereview.chromium.org/1105803002/diff/140001/chromecast/public/medi...
File chromecast/public/media/cast_audio_output_stream.h (right):

https://codereview.chromium.org/1105803002/diff/140001/chromecast/public/medi...
chromecast/public/media/cast_audio_output_stream.h:34: virtual
~AudioSourceCallback() {}
On 2015/07/29 20:18:49, byungchul wrote:
> It should be protected since CastAudioOutputStream doesn't take the ownership.

Done.

https://codereview.chromium.org/1105803002/diff/140001/chromecast/public/medi...
chromecast/public/media/cast_audio_output_stream.h:66: virtual const
AudioParameters GetParameters() const = 0;
On 2015/07/29 20:18:49, byungchul wrote:
> const function would be too restricted as a shlib api.
>  

Done.

https://codereview.chromium.org/1105803002/diff/140001/chromecast/public/medi...
chromecast/public/media/cast_audio_output_stream.h:68: // Open the stream. A
call to this method must always be followed by a call to
On 2015/07/29 22:10:14, halliwell wrote:
> This comment is written from the POV of us (clients of the API).  I think we
try
> to phrase vendor interfaces from their POV.  i.e. "this method will always be
> followed by a call to Close".

Done.

[commit-bot: I haz the power, 2015-07-30 21:12:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-30 21:12:14]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[halliwell, 2015-07-30 21:55:49]

https://codereview.chromium.org/1105803002/diff/140001/chromecast/public/cast...
File chromecast/public/cast_media_shlib.h (right):

https://codereview.chromium.org/1105803002/diff/140001/chromecast/public/cast...
chromecast/public/cast_media_shlib.h:46: // may be destroyed in Finalize().
On 2015/07/29 22:10:14, halliwell wrote:
> May be or must be?  Check comment above about fact that we can go through
> multiple init/finalize flows.  VideoPlane has to be created/destroyed for each
> such cycle.  I don't know what the constraints are on audio, but we should be
> clear on expectations.

This comment still not addressed, btw.

[byungchul, 2015-07-30 22:12:16]

https://codereview.chromium.org/1105803002/diff/170001/chromecast/public/medi...
File chromecast/public/media/cast_audio_output_stream.h (right):

https://codereview.chromium.org/1105803002/diff/170001/chromecast/public/medi...
chromecast/public/media/cast_audio_output_stream.h:34: virtual
~AudioSourceCallback() {}
protected should go after public ones.

[slan, 2015-09-23 03:35:04]

This patch was made obsolete by the recently-landed:
https://codereview.chromium.org/1308153005/

Closing it and enshrining it in the Hall of Fame for "Most Comments on Unlanded
Patch".