Load CMA backend from shared library

Load CMA backend from shared library

* CMA backend interfaces moved into chromecast/public/media
* CMA backend implementation loaded from libcast_media through
  CastMediaShlib
* Backend API changes to remove dependency on Chromium types:
  * DecryptContext replaced with CastKeySystem
  * scoped_refptr replaced with raw pointers
  * In particular, DecoderBufferBase replaced in the backend API
    with non-refcounted DecoderBuffer.  In order to allow us to keep
    using refcounting and in particular use DecoderBufferBase in the
    rest of our pipeline, it gets wrapped in a DecoderBufferImpl
    (implementation of DecoderBuffer) for passing to the backend. So
    backends must delete the buffer pointer they're given but this is
    actually just decrementing the refcount on the underlying buffer.
  * base::Callback replaced with abstract interfaces
  * base::TimeDelta replaced with chromecast::TimeDelta in backend
    interface, essentially a stripped-down copy
  * NonThreadSafe removed (implementations can use ThreadChecker)
* TaskRunner interface provides PostTask/PostDelayedTask service to
  backend implementations
* Backend ownership / factory model changed.  Now, backend
  implementations return a MediaPipelineBackend which manages ownership
  and hookup of all the components.  It returns access to them as raw
  pointers.  The pipeline manages the lifetime of the MediaPipelineBackend
  instance.
* GetIsCodecSupportedOnChromecast refactored to remove calls back into
  cast_shell from backends (implementation can just return 'default' to
  indicate default logic should apply, including HDMI capabilities.

BUG=508534
Committed: https://crrev.com/dad541d95f2df33f23d52dce79151291918a7a67
Cr-Commit-Position: refs/heads/master@{#342370}

[halliwell, 2015-07-27 06:19:46]

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

[commit-bot: I haz the power, 2015-07-27 06:20:19]

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

[commit-bot: I haz the power, 2015-07-27 06:40:28]

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

[commit-bot: I haz the power, 2015-07-27 06:40:28]

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

[halliwell, 2015-07-27 15:19:51]

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

[halliwell, 2015-07-27 15:26:45]

This is a big one ... if you don't have time for everything, servolk@ please
look at codec support changes, byungchul@ please look at public/shared library
API changes.

erickung, lcwu - cc'd as fyi, feel free to jump in if you want :)

Testing done so far:
* Youtube + Netflix playback on a Chromecast
* cast_media_unittests pass

Corresponding internal reviews are coming.  Other known TODO items:
* Handle statistics update frequency in cast_shell
* Add unit tests for our new TimeDelta class
* gn updates

Wanted to get the review out there before finishing all those.

[gunsch, 2015-07-27 17:14:50]

Reviewed the whole CL. Overall LG, as it's actually pretty mechanical. 

High-level comments:

* I'd prefer continuing to use the media:: primitives _even more_ than your CL
does (I realize in most cases you still do). I think we should be diverging as
little as possible.

* Can you attach a bug to this? :)

https://codereview.chromium.org/1257013003/diff/20001/chromecast/media/base/m...
File chromecast/media/base/media_codec_support.cc (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/media/base/m...
chromecast/media/base/media_codec_support.cc:12: // TODO(gunsch/servolk): delete
this definition once a solution exists upstream.
You might be able to resolve this TODO now. Internal implementation shouldn't be
calling net::DefaultIsCodecSupported after your changes.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/media/cma/ba...
File chromecast/media/cma/base/backend_client_callbacks.h (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/media/cma/ba...
chromecast/media/cma/base/backend_client_callbacks.h:5: #ifndef
CHROMECAST_MEDIA_CMA_BASE_BACKEND_CLIENT_CALLBACKS_H_
Let's put these somewhere a little higher level. Maybe cma/pipeline/ ? They're
not appropriate for use in all parts of CMA.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/media/cma/ba...
chromecast/media/cma/base/backend_client_callbacks.h:23: class
MediaComponentDeviceClientImpl : public MediaComponentDevice::Client {
file nit: can we split these into three files? I know they'll be small, but I
hate looking at code that references "FrameStatusCBImpl" and then not being able
to figure out what file it's defined in.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/media/cma/ba...
File chromecast/media/cma/base/decrypt_config_impl.h (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/media/cma/ba...
chromecast/media/cma/base/decrypt_config_impl.h:16: DecryptConfigImpl(const
std::string& key_id,
WDYT about having the ctor instead be
DecryptConfigImpl(::media::DecryptConfig*), and just proxying all the getters?
Then our code can continue to use the ::media::DecryptConfig primitives as much
as possible.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/media/cma/pi...
File chromecast/media/cma/pipeline/av_pipeline_impl.cc (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/media/cma/pi...
chromecast/media/cma/pipeline/av_pipeline_impl.cc:155: buffered_time_ =
TimeDelta::Invalid();
To me, this re-affirms not having our own TimeDelta class. I think we'll be
better off continuing to use the ::media primitives here and uint64_t's
otherwise.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/media/cma/pi...
File chromecast/media/cma/pipeline/media_pipeline_impl.cc (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/media/cma/pi...
chromecast/media/cma/pipeline/media_pipeline_impl.cc:74:
scoped_ptr<MediaPipelineBackend> media_device_components) {
media_device_components --> media_pipeline_backend ?

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
File chromecast/public/media/DEPS (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/DEPS:3: "-chromecast",
These rules should all be guaranteed by chromecast/public/DEPS already. No need
to duplicate.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/DEPS:4: # TODO(halliwell) ... figure out whether to use
relative include path
did you figure it out? :)

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
File chromecast/public/media/decoder_buffer.h (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/decoder_buffer.h:23: class DecoderBuffer {
Any chance we can change the name, to avoid confusion with media::DecoderBuffer?

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
File chromecast/public/media/decrypt_config.h (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/decrypt_config.h:1: // Copyright (c) 2015 The Chromium
Authors. All rights reserved.
remove (c)

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/decrypt_config.h:25: SubsampleEntry() : clear_bytes(0),
cypher_bytes(0) {}
Do we need both ctors, or in practice do we only use one? (I realize this is a
copy from media/base/).

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/decrypt_config.h:37: virtual const std::string& key_id()
const = 0;
I'm a little wary of providing references here rather than copies, but will only
cast_shell be providing implementations of DecryptConfig to the backend and not
the other way around? Might be OK in that case.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
File chromecast/public/media/video_pipeline_device.h (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/video_pipeline_device.h:38: // Implementation takes
ownership of |client|.
Is there guidance on when |client| should be deleted?

Can we instead control ownership and delete on pipeline teardown to avoid a
small, not-very-noticeable memory leak in case OEM implementation doesn't delete
it?

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/time_...
File chromecast/public/time_delta.h (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/time_...
chromecast/public/time_delta.h:1: // Copyright (c) 2015 The Chromium Authors.
All rights reserved.
no (c)

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/time_...
chromecast/public/time_delta.h:14: class TimeDelta {
Could we just use |uint64_t microseconds| everywhere that we currently pass
TimeDelta to the backend?

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/time_...
chromecast/public/time_delta.h:51: TimeDelta operator+(TimeDelta other) const {
Do we need/use the addition/subtraction operators?

[byungchul, 2015-07-27 18:22:23]

https://codereview.chromium.org/1257013003/diff/20001/chromecast/base/task_ru...
File chromecast/base/task_runner_impl.cc (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/base/task_ru...
chromecast/base/task_runner_impl.cc:16: :
runner_(base::ThreadTaskRunnerHandle::Get()) {}
DCHECK(runner_.get());

https://codereview.chromium.org/1257013003/diff/20001/chromecast/base/task_ru...
chromecast/base/task_runner_impl.cc:18: TaskRunnerImpl::~TaskRunnerImpl() {}
wrap line after {

https://codereview.chromium.org/1257013003/diff/20001/chromecast/base/task_ru...
File chromecast/base/task_runner_impl.h (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/base/task_ru...
chromecast/base/task_runner_impl.h:29:
scoped_refptr<base::SingleThreadTaskRunner> runner_;
DISALLOW_..

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
File chromecast/public/media/DEPS (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/DEPS:10: ]
chromecast/public dir will be distributed to oem partners. Please add a comment
that this file must be excluded from that.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
File chromecast/public/media/audio_pipeline_device.h (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/audio_pipeline_device.h:17: AudioPipelineDevice() {}
not necessary?

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
File chromecast/public/media/decoder_buffer.h (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/decoder_buffer.h:12: #include
"chromecast/public/time_delta.h"
should be relative to chromecast/public

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/decoder_buffer.h:25: DecoderBuffer() {}
not necessary

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/decoder_buffer.h:30: virtual StreamId stream_id() const
= 0;
Isn't const in public apis too restricted?

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/decoder_buffer.h:36: virtual void
set_timestamp(TimeDelta timestamp) = 0;
No simple setters/getters in public apis. Otherwise, a simple struct?

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
File chromecast/public/media/decoder_config.h (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/decoder_config.h:101: is_encrypted(false) {}
does chorimum allow this inline ctor? It looks a complex struct.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/decoder_config.h:136: is_encrypted(false) {}
ditto

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
File chromecast/public/media/decrypt_config.h (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/decrypt_config.h:37: virtual const std::string& key_id()
const = 0;
On 2015/07/27 17:14:49, gunsch wrote:
> I'm a little wary of providing references here rather than copies, but will
only
> cast_shell be providing implementations of DecryptConfig to the backend and
not
> the other way around? Might be OK in that case.

Make it as a simple struct?

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
File chromecast/public/media/media_clock_device.h (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/media_clock_device.h:10: #include
"chromecast/public/time_delta.h"
relative to chromecast/public

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/media_clock_device.h:74: }
Why are these 2 files defined here inline? Is it allowed for chromium? Are they
really necessary for shlibs?

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
File chromecast/public/media/media_component_device.h (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/media_component_device.h:121: }
Are they necessary for shlibs?

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
File chromecast/public/media/media_pipeline_backend.h (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/media_pipeline_backend.h:24: MediaPipelineBackend() {}
not necessary

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
File chromecast/public/media/media_pipeline_device_params.h (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/media_pipeline_device_params.h:8: #include
"chromecast/public/task_runner.h"
relative to chromecast/public

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/media_pipeline_device_params.h:13: class
MediaPipelineDeviceParams {
struct

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/media_pipeline_device_params.h:32:
~MediaPipelineDeviceParams() {}
not necessary

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/media_pipeline_device_params.h:35: TaskRunner* const
task_runner;
Comment about what task_runner means.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
File chromecast/public/media/video_pipeline_device.h (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/video_pipeline_device.h:34: VideoPipelineDevice() {}
not necessary

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/task_...
File chromecast/public/task_runner.h (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/task_...
chromecast/public/task_runner.h:28: virtual bool PostTask(Task* task) = 0;
Not necessary. Let's reduce # of public APIs.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/time_...
File chromecast/public/time_delta.h (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/time_...
chromecast/public/time_delta.h:14: class TimeDelta {
On 2015/07/27 17:14:50, gunsch wrote:
> Could we just use |uint64_t microseconds| everywhere that we currently pass
> TimeDelta to the backend?

+1

[servolk, 2015-07-27 21:25:47]

High level nit: it's probably too late for that, but it would've been much
easier to review if this was split in a few separate CLs (e.g. a separate CL for
getting rid of base::TimeDelta usage everywhere, a separate CL switching to the
new TaskRunner implementation everywhere, a separate CL for switching to the new
default backend model, etc).

https://codereview.chromium.org/1257013003/diff/20001/chromecast/base/task_ru...
File chromecast/base/task_runner_impl.cc (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/base/task_ru...
chromecast/base/task_runner_impl.cc:25: return runner_->PostTask(FROM_HERE,
Nit: this will make it look like all posttasks are posted from here. Can we pass
FROM_HERE as a parameter from the call site to preserve the actual PostTask
origin?

https://codereview.chromium.org/1257013003/diff/20001/chromecast/base/task_ru...
chromecast/base/task_runner_impl.cc:30: return
runner_->PostDelayedTask(FROM_HERE,
Same

https://codereview.chromium.org/1257013003/diff/20001/chromecast/media/base/m...
File chromecast/media/base/media_codec_support.cc (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/media/base/m...
chromecast/media/base/media_codec_support.cc:12: // TODO(gunsch/servolk): delete
this definition once a solution exists upstream.
On 2015/07/27 17:14:49, gunsch wrote:
> You might be able to resolve this TODO now. Internal implementation shouldn't
be
> calling net::DefaultIsCodecSupported after your changes.

It's true, internal implementations won't call this anymore. But the problem is
that DefaultIsCodecSupported still doesn't exist in the upstream. So we'll still
need to provide our own implementation for upstream cast_shell builds.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/media/cma/ba...
File chromecast/media/cma/base/backend_client_callbacks.cc (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/media/cma/ba...
chromecast/media/cma/base/backend_client_callbacks.cc:23: if
(!eos_cb_.is_null()) {
Strictly speaking you shouldn't be accessing eos_cb_ from a different thread. So
the null check has to be done once we are on the right thread, rather than
before that.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/media/cma/ba...
chromecast/media/cma/base/backend_client_callbacks.cc:39: if
(!size_change_cb_.is_null()) {
ditto

https://codereview.chromium.org/1257013003/diff/20001/chromecast/media/cma/ba...
chromecast/media/cma/base/backend_client_callbacks.cc:53: if (!cb_.is_null()) {
ditto

https://codereview.chromium.org/1257013003/diff/20001/chromecast/media/cma/ba...
File chromecast/media/cma/base/decoder_buffer_adapter.cc (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/media/cma/ba...
chromecast/media/cma/base/decoder_buffer_adapter.cc:55: const
::media::DecryptConfig* config = buffer_->decrypt_config();
Nit: this conversion code should probably be in DecryptConfigImpl instead of
inlined here. I.e. I'd introduce something like
DecryptConfigImpl::CreateFromDecryptConfig.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/media/cma/pi...
File chromecast/media/cma/pipeline/av_pipeline_impl.cc (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/media/cma/pi...
chromecast/media/cma/pipeline/av_pipeline_impl.cc:155: buffered_time_ =
TimeDelta::Invalid();
On 2015/07/27 17:14:49, gunsch wrote:
> To me, this re-affirms not having our own TimeDelta class. I think we'll be
> better off continuing to use the ::media primitives here and uint64_t's
> otherwise.

If our goal is to fully decouple CMA backend code from Chromium dependencies,
then we don't have much of a choice. Although personally, I agree, I think
having to duplicate this functionality is very unfortunate. Perhaps we could
keep dependency at least on Chromium base/? Wasn't there some effort to make
libbase more separate and easily shareable/packageable for use by third-party
devs?

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
File chromecast/public/media/audio_pipeline_device.h (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/audio_pipeline_device.h:18: ~AudioPipelineDevice()
override {}
Nit: Have you tried building this on clang? AFAIR clang doesn't like virtual
inline methods and constructors/destructors of non-trivial objects.

[halliwell, 2015-07-28 02:19:36]

Most comments addressed in patchset 3.

Sergey - would have loved to split this into smaller CLs but decided not to
because of the cost of updating backend implementations.  Probably could have
split things into patchsets on the review but yes, it's too late now.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/base/task_ru...
File chromecast/base/task_runner_impl.cc (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/base/task_ru...
chromecast/base/task_runner_impl.cc:16: :
runner_(base::ThreadTaskRunnerHandle::Get()) {}
On 2015/07/27 18:22:22, byungchul wrote:
> DCHECK(runner_.get());

Done.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/base/task_ru...
chromecast/base/task_runner_impl.cc:18: TaskRunnerImpl::~TaskRunnerImpl() {}
On 2015/07/27 18:22:22, byungchul wrote:
> wrap line after {

clang format prefers it this way

https://codereview.chromium.org/1257013003/diff/20001/chromecast/base/task_ru...
chromecast/base/task_runner_impl.cc:25: return runner_->PostTask(FROM_HERE,
On 2015/07/27 21:25:47, servolk wrote:
> Nit: this will make it look like all posttasks are posted from here. Can we
pass
> FROM_HERE as a parameter from the call site to preserve the actual PostTask
> origin?

Good point.  the downside to passing the info through is we need to introduce
another chromecast/public type to hold that info.  I'm not sure how much it's
worth it, wdyt?  It's not super hard if this information is going to be very
useful.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/base/task_ru...
chromecast/base/task_runner_impl.cc:30: return
runner_->PostDelayedTask(FROM_HERE,
On 2015/07/27 21:25:47, servolk wrote:
> Same

Same, need to introduce a new type.  Let me know what you think.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/base/task_ru...
File chromecast/base/task_runner_impl.h (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/base/task_ru...
chromecast/base/task_runner_impl.h:29:
scoped_refptr<base::SingleThreadTaskRunner> runner_;
On 2015/07/27 18:22:23, byungchul wrote:
> DISALLOW_..

Done.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/media/base/m...
File chromecast/media/base/media_codec_support.cc (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/media/base/m...
chromecast/media/base/media_codec_support.cc:12: // TODO(gunsch/servolk): delete
this definition once a solution exists upstream.
On 2015/07/27 21:25:47, servolk wrote:
> On 2015/07/27 17:14:49, gunsch wrote:
> > You might be able to resolve this TODO now. Internal implementation
shouldn't
> be
> > calling net::DefaultIsCodecSupported after your changes.
> 
> It's true, internal implementations won't call this anymore. But the problem
is
> that DefaultIsCodecSupported still doesn't exist in the upstream. So we'll
still
> need to provide our own implementation for upstream cast_shell builds.

I could remove this TODO by just returning true lower down in this file.  And
we'd replace with a downstream [Local] change to call DefaultIsCodecSupported. 
Alternatively, we remove this stub implementation downstream.

Either way, the TODO in the header will still be there (to define
net::IsCodecSupportedCB).

Let me know if you'd prefer any changes to what's here.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/media/cma/ba...
File chromecast/media/cma/base/backend_client_callbacks.cc (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/media/cma/ba...
chromecast/media/cma/base/backend_client_callbacks.cc:23: if
(!eos_cb_.is_null()) {
On 2015/07/27 21:25:47, servolk wrote:
> Strictly speaking you shouldn't be accessing eos_cb_ from a different thread.
So
> the null check has to be done once we are on the right thread, rather than
> before that.

Done.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/media/cma/ba...
chromecast/media/cma/base/backend_client_callbacks.cc:39: if
(!size_change_cb_.is_null()) {
On 2015/07/27 21:25:47, servolk wrote:
> ditto

Done.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/media/cma/ba...
chromecast/media/cma/base/backend_client_callbacks.cc:53: if (!cb_.is_null()) {
On 2015/07/27 21:25:47, servolk wrote:
> ditto

Done.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/media/cma/ba...
File chromecast/media/cma/base/backend_client_callbacks.h (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/media/cma/ba...
chromecast/media/cma/base/backend_client_callbacks.h:5: #ifndef
CHROMECAST_MEDIA_CMA_BASE_BACKEND_CLIENT_CALLBACKS_H_
On 2015/07/27 17:14:49, gunsch wrote:
> Let's put these somewhere a little higher level. Maybe cma/pipeline/ ? They're
> not appropriate for use in all parts of CMA.

Done.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/media/cma/ba...
chromecast/media/cma/base/backend_client_callbacks.h:23: class
MediaComponentDeviceClientImpl : public MediaComponentDevice::Client {
On 2015/07/27 17:14:49, gunsch wrote:
> file nit: can we split these into three files? I know they'll be small, but I
> hate looking at code that references "FrameStatusCBImpl" and then not being
able
> to figure out what file it's defined in.

Yes I hate that too ... was being lazy here ... done :)

https://codereview.chromium.org/1257013003/diff/20001/chromecast/media/cma/ba...
File chromecast/media/cma/base/decoder_buffer_adapter.cc (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/media/cma/ba...
chromecast/media/cma/base/decoder_buffer_adapter.cc:55: const
::media::DecryptConfig* config = buffer_->decrypt_config();
On 2015/07/27 21:25:47, servolk wrote:
> Nit: this conversion code should probably be in DecryptConfigImpl instead of
> inlined here. I.e. I'd introduce something like
> DecryptConfigImpl::CreateFromDecryptConfig.

Indeed.  Originally I was trying to not have DecryptConfigImpl and put
everything in public headers, but it was too much inlining for Chromium rules :)
 Moved into DecryptConfigImpl now.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/media/cma/ba...
File chromecast/media/cma/base/decrypt_config_impl.h (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/media/cma/ba...
chromecast/media/cma/base/decrypt_config_impl.h:16: DecryptConfigImpl(const
std::string& key_id,
On 2015/07/27 17:14:49, gunsch wrote:
> WDYT about having the ctor instead be
> DecryptConfigImpl(::media::DecryptConfig*), and just proxying all the getters?
> Then our code can continue to use the ::media::DecryptConfig primitives as
much
> as possible.

I've made a constructor that takes that, in response to Sergey's comment
elsewhere.  However, we have to copy the subsamples because they're a different
type.  We could proxy key_id and iv I guess, but I think it's a bit simpler just
copying, it means you don't have to reason about lifetime of the
::media::DecryptConfig at all.  Also, DecryptConfigMarshaller can construct this
object directly, it seems unnecessary to construct a temporary
::media::DecryptConfig just to wrap it in this.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/media/cma/pi...
File chromecast/media/cma/pipeline/av_pipeline_impl.cc (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/media/cma/pi...
chromecast/media/cma/pipeline/av_pipeline_impl.cc:155: buffered_time_ =
TimeDelta::Invalid();
On 2015/07/27 21:25:47, servolk wrote:
> On 2015/07/27 17:14:49, gunsch wrote:
> > To me, this re-affirms not having our own TimeDelta class. I think we'll be
> > better off continuing to use the ::media primitives here and uint64_t's
> > otherwise.
> 
> If our goal is to fully decouple CMA backend code from Chromium dependencies,
> then we don't have much of a choice. Although personally, I agree, I think
> having to duplicate this functionality is very unfortunate. Perhaps we could
> keep dependency at least on Chromium base/? Wasn't there some effort to make
> libbase more separate and easily shareable/packageable for use by third-party
> devs?

I've removed TimeDelta.  It made initial refactoring easier/more mechanical but
you're right it doesn't add much value.  And in fact most backend code seems to
work with raw integers anyway, so the code actually gets simpler after passing
integers through.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/media/cma/pi...
File chromecast/media/cma/pipeline/media_pipeline_impl.cc (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/media/cma/pi...
chromecast/media/cma/pipeline/media_pipeline_impl.cc:74:
scoped_ptr<MediaPipelineBackend> media_device_components) {
On 2015/07/27 17:14:49, gunsch wrote:
> media_device_components --> media_pipeline_backend ?

Done.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
File chromecast/public/media/DEPS (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/DEPS:3: "-chromecast",
On 2015/07/27 17:14:49, gunsch wrote:
> These rules should all be guaranteed by chromecast/public/DEPS already. No
need
> to duplicate.

got it.  removing the file anyway though.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/DEPS:4: # TODO(halliwell) ... figure out whether to use
relative include path
On 2015/07/27 17:14:49, gunsch wrote:
> did you figure it out? :)

I did now.  I discovered that:
* Up a level, headers were able to #include "chromecast_export.h" by virtue of
it being in the same directory (the rule is the including file's directory gets
searched first before the include paths)
* You can't use that fact here to #include "../chromecast_export.h" because DEPS
doesn't allow it (and the DEPS checker crashes if you put ".." in it!)

So after discussion with Byungchul, it seems like we should have
chromecast/public added to include paths for our targets.  I did this by adding
to target_defaults in chromecast.gyp and media.gyp ... not sure if we should
have something more targeted, but it seems like errors could pop up at any time
if we don't add it to all targets?

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/DEPS:10: ]
On 2015/07/27 18:22:23, byungchul wrote:
> chromecast/public dir will be distributed to oem partners. Please add a
comment
> that this file must be excluded from that.

File deleted anyway.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
File chromecast/public/media/audio_pipeline_device.h (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/audio_pipeline_device.h:17: AudioPipelineDevice() {}
On 2015/07/27 18:22:23, byungchul wrote:
> not necessary?

removed.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/audio_pipeline_device.h:18: ~AudioPipelineDevice()
override {}
On 2015/07/27 21:25:47, servolk wrote:
> Nit: Have you tried building this on clang? AFAIR clang doesn't like virtual
> inline methods and constructors/destructors of non-trivial objects.

Yes, it builds fine (and meets presubmit checks).

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
File chromecast/public/media/decoder_buffer.h (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/decoder_buffer.h:12: #include
"chromecast/public/time_delta.h"
On 2015/07/27 18:22:23, byungchul wrote:
> should be relative to chromecast/public

TimeDelta removed now anyway.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/decoder_buffer.h:23: class DecoderBuffer {
On 2015/07/27 17:14:49, gunsch wrote:
> Any chance we can change the name, to avoid confusion with
media::DecoderBuffer?

Happy to change the name.  DecryptConfig is a similar case.  What do you think
of CastDecoderBuffer and CastDecryptConfig?  I think there's a benefit to using
a prefix rather than a completely different name, because while we want to avoid
confusion, it's also useful to see at a glance that they're the same thing (one
just a wrapper around the other).

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/decoder_buffer.h:25: DecoderBuffer() {}
On 2015/07/27 18:22:23, byungchul wrote:
> not necessary

removed.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/decoder_buffer.h:30: virtual StreamId stream_id() const
= 0;
On 2015/07/27 18:22:23, byungchul wrote:
> Isn't const in public apis too restricted?

This isn't an API for vendors to implement.  It's just to wrap DecoderBufferBase
without leaking Chromium types into public API.  So there will only be 1
implementation and this is its signature.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/decoder_buffer.h:36: virtual void
set_timestamp(TimeDelta timestamp) = 0;
On 2015/07/27 18:22:23, byungchul wrote:
> No simple setters/getters in public apis. Otherwise, a simple struct?

I removed this and also writable_data.  I had been just duplicating the
DecoderBufferBase API but of course vendors don't need access to these parts.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
File chromecast/public/media/decoder_config.h (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/decoder_config.h:101: is_encrypted(false) {}
On 2015/07/27 18:22:23, byungchul wrote:
> does chorimum allow this inline ctor? It looks a complex struct.

it does.  This has been in for a while now.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/decoder_config.h:136: is_encrypted(false) {}
On 2015/07/27 18:22:23, byungchul wrote:
> ditto

ditto! :)

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
File chromecast/public/media/decrypt_config.h (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/decrypt_config.h:1: // Copyright (c) 2015 The Chromium
Authors. All rights reserved.
On 2015/07/27 17:14:49, gunsch wrote:
> remove (c)

Done.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/decrypt_config.h:25: SubsampleEntry() : clear_bytes(0),
cypher_bytes(0) {}
On 2015/07/27 17:14:49, gunsch wrote:
> Do we need both ctors, or in practice do we only use one? (I realize this is a
> copy from media/base/).

we need both - obviously we only use the uint32_t, uint32_t version to copy the
data across, but since it's in a std::vector, a no-arg ctor is required.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/decrypt_config.h:37: virtual const std::string& key_id()
const = 0;
On 2015/07/27 17:14:49, gunsch wrote:
> I'm a little wary of providing references here rather than copies, but will
only
> cast_shell be providing implementations of DecryptConfig to the backend and
not
> the other way around? Might be OK in that case.

correct, it's us implementing this, not vendors.  We're just wrapping the
media::DecryptConfig API.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
File chromecast/public/media/media_clock_device.h (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/media_clock_device.h:10: #include
"chromecast/public/time_delta.h"
On 2015/07/27 18:22:23, byungchul wrote:
> relative to chromecast/public

TimeDelta removed now anyway.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/media_clock_device.h:74: }
On 2015/07/27 18:22:23, byungchul wrote:
> Why are these 2 files defined here inline? Is it allowed for chromium? Are
they
> really necessary for shlibs?

Yes, it's allowed.  And all vendors call these functions.  Easiest way was to
just inline them for now.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
File chromecast/public/media/media_component_device.h (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/media_component_device.h:121: }
On 2015/07/27 18:22:23, byungchul wrote:
> Are they necessary for shlibs?

yes, all vendors rely on these currently.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
File chromecast/public/media/media_pipeline_backend.h (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/media_pipeline_backend.h:24: MediaPipelineBackend() {}
On 2015/07/27 18:22:23, byungchul wrote:
> not necessary

removed.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
File chromecast/public/media/media_pipeline_device_params.h (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/media_pipeline_device_params.h:8: #include
"chromecast/public/task_runner.h"
On 2015/07/27 18:22:23, byungchul wrote:
> relative to chromecast/public

done - as mentioned elsewhere, this required adding chromecast/public to target
include dirs

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/media_pipeline_device_params.h:13: class
MediaPipelineDeviceParams {
On 2015/07/27 18:22:23, byungchul wrote:
> struct

Done.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/media_pipeline_device_params.h:32:
~MediaPipelineDeviceParams() {}
On 2015/07/27 18:22:23, byungchul wrote:
> not necessary

removed.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/media_pipeline_device_params.h:35: TaskRunner* const
task_runner;
On 2015/07/27 18:22:23, byungchul wrote:
> Comment about what task_runner means.

Done.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
File chromecast/public/media/video_pipeline_device.h (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/video_pipeline_device.h:34: VideoPipelineDevice() {}
On 2015/07/27 18:22:23, byungchul wrote:
> not necessary

removed.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/video_pipeline_device.h:38: // Implementation takes
ownership of |client|.
On 2015/07/27 17:14:49, gunsch wrote:
> Is there guidance on when |client| should be deleted?
> 
> Can we instead control ownership and delete on pipeline teardown to avoid a
> small, not-very-noticeable memory leak in case OEM implementation doesn't
delete
> it?

Indeed, pipeline now owns, simplifies this for OEM.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/task_...
File chromecast/public/task_runner.h (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/task_...
chromecast/public/task_runner.h:28: virtual bool PostTask(Task* task) = 0;
On 2015/07/27 18:22:23, byungchul wrote:
> Not necessary. Let's reduce # of public APIs.

Done.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/time_...
File chromecast/public/time_delta.h (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/time_...
chromecast/public/time_delta.h:1: // Copyright (c) 2015 The Chromium Authors.
All rights reserved.
On 2015/07/27 17:14:49, gunsch wrote:
> no (c)

file removed.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/time_...
chromecast/public/time_delta.h:14: class TimeDelta {
On 2015/07/27 18:22:23, byungchul wrote:
> On 2015/07/27 17:14:50, gunsch wrote:
> > Could we just use |uint64_t microseconds| everywhere that we currently pass
> > TimeDelta to the backend?
> 
> +1

Done.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/time_...
chromecast/public/time_delta.h:51: TimeDelta operator+(TimeDelta other) const {
On 2015/07/27 17:14:49, gunsch wrote:
> Do we need/use the addition/subtraction operators?

we did use them (and no others, that's why I didn't stray into multiplication!).
 Anyway, pipeline code is back to using base::TimeDelta and passing int64_t to
backend.

[byungchul, 2015-07-28 18:05:44]

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
File chromecast/public/media/decoder_buffer.h (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/decoder_buffer.h:36: virtual void
set_timestamp(TimeDelta timestamp) = 0;
On 2015/07/28 02:19:35, halliwell wrote:
> On 2015/07/27 18:22:23, byungchul wrote:
> > No simple setters/getters in public apis. Otherwise, a simple struct?
> 
> I removed this and also writable_data.  I had been just duplicating the
> DecoderBufferBase API but of course vendors don't need access to these parts.

If it is not for vendors, why is this defined in public?

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
File chromecast/public/media/media_clock_device.h (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/media_clock_device.h:74: }
On 2015/07/28 02:19:36, halliwell wrote:
> On 2015/07/27 18:22:23, byungchul wrote:
> > Why are these 2 files defined here inline? Is it allowed for chromium? Are
> they
> > really necessary for shlibs?
> 
> Yes, it's allowed.  And all vendors call these functions.  Easiest way was to
> just inline them for now.

I think
1) StateToString() is not necessary to be in public apis. It should be in each
implementations.
3) Who changes state?
3) how can we force vendors to call IsValidStateTransition() whenever state is
changed? Why is it correct? For example, the state cannot be changed from
uninitialized to running, but it can be changed uninitialized -> idle ->
running. If the caller missed the change of uninitialized -> idle, it is same to
the change of uinitialized -> running.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
File chromecast/public/media/media_component_device.h (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/media_component_device.h:121: }
On 2015/07/28 02:19:36, halliwell wrote:
> On 2015/07/27 18:22:23, byungchul wrote:
> > Are they necessary for shlibs?
> 
> yes, all vendors rely on these currently.

See the other comment in media_clock_device.h

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
File chromecast/public/media/video_pipeline_device.h (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/video_pipeline_device.h:38: // Implementation takes
ownership of |client|.
On 2015/07/28 02:19:36, halliwell wrote:
> On 2015/07/27 17:14:49, gunsch wrote:
> > Is there guidance on when |client| should be deleted?
> > 
> > Can we instead control ownership and delete on pipeline teardown to avoid a
> > small, not-very-noticeable memory leak in case OEM implementation doesn't
> delete
> > it?
> 
> Indeed, pipeline now owns, simplifies this for OEM.

It would be confusing for vendros if we mix the ownership of objects. TashRunner
takes the ownership of tasks. I am fine with that exception, but please check
others.

[servolk, 2015-07-28 18:12:55]

https://codereview.chromium.org/1257013003/diff/20001/chromecast/base/task_ru...
File chromecast/base/task_runner_impl.cc (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/base/task_ru...
chromecast/base/task_runner_impl.cc:25: return runner_->PostTask(FROM_HERE,
On 2015/07/28 02:19:35, halliwell wrote:
> On 2015/07/27 21:25:47, servolk wrote:
> > Nit: this will make it look like all posttasks are posted from here. Can we
> pass
> > FROM_HERE as a parameter from the call site to preserve the actual PostTask
> > origin?
> 
> Good point.  the downside to passing the info through is we need to introduce
> another chromecast/public type to hold that info.  I'm not sure how much it's
> worth it, wdyt?  It's not super hard if this information is going to be very
> useful.

TBH I don't remember if I ever used this info directly, so it's not strictly
necessary. But it would be still nice to fix this, probably some analysis
tool(s) depend on this. Maybe create a tracking P1 bug/TODO for this and leave
as is for now?

[halliwell, 2015-07-28 23:26:06]

Patchset 4 addresses all remaining comments, afaik, except for question of
VideoClient ownership.

Also updated GetRenderingDelay API (feature request from kmackay, makes sense to
include while I'm in here anyway).

PTAL.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/base/task_ru...
File chromecast/base/task_runner_impl.cc (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/base/task_ru...
chromecast/base/task_runner_impl.cc:25: return runner_->PostTask(FROM_HERE,
On 2015/07/28 18:12:55, servolk wrote:
> On 2015/07/28 02:19:35, halliwell wrote:
> > On 2015/07/27 21:25:47, servolk wrote:
> > > Nit: this will make it look like all posttasks are posted from here. Can
we
> > pass
> > > FROM_HERE as a parameter from the call site to preserve the actual
PostTask
> > > origin?
> > 
> > Good point.  the downside to passing the info through is we need to
introduce
> > another chromecast/public type to hold that info.  I'm not sure how much
it's
> > worth it, wdyt?  It's not super hard if this information is going to be very
> > useful.
> 
> TBH I don't remember if I ever used this info directly, so it's not strictly
> necessary. But it would be still nice to fix this, probably some analysis
> tool(s) depend on this. Maybe create a tracking P1 bug/TODO for this and leave
> as is for now?

Ok, TODO and internal tracking bug filed.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
File chromecast/public/media/decoder_buffer.h (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/decoder_buffer.h:23: class DecoderBuffer {
On 2015/07/28 02:19:35, halliwell wrote:
> On 2015/07/27 17:14:49, gunsch wrote:
> > Any chance we can change the name, to avoid confusion with
> media::DecoderBuffer?
> 
> Happy to change the name.  DecryptConfig is a similar case.  What do you think
> of CastDecoderBuffer and CastDecryptConfig?  I think there's a benefit to
using
> a prefix rather than a completely different name, because while we want to
avoid
> confusion, it's also useful to see at a glance that they're the same thing
(one
> just a wrapper around the other).

Ok, renamed to CastDecoderBuffer, CastDecryptConfig (and implementations are
CastDecoderBufferImpl, CastDecryptConfigImpl).

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/decoder_buffer.h:36: virtual void
set_timestamp(TimeDelta timestamp) = 0;
On 2015/07/28 18:05:43, byungchul wrote:
> On 2015/07/28 02:19:35, halliwell wrote:
> > On 2015/07/27 18:22:23, byungchul wrote:
> > > No simple setters/getters in public apis. Otherwise, a simple struct?
> > 
> > I removed this and also writable_data.  I had been just duplicating the
> > DecoderBufferBase API but of course vendors don't need access to these
parts.
> 
> If it is not for vendors, why is this defined in public?

We need to pass buffers to vendors, this is their API for accessing buffer data.
 They just don't need the setters, so I removed them in the later patchset.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
File chromecast/public/media/media_clock_device.h (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/media_clock_device.h:74: }
On 2015/07/28 18:05:43, byungchul wrote:
> On 2015/07/28 02:19:36, halliwell wrote:
> > On 2015/07/27 18:22:23, byungchul wrote:
> > > Why are these 2 files defined here inline? Is it allowed for chromium? Are
> > they
> > > really necessary for shlibs?
> > 
> > Yes, it's allowed.  And all vendors call these functions.  Easiest way was
to
> > just inline them for now.
> 
> I think
> 1) StateToString() is not necessary to be in public apis. It should be in each
> implementations.
> 3) Who changes state?
> 3) how can we force vendors to call IsValidStateTransition() whenever state is
> changed? Why is it correct? For example, the state cannot be changed from
> uninitialized to running, but it can be changed uninitialized -> idle ->
> running. If the caller missed the change of uninitialized -> idle, it is same
to
> the change of uinitialized -> running.

StateToString now moved out into implementation code.

I moved IsValidStateTransition into backends, also used it for an assertion in
the default implementation, I don't think we should allow invalid state
transitions as something vendors are expected to support.

I think the state machine is still awkward for vendors to implement, lcwu would
like us to look at simplifying the interface later on, but not for this release.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
File chromecast/public/media/media_component_device.h (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/media_component_device.h:121: }
On 2015/07/28 18:05:43, byungchul wrote:
> On 2015/07/28 02:19:36, halliwell wrote:
> > On 2015/07/27 18:22:23, byungchul wrote:
> > > Are they necessary for shlibs?
> > 
> > yes, all vendors rely on these currently.
> 
> See the other comment in media_clock_device.h

As for clock, removed from public headers.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
File chromecast/public/media/video_pipeline_device.h (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/video_pipeline_device.h:38: // Implementation takes
ownership of |client|.
On 2015/07/28 18:05:43, byungchul wrote:
> On 2015/07/28 02:19:36, halliwell wrote:
> > On 2015/07/27 17:14:49, gunsch wrote:
> > > Is there guidance on when |client| should be deleted?
> > > 
> > > Can we instead control ownership and delete on pipeline teardown to avoid
a
> > > small, not-very-noticeable memory leak in case OEM implementation doesn't
> > delete
> > > it?
> > 
> > Indeed, pipeline now owns, simplifies this for OEM.
> 
> It would be confusing for vendros if we mix the ownership of objects.
TashRunner
> takes the ownership of tasks. I am fine with that exception, but please check
> others.

Yeah I'm considering going back to vendor ownership here.  The FrameStatus
callback has to be owned by vendors because there's a new one per frame pushed
in.  So it feels more consistent to have them own all callbacks.

When we implement conformance tests, we can pass in a checking implementation
that ensures vendors delete this object on pipeline teardown.

Will wait for gunsch's thoughts on this since it was his suggestion originally.

[servolk, 2015-07-29 01:46:26]

https://codereview.chromium.org/1257013003/diff/20001/chromecast/base/task_ru...
File chromecast/base/task_runner_impl.cc (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/base/task_ru...
chromecast/base/task_runner_impl.cc:25: return runner_->PostTask(FROM_HERE,
On 2015/07/28 23:26:05, halliwell wrote:
> On 2015/07/28 18:12:55, servolk wrote:
> > On 2015/07/28 02:19:35, halliwell wrote:
> > > On 2015/07/27 21:25:47, servolk wrote:
> > > > Nit: this will make it look like all posttasks are posted from here. Can
> we
> > > pass
> > > > FROM_HERE as a parameter from the call site to preserve the actual
> PostTask
> > > > origin?
> > > 
> > > Good point.  the downside to passing the info through is we need to
> introduce
> > > another chromecast/public type to hold that info.  I'm not sure how much
> it's
> > > worth it, wdyt?  It's not super hard if this information is going to be
very
> > > useful.
> > 
> > TBH I don't remember if I ever used this info directly, so it's not strictly
> > necessary. But it would be still nice to fix this, probably some analysis
> > tool(s) depend on this. Maybe create a tracking P1 bug/TODO for this and
leave
> > as is for now?
> 
> Ok, TODO and internal tracking bug filed.

Wait, I meant a TODO comment in code, e.g. //TODO(haliwell): Fix FROM_HERE
But I don't see anything like that in task_runner_impl.* or task_runner.h.
Also if you are keeping the version which allows specifying delay, let's keep
the name PostDelayedTask for consistency.

[halliwell, 2015-07-29 01:55:54]

https://codereview.chromium.org/1257013003/diff/20001/chromecast/base/task_ru...
File chromecast/base/task_runner_impl.cc (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/base/task_ru...
chromecast/base/task_runner_impl.cc:25: return runner_->PostTask(FROM_HERE,
On 2015/07/29 01:46:26, servolk wrote:
> On 2015/07/28 23:26:05, halliwell wrote:
> > On 2015/07/28 18:12:55, servolk wrote:
> > > On 2015/07/28 02:19:35, halliwell wrote:
> > > > On 2015/07/27 21:25:47, servolk wrote:
> > > > > Nit: this will make it look like all posttasks are posted from here.
Can
> > we
> > > > pass
> > > > > FROM_HERE as a parameter from the call site to preserve the actual
> > PostTask
> > > > > origin?
> > > > 
> > > > Good point.  the downside to passing the info through is we need to
> > introduce
> > > > another chromecast/public type to hold that info.  I'm not sure how much
> > it's
> > > > worth it, wdyt?  It's not super hard if this information is going to be
> very
> > > > useful.
> > > 
> > > TBH I don't remember if I ever used this info directly, so it's not
strictly
> > > necessary. But it would be still nice to fix this, probably some analysis
> > > tool(s) depend on this. Maybe create a tracking P1 bug/TODO for this and
> leave
> > > as is for now?
> > 
> > Ok, TODO and internal tracking bug filed.
> 
> Wait, I meant a TODO comment in code, e.g. //TODO(haliwell): Fix FROM_HERE
> But I don't see anything like that in task_runner_impl.* or task_runner.h.
> Also if you are keeping the version which allows specifying delay, let's keep
> the name PostDelayedTask for consistency.

The TODO is in task_runner_impl.cc.  Not sure PostDelayedTask is accurate, given
it's frequently used to post tasks without delay now (delay = 0) after merging
them.  We should view the API from vendor's perspective, they don't care about
the existence of PostDelayedTask in chromium.

[gunsch, 2015-07-29 17:04:20]

Focused on nit-picking the public API this round.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/media/base/m...
File chromecast/media/base/media_codec_support.cc (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/media/base/m...
chromecast/media/base/media_codec_support.cc:12: // TODO(gunsch/servolk): delete
this definition once a solution exists upstream.
On 2015/07/28 02:19:35, halliwell wrote:
> On 2015/07/27 21:25:47, servolk wrote:
> > On 2015/07/27 17:14:49, gunsch wrote:
> > > You might be able to resolve this TODO now. Internal implementation
> shouldn't
> > be
> > > calling net::DefaultIsCodecSupported after your changes.
> > 
> > It's true, internal implementations won't call this anymore. But the problem
> is
> > that DefaultIsCodecSupported still doesn't exist in the upstream. So we'll
> still
> > need to provide our own implementation for upstream cast_shell builds.
> 
> I could remove this TODO by just returning true lower down in this file.  And
> we'd replace with a downstream [Local] change to call DefaultIsCodecSupported.

> Alternatively, we remove this stub implementation downstream.
> 
> Either way, the TODO in the header will still be there (to define
> net::IsCodecSupportedCB).
> 
> Let me know if you'd prefer any changes to what's here.

Yeah, go ahead and delete it. We shouldn't be defining things in the net
namespace :)

The only reason we checked it in was because internal code expected it in the
chromium/src repo (due to our downstream hack), so to keep building we at least
needed a definition here. Since you're moving that out of internal, I'd rather
just "return true" below with a TODO.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
File chromecast/public/media/video_pipeline_device.h (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/video_pipeline_device.h:38: // Implementation takes
ownership of |client|.
On 2015/07/28 23:26:06, halliwell wrote:
> On 2015/07/28 18:05:43, byungchul wrote:
> > On 2015/07/28 02:19:36, halliwell wrote:
> > > On 2015/07/27 17:14:49, gunsch wrote:
> > > > Is there guidance on when |client| should be deleted?
> > > > 
> > > > Can we instead control ownership and delete on pipeline teardown to
avoid
> a
> > > > small, not-very-noticeable memory leak in case OEM implementation
doesn't
> > > delete
> > > > it?
> > > 
> > > Indeed, pipeline now owns, simplifies this for OEM.
> > 
> > It would be confusing for vendros if we mix the ownership of objects.
> TashRunner
> > takes the ownership of tasks. I am fine with that exception, but please
check
> > others.
> 
> Yeah I'm considering going back to vendor ownership here.  The FrameStatus
> callback has to be owned by vendors because there's a new one per frame pushed
> in.  So it feels more consistent to have them own all callbacks.
> 
> When we implement conformance tests, we can pass in a checking implementation
> that ensures vendors delete this object on pipeline teardown.
> 
> Will wait for gunsch's thoughts on this since it was his suggestion
originally.

I like the idea of a conformance test guaranteeing lifetime. I don't think we
have anything that does that today. How hard would it be to add a small test
case for that in this CL (or as a quick follow-up CL?). I'd be much more okay
passing ownership to the shlib in that case.

https://codereview.chromium.org/1257013003/diff/60001/chromecast/base/task_ru...
File chromecast/base/task_runner_impl.cc (right):

https://codereview.chromium.org/1257013003/diff/60001/chromecast/base/task_ru...
chromecast/base/task_runner_impl.cc:1: // Copyright (c) 2015 The Chromium
Authors. All rights reserved.
no (c)

https://codereview.chromium.org/1257013003/diff/60001/chromecast/base/task_ru...
File chromecast/base/task_runner_impl.h (right):

https://codereview.chromium.org/1257013003/diff/60001/chromecast/base/task_ru...
chromecast/base/task_runner_impl.h:1: // Copyright (c) 2015 The Chromium
Authors. All rights reserved.
no (c)

https://codereview.chromium.org/1257013003/diff/60001/chromecast/public/media...
File chromecast/public/media/cast_decoder_buffer.h (right):

https://codereview.chromium.org/1257013003/diff/60001/chromecast/public/media...
chromecast/public/media/cast_decoder_buffer.h:19: // DecoderBuffer exposes only
the properties of an audio/video buffer.
Can we rewrite this description to be more a description for the OEM of what
this class is and how it's meant to be used, rather than implementation details?

https://codereview.chromium.org/1257013003/diff/60001/chromecast/public/media...
chromecast/public/media/cast_decoder_buffer.h:27: // and default value is
kPrimary.
Can we make this not optional, and simply return kPrimary in the "default" case,
instead of putting that on the OEM?

https://codereview.chromium.org/1257013003/diff/60001/chromecast/public/media...
chromecast/public/media/cast_decoder_buffer.h:40: // Returns NULL if the buffer
has no decrypt info.
Can we strengthen this statement to "Returns NULL iff the buffer is not
encrypted" ?

https://codereview.chromium.org/1257013003/diff/60001/chromecast/public/media...
File chromecast/public/media/cast_decrypt_config.h (right):

https://codereview.chromium.org/1257013003/diff/60001/chromecast/public/media...
chromecast/public/media/cast_decrypt_config.h:32: // Contains all information
that a decryptor needs to decrypt a media sample.
"that a decryptor needs" --> "needed"

and probably "all information" --> "metadata", since we don't actually include
the key :)

https://codereview.chromium.org/1257013003/diff/60001/chromecast/public/task_...
File chromecast/public/task_runner.h (right):

https://codereview.chromium.org/1257013003/diff/60001/chromecast/public/task_...
chromecast/public/task_runner.h:28: virtual bool PostTask(Task* task, uint64_t
delay_milliseconds) = 0;
Maybe document that:
a) delay_milliseconds must be >= 0 (and potentially DCHECK that)
b) task will always be added to the thread's task queue, and 0ms does not
necessarily mean executing immediately

[halliwell, 2015-07-29 19:58:20]

https://codereview.chromium.org/1257013003/diff/20001/chromecast/media/base/m...
File chromecast/media/base/media_codec_support.cc (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/media/base/m...
chromecast/media/base/media_codec_support.cc:12: // TODO(gunsch/servolk): delete
this definition once a solution exists upstream.
On 2015/07/29 17:04:19, gunsch wrote:
> On 2015/07/28 02:19:35, halliwell wrote:
> > On 2015/07/27 21:25:47, servolk wrote:
> > > On 2015/07/27 17:14:49, gunsch wrote:
> > > > You might be able to resolve this TODO now. Internal implementation
> > shouldn't
> > > be
> > > > calling net::DefaultIsCodecSupported after your changes.
> > > 
> > > It's true, internal implementations won't call this anymore. But the
problem
> > is
> > > that DefaultIsCodecSupported still doesn't exist in the upstream. So we'll
> > still
> > > need to provide our own implementation for upstream cast_shell builds.
> > 
> > I could remove this TODO by just returning true lower down in this file. 
And
> > we'd replace with a downstream [Local] change to call
DefaultIsCodecSupported.
> 
> > Alternatively, we remove this stub implementation downstream.
> > 
> > Either way, the TODO in the header will still be there (to define
> > net::IsCodecSupportedCB).
> > 
> > Let me know if you'd prefer any changes to what's here.
> 
> Yeah, go ahead and delete it. We shouldn't be defining things in the net
> namespace :)
> 
> The only reason we checked it in was because internal code expected it in the
> chromium/src repo (due to our downstream hack), so to keep building we at
least
> needed a definition here. Since you're moving that out of internal, I'd rather
> just "return true" below with a TODO.

Ok, removed.

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
File chromecast/public/media/video_pipeline_device.h (right):

https://codereview.chromium.org/1257013003/diff/20001/chromecast/public/media...
chromecast/public/media/video_pipeline_device.h:38: // Implementation takes
ownership of |client|.
On 2015/07/29 17:04:20, gunsch wrote:
> On 2015/07/28 23:26:06, halliwell wrote:
> > On 2015/07/28 18:05:43, byungchul wrote:
> > > On 2015/07/28 02:19:36, halliwell wrote:
> > > > On 2015/07/27 17:14:49, gunsch wrote:
> > > > > Is there guidance on when |client| should be deleted?
> > > > > 
> > > > > Can we instead control ownership and delete on pipeline teardown to
> avoid
> > a
> > > > > small, not-very-noticeable memory leak in case OEM implementation
> doesn't
> > > > delete
> > > > > it?
> > > > 
> > > > Indeed, pipeline now owns, simplifies this for OEM.
> > > 
> > > It would be confusing for vendros if we mix the ownership of objects.
> > TashRunner
> > > takes the ownership of tasks. I am fine with that exception, but please
> check
> > > others.
> > 
> > Yeah I'm considering going back to vendor ownership here.  The FrameStatus
> > callback has to be owned by vendors because there's a new one per frame
pushed
> > in.  So it feels more consistent to have them own all callbacks.
> > 
> > When we implement conformance tests, we can pass in a checking
implementation
> > that ensures vendors delete this object on pipeline teardown.
> > 
> > Will wait for gunsch's thoughts on this since it was his suggestion
> originally.
> 
> I like the idea of a conformance test guaranteeing lifetime. I don't think we
> have anything that does that today. How hard would it be to add a small test
> case for that in this CL (or as a quick follow-up CL?). I'd be much more okay
> passing ownership to the shlib in that case.

Don't think it would be too hard, but would prefer follow-up CL purely because
of the time constraints.  I've filed b/22822861 to track internally ...
generalised to cover ownership of all the objects that we pass into CMA backend
now.

So have reverted to backend owning VideoClient.

https://codereview.chromium.org/1257013003/diff/60001/chromecast/base/task_ru...
File chromecast/base/task_runner_impl.cc (right):

https://codereview.chromium.org/1257013003/diff/60001/chromecast/base/task_ru...
chromecast/base/task_runner_impl.cc:1: // Copyright (c) 2015 The Chromium
Authors. All rights reserved.
On 2015/07/29 17:04:20, gunsch wrote:
> no (c)

Done.

https://codereview.chromium.org/1257013003/diff/60001/chromecast/base/task_ru...
File chromecast/base/task_runner_impl.h (right):

https://codereview.chromium.org/1257013003/diff/60001/chromecast/base/task_ru...
chromecast/base/task_runner_impl.h:1: // Copyright (c) 2015 The Chromium
Authors. All rights reserved.
On 2015/07/29 17:04:20, gunsch wrote:
> no (c)

Done.

https://codereview.chromium.org/1257013003/diff/60001/chromecast/public/media...
File chromecast/public/media/cast_decoder_buffer.h (right):

https://codereview.chromium.org/1257013003/diff/60001/chromecast/public/media...
chromecast/public/media/cast_decoder_buffer.h:19: // DecoderBuffer exposes only
the properties of an audio/video buffer.
On 2015/07/29 17:04:20, gunsch wrote:
> Can we rewrite this description to be more a description for the OEM of what
> this class is and how it's meant to be used, rather than implementation
details?

Done.  I took a swing at updating a number of other comments too, several were
phrased from the POV of API clients, not implementers.

https://codereview.chromium.org/1257013003/diff/60001/chromecast/public/media...
chromecast/public/media/cast_decoder_buffer.h:27: // and default value is
kPrimary.
On 2015/07/29 17:04:20, gunsch wrote:
> Can we make this not optional, and simply return kPrimary in the "default"
case,
> instead of putting that on the OEM?

I'm not quite sure what the intent of the "it's optional" comment was ... was it
just trying to say that in a single-stream case, the stream id is always
kPrimary?  Simplified now ... let me know if I missed something :)

https://codereview.chromium.org/1257013003/diff/60001/chromecast/public/media...
chromecast/public/media/cast_decoder_buffer.h:40: // Returns NULL if the buffer
has no decrypt info.
On 2015/07/29 17:04:20, gunsch wrote:
> Can we strengthen this statement to "Returns NULL iff the buffer is not
> encrypted" ?

We can, comment updated.

https://codereview.chromium.org/1257013003/diff/60001/chromecast/public/media...
File chromecast/public/media/cast_decrypt_config.h (right):

https://codereview.chromium.org/1257013003/diff/60001/chromecast/public/media...
chromecast/public/media/cast_decrypt_config.h:32: // Contains all information
that a decryptor needs to decrypt a media sample.
On 2015/07/29 17:04:20, gunsch wrote:
> "that a decryptor needs" --> "needed"
> 
> and probably "all information" --> "metadata", since we don't actually include
> the key :)

Done.

https://codereview.chromium.org/1257013003/diff/60001/chromecast/public/task_...
File chromecast/public/task_runner.h (right):

https://codereview.chromium.org/1257013003/diff/60001/chromecast/public/task_...
chromecast/public/task_runner.h:28: virtual bool PostTask(Task* task, uint64_t
delay_milliseconds) = 0;
On 2015/07/29 17:04:20, gunsch wrote:
> Maybe document that:
> a) delay_milliseconds must be >= 0 (and potentially DCHECK that)
> b) task will always be added to the thread's task queue, and 0ms does not
> necessarily mean executing immediately

it's a uint, so no need to check >= 0.
(b) = done.

[gunsch-google, 2015-07-29 20:06:53]

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

[gunsch-google, 2015-07-29 20:06:54]

LGTM, thanks for the extra iterating on the public API docs.

[byungchul, 2015-07-29 20:32:23]

https://codereview.chromium.org/1257013003/diff/80001/chromecast/public/media...
File chromecast/public/media/cast_decoder_buffer.h (right):

https://codereview.chromium.org/1257013003/diff/80001/chromecast/public/media...
chromecast/public/media/cast_decoder_buffer.h:26: virtual ~CastDecoderBuffer()
{}
If it won't be destroyed by shlib, it should be a protected dtor.

https://codereview.chromium.org/1257013003/diff/80001/chromecast/public/media...
chromecast/public/media/cast_decoder_buffer.h:29: virtual StreamId stream_id()
const = 0;
Still don't understand why these names are lower-case, and then why not a simple
struct. What's the meaning of lower-case function names here?

https://codereview.chromium.org/1257013003/diff/80001/chromecast/public/media...
File chromecast/public/media/cast_decrypt_config.h (right):

https://codereview.chromium.org/1257013003/diff/80001/chromecast/public/media...
chromecast/public/media/cast_decrypt_config.h:35: virtual ~CastDecryptConfig()
{}
ditto

https://codereview.chromium.org/1257013003/diff/80001/chromecast/public/media...
File chromecast/public/media/media_component_device.h (right):

https://codereview.chromium.org/1257013003/diff/80001/chromecast/public/media...
chromecast/public/media/media_component_device.h:19: //                      
-------------- kRunning <---
Use + at the point the line is changed.

----+
    |
    v

https://codereview.chromium.org/1257013003/diff/80001/chromecast/public/media...
chromecast/public/media/media_component_device.h:117: // |completion_cb| is
owned by the implementation and must be freed, even in
s/freed/deleted or destroyed?

https://codereview.chromium.org/1257013003/diff/80001/chromecast/public/media...
File chromecast/public/media/media_pipeline_device_params.h (right):

https://codereview.chromium.org/1257013003/diff/80001/chromecast/public/media...
chromecast/public/media/media_pipeline_device_params.h:33: MediaSyncType
sync_type;
const?

https://codereview.chromium.org/1257013003/diff/80001/chromecast/public/task_...
File chromecast/public/task_runner.h (right):

https://codereview.chromium.org/1257013003/diff/80001/chromecast/public/task_...
chromecast/public/task_runner.h:30: virtual bool BelongsToCurrentThread() = 0;
Is it necessary?

[byungchul, 2015-07-29 20:34:09]

https://codereview.chromium.org/1257013003/diff/80001/chromecast/base/task_ru...
File chromecast/base/task_runner_impl.h (right):

https://codereview.chromium.org/1257013003/diff/80001/chromecast/base/task_ru...
chromecast/base/task_runner_impl.h:29:
scoped_refptr<base::SingleThreadTaskRunner> runner_;
const

[gunsch-google, 2015-07-29 20:48:26]

https://codereview.chromium.org/1257013003/diff/80001/chromecast/public/media...
File chromecast/public/media/cast_decoder_buffer.h (right):

https://codereview.chromium.org/1257013003/diff/80001/chromecast/public/media...
chromecast/public/media/cast_decoder_buffer.h:26: virtual ~CastDecoderBuffer()
{}
On 2015/07/29 20:32:23, byungchul wrote:
> If it won't be destroyed by shlib, it should be a protected dtor.

This will be owned and destroyed by shlib.

https://codereview.chromium.org/1257013003/diff/80001/chromecast/public/media...
chromecast/public/media/cast_decoder_buffer.h:29: virtual StreamId stream_id()
const = 0;
On 2015/07/29 20:32:23, byungchul wrote:
> Still don't understand why these names are lower-case, and then why not a
simple
> struct. What's the meaning of lower-case function names here?

I'm in favor of getters. For example, data() and decrypt_config() might not be
owned by this class, but this class might just indirectly point to the
references elsewhere (which it does). So to avoid copying and keep lifetime
management reasonable, they need getters. At that point, simple struct is not a
good choice.

[gunsch, 2015-07-29 20:48:49]

Oops, just noticed I was reviewing from the wrong account. lgtm from this one :)

[gunsch, 2015-07-29 20:49:01]

gunsch@chromium.org changed reviewers:
- gunsch@google.com

[halliwell, 2015-07-29 22:39:27]

https://codereview.chromium.org/1257013003/diff/80001/chromecast/public/media...
File chromecast/public/media/cast_decoder_buffer.h (right):

https://codereview.chromium.org/1257013003/diff/80001/chromecast/public/media...
chromecast/public/media/cast_decoder_buffer.h:26: virtual ~CastDecoderBuffer()
{}
On 2015/07/29 20:48:26, gunsch-google wrote:
> On 2015/07/29 20:32:23, byungchul wrote:
> > If it won't be destroyed by shlib, it should be a protected dtor.
> 
> This will be owned and destroyed by shlib.

Correct.  This definitely needs to be public.

https://codereview.chromium.org/1257013003/diff/80001/chromecast/public/media...
chromecast/public/media/cast_decoder_buffer.h:29: virtual StreamId stream_id()
const = 0;
On 2015/07/29 20:48:26, gunsch-google wrote:
> On 2015/07/29 20:32:23, byungchul wrote:
> > Still don't understand why these names are lower-case, and then why not a
> simple
> > struct. What's the meaning of lower-case function names here?
> 
> I'm in favor of getters. For example, data() and decrypt_config() might not be
> owned by this class, but this class might just indirectly point to the
> references elsewhere (which it does). So to avoid copying and keep lifetime
> management reasonable, they need getters. At that point, simple struct is not
a
> good choice.

After offline discussion, prefer to leave these as-is.  We definitely need
getter functions here to avoid copying data.  Whether to CamelCase their names
seems debatable and I'd rather spend remaining time on converting other backend
implementations, let me know if you strongly object though :)

https://codereview.chromium.org/1257013003/diff/80001/chromecast/public/media...
File chromecast/public/media/cast_decrypt_config.h (right):

https://codereview.chromium.org/1257013003/diff/80001/chromecast/public/media...
chromecast/public/media/cast_decrypt_config.h:35: virtual ~CastDecryptConfig()
{}
On 2015/07/29 20:32:23, byungchul wrote:
> ditto

This is needed, similar to the other case.

https://codereview.chromium.org/1257013003/diff/80001/chromecast/public/media...
File chromecast/public/media/media_component_device.h (right):

https://codereview.chromium.org/1257013003/diff/80001/chromecast/public/media...
chromecast/public/media/media_component_device.h:19: //                      
-------------- kRunning <---
On 2015/07/29 20:32:23, byungchul wrote:
> Use + at the point the line is changed.
> 
> ----+
>     |
>     v

Done.

https://codereview.chromium.org/1257013003/diff/80001/chromecast/public/media...
chromecast/public/media/media_component_device.h:117: // |completion_cb| is
owned by the implementation and must be freed, even in
On 2015/07/29 20:32:23, byungchul wrote:
> s/freed/deleted or destroyed?

Done.

https://codereview.chromium.org/1257013003/diff/80001/chromecast/public/media...
File chromecast/public/media/media_pipeline_device_params.h (right):

https://codereview.chromium.org/1257013003/diff/80001/chromecast/public/media...
chromecast/public/media/media_pipeline_device_params.h:33: MediaSyncType
sync_type;
On 2015/07/29 20:32:23, byungchul wrote:
> const?

Done (and added another constructor).

https://codereview.chromium.org/1257013003/diff/80001/chromecast/public/task_...
File chromecast/public/task_runner.h (right):

https://codereview.chromium.org/1257013003/diff/80001/chromecast/public/task_...
chromecast/public/task_runner.h:30: virtual bool BelongsToCurrentThread() = 0;
On 2015/07/29 20:32:23, byungchul wrote:
> Is it necessary?

It's useful, and used: for the standard "if (!belongs to current thread) { post
}" pattern

[byungchul, 2015-07-29 23:12:30]

https://codereview.chromium.org/1257013003/diff/80001/chromecast/public/media...
File chromecast/public/media/cast_decoder_buffer.h (right):

https://codereview.chromium.org/1257013003/diff/80001/chromecast/public/media...
chromecast/public/media/cast_decoder_buffer.h:29: virtual StreamId stream_id()
const = 0;
On 2015/07/29 22:39:27, halliwell wrote:
> On 2015/07/29 20:48:26, gunsch-google wrote:
> > On 2015/07/29 20:32:23, byungchul wrote:
> > > Still don't understand why these names are lower-case, and then why not a
> > simple
> > > struct. What's the meaning of lower-case function names here?
> > 
> > I'm in favor of getters. For example, data() and decrypt_config() might not
be
> > owned by this class, but this class might just indirectly point to the
> > references elsewhere (which it does). So to avoid copying and keep lifetime
> > management reasonable, they need getters. At that point, simple struct is
not
> a
> > good choice.
> 
> After offline discussion, prefer to leave these as-is.  We definitely need
> getter functions here to avoid copying data.  Whether to CamelCase their names
> seems debatable and I'd rather spend remaining time on converting other
backend
> implementations, let me know if you strongly object though :)

A TODO would be appreciated to not forget re-address naming later :)

https://codereview.chromium.org/1257013003/diff/80001/chromecast/public/task_...
File chromecast/public/task_runner.h (right):

https://codereview.chromium.org/1257013003/diff/80001/chromecast/public/task_...
chromecast/public/task_runner.h:30: virtual bool BelongsToCurrentThread() = 0;
On 2015/07/29 22:39:27, halliwell wrote:
> On 2015/07/29 20:32:23, byungchul wrote:
> > Is it necessary?
> 
> It's useful, and used: for the standard "if (!belongs to current thread) {
post
> }" pattern

Shouldn't the pattern, "if (!belongs to current thread) { post
}" be used in TaskRunnerImpl?

https://codereview.chromium.org/1257013003/diff/100001/chromecast/public/medi...
File chromecast/public/media/media_component_device.h (right):

https://codereview.chromium.org/1257013003/diff/100001/chromecast/public/medi...
chromecast/public/media/media_component_device.h:22: // kUninitialized <-->
kIdle  --------------        |
   |
---+ here
   |

[halliwell, 2015-07-30 00:19:57]

https://codereview.chromium.org/1257013003/diff/80001/chromecast/public/media...
File chromecast/public/media/cast_decoder_buffer.h (right):

https://codereview.chromium.org/1257013003/diff/80001/chromecast/public/media...
chromecast/public/media/cast_decoder_buffer.h:29: virtual StreamId stream_id()
const = 0;
On 2015/07/29 23:12:29, byungchul wrote:
> On 2015/07/29 22:39:27, halliwell wrote:
> > On 2015/07/29 20:48:26, gunsch-google wrote:
> > > On 2015/07/29 20:32:23, byungchul wrote:
> > > > Still don't understand why these names are lower-case, and then why not
a
> > > simple
> > > > struct. What's the meaning of lower-case function names here?
> > > 
> > > I'm in favor of getters. For example, data() and decrypt_config() might
not
> be
> > > owned by this class, but this class might just indirectly point to the
> > > references elsewhere (which it does). So to avoid copying and keep
lifetime
> > > management reasonable, they need getters. At that point, simple struct is
> not
> > a
> > > good choice.
> > 
> > After offline discussion, prefer to leave these as-is.  We definitely need
> > getter functions here to avoid copying data.  Whether to CamelCase their
names
> > seems debatable and I'd rather spend remaining time on converting other
> backend
> > implementations, let me know if you strongly object though :)
> 
> A TODO would be appreciated to not forget re-address naming later :)

Ok, TODO added :)

https://codereview.chromium.org/1257013003/diff/80001/chromecast/public/task_...
File chromecast/public/task_runner.h (right):

https://codereview.chromium.org/1257013003/diff/80001/chromecast/public/task_...
chromecast/public/task_runner.h:30: virtual bool BelongsToCurrentThread() = 0;
On 2015/07/29 23:12:30, byungchul wrote:
> On 2015/07/29 22:39:27, halliwell wrote:
> > On 2015/07/29 20:32:23, byungchul wrote:
> > > Is it necessary?
> > 
> > It's useful, and used: for the standard "if (!belongs to current thread) {
> post
> > }" pattern
> 
> Shouldn't the pattern, "if (!belongs to current thread) { post
> }" be used in TaskRunnerImpl?

That's fair.  I removed it from the API here.  TaskRunnerImpl will run (and
delete) the task immediately if it's already on the right thread and if delay ==
0.

https://codereview.chromium.org/1257013003/diff/100001/chromecast/public/medi...
File chromecast/public/media/media_component_device.h (right):

https://codereview.chromium.org/1257013003/diff/100001/chromecast/public/medi...
chromecast/public/media/media_component_device.h:22: // kUninitialized <-->
kIdle  --------------        |
On 2015/07/29 23:12:30, byungchul wrote:
>    |
> ---+ here
>    |

Done.

[byungchul, 2015-07-30 00:32:30]

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

https://codereview.chromium.org/1257013003/diff/120001/chromecast/base/task_r...
chromecast/base/task_runner_impl.cc:23: // vendor backends to send the callsite
info.
move this TODO inside else clause.

https://codereview.chromium.org/1257013003/diff/120001/chromecast/base/task_r...
chromecast/base/task_runner_impl.cc:24: if (delay_milliseconds == 0 &&
runner_->BelongsToCurrentThread()) {
Would it make sense to run task if milliseconds < 0, and to post task always if
0? Vendor can pass 1, but PostTask(0) is always scheduled first before any
delayed tasks. Or, revert this change? (sorry).

https://codereview.chromium.org/1257013003/diff/120001/chromecast/base/task_r...
chromecast/base/task_runner_impl.cc:28: } else {
not else after return.

https://codereview.chromium.org/1257013003/diff/120001/chromecast/public/task...
File chromecast/public/task_runner.h (right):

https://codereview.chromium.org/1257013003/diff/120001/chromecast/public/task...
chromecast/public/task_runner.h:30: // mean the task will run immediately, just
as soon as it can be.
Comment that if it is called on same thread and delay is 0, task will be
executed immediately, i.e. before this function returns.

[halliwell, 2015-07-30 03:08:11]

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

https://codereview.chromium.org/1257013003/diff/120001/chromecast/base/task_r...
chromecast/base/task_runner_impl.cc:23: // vendor backends to send the callsite
info.
On 2015/07/30 00:32:30, byungchul wrote:
> move this TODO inside else clause.

Done.

https://codereview.chromium.org/1257013003/diff/120001/chromecast/base/task_r...
chromecast/base/task_runner_impl.cc:24: if (delay_milliseconds == 0 &&
runner_->BelongsToCurrentThread()) {
On 2015/07/30 00:32:30, byungchul wrote:
> Would it make sense to run task if milliseconds < 0, and to post task always
if
> 0? Vendor can pass 1, but PostTask(0) is always scheduled first before any
> delayed tasks. Or, revert this change? (sorry).

ms is uint, so always >= 0.  I think this change is ok for vendors ... seems
exactly what you'd expect that PostTask(0) would run before delayed tasks.

https://codereview.chromium.org/1257013003/diff/120001/chromecast/base/task_r...
chromecast/base/task_runner_impl.cc:28: } else {
On 2015/07/30 00:32:30, byungchul wrote:
> not else after return.

Done.

https://codereview.chromium.org/1257013003/diff/120001/chromecast/public/task...
File chromecast/public/task_runner.h (right):

https://codereview.chromium.org/1257013003/diff/120001/chromecast/public/task...
chromecast/public/task_runner.h:30: // mean the task will run immediately, just
as soon as it can be.
On 2015/07/30 00:32:30, byungchul wrote:
> Comment that if it is called on same thread and delay is 0, task will be
> executed immediately, i.e. before this function returns.

Done.

[halliwell, 2015-07-30 03:36:35]

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

[commit-bot: I haz the power, 2015-07-30 03:37:04]

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

[commit-bot: I haz the power, 2015-07-30 03:39:23]

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

[commit-bot: I haz the power, 2015-07-30 03:39:24]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[halliwell, 2015-07-30 03:50:49]

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

[commit-bot: I haz the power, 2015-07-30 03:51:12]

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

[byungchul, 2015-07-30 05:22:54]

lgtm

A nit though

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

https://codereview.chromium.org/1257013003/diff/120001/chromecast/base/task_r...
chromecast/base/task_runner_impl.cc:24: if (delay_milliseconds == 0 &&
runner_->BelongsToCurrentThread()) {
On 2015/07/30 03:08:10, halliwell wrote:
> On 2015/07/30 00:32:30, byungchul wrote:
> > Would it make sense to run task if milliseconds < 0, and to post task always
> if
> > 0? Vendor can pass 1, but PostTask(0) is always scheduled first before any
> > delayed tasks. Or, revert this change? (sorry).
> 
> ms is uint, so always >= 0.  I think this change is ok for vendors ... seems
> exactly what you'd expect that PostTask(0) would run before delayed tasks.

My point was, vendor cannot do PostTask(0) on same thread. Vendor may to
PostTask(1) for similar effect, but that is scheduled after PostTask(0) called
later by cast_shell.

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

https://codereview.chromium.org/1257013003/diff/140001/chromecast/base/task_r...
chromecast/base/task_runner_impl.cc:21: bool TaskRunnerImpl::PostTask(Task*
task, uint64_t delay_milliseconds) {
DCHECK(task);

[halliwell, 2015-07-30 05:40:19]

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

https://codereview.chromium.org/1257013003/diff/120001/chromecast/base/task_r...
chromecast/base/task_runner_impl.cc:24: if (delay_milliseconds == 0 &&
runner_->BelongsToCurrentThread()) {
On 2015/07/30 05:22:53, byungchul wrote:
> On 2015/07/30 03:08:10, halliwell wrote:
> > On 2015/07/30 00:32:30, byungchul wrote:
> > > Would it make sense to run task if milliseconds < 0, and to post task
always
> > if
> > > 0? Vendor can pass 1, but PostTask(0) is always scheduled first before any
> > > delayed tasks. Or, revert this change? (sorry).
> > 
> > ms is uint, so always >= 0.  I think this change is ok for vendors ... seems
> > exactly what you'd expect that PostTask(0) would run before delayed tasks.
> 
> My point was, vendor cannot do PostTask(0) on same thread. Vendor may to
> PostTask(1) for similar effect, but that is scheduled after PostTask(0) called
> later by cast_shell.

Yes, agreed ... but I think that's ok.  Current and intended vendor usage isn't
so critically dependent on ordering issues.

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

https://codereview.chromium.org/1257013003/diff/140001/chromecast/base/task_r...
chromecast/base/task_runner_impl.cc:21: bool TaskRunnerImpl::PostTask(Task*
task, uint64_t delay_milliseconds) {
On 2015/07/30 05:22:54, byungchul wrote:
> DCHECK(task);

Done.

[byungchul, 2015-07-30 17:19:07]

https://codereview.chromium.org/1257013003/diff/180001/chromecast/public/task...
File chromecast/public/task_runner.h (right):

https://codereview.chromium.org/1257013003/diff/180001/chromecast/public/task...
chromecast/public/task_runner.h:27: virtual ~TaskRunner() {}
Should it be protected since it won't be destroyed by vendor?

[servolk, 2015-07-30 17:44:18]

https://codereview.chromium.org/1257013003/diff/180001/chromecast/base/task_r...
File chromecast/base/task_runner_impl.h (right):

https://codereview.chromium.org/1257013003/diff/180001/chromecast/base/task_r...
chromecast/base/task_runner_impl.h:25: bool PostTask(Task* task, uint64_t
milliseconds) override;
Nit: please rename the second parameter to delay_milliseconds to make it easier
to understand its meaning and for consistency with the function definition in
the interface and .cc file.

https://codereview.chromium.org/1257013003/diff/180001/chromecast/public/task...
File chromecast/public/task_runner.h (right):

https://codereview.chromium.org/1257013003/diff/180001/chromecast/public/task...
chromecast/public/task_runner.h:33: virtual bool PostTask(Task* task, uint64_t
delay_milliseconds) = 0;
We are using uint64_t values to represent number of microseconds in other parts
of public API (MediaComponentDevice), so maybe let's switch to microseconds here
as well for API consistency?

[servolk, 2015-07-30 17:50:29]

https://codereview.chromium.org/1257013003/diff/180001/chromecast/public/medi...
File chromecast/public/media/media_clock_device.h (right):

https://codereview.chromium.org/1257013003/diff/180001/chromecast/public/medi...
chromecast/public/media/media_clock_device.h:57: virtual int64_t GetTime() = 0;
Now that it returns just int64_t, let's rename it into GetTimeMicroseconds to
make it clear what value is being returned.

https://codereview.chromium.org/1257013003/diff/180001/chromecast/public/medi...
File chromecast/public/media/media_component_device.h (right):

https://codereview.chromium.org/1257013003/diff/180001/chromecast/public/medi...
chromecast/public/media/media_component_device.h:85: int64_t delay;
Rename into delay_microseconds and timestamp_microseconds to make clear what
units are being used here?
I'd also suggest renaming constructor input params as well to make it clear that
these are in microseconds.

[halliwell, 2015-07-30 18:20:19]

https://codereview.chromium.org/1257013003/diff/180001/chromecast/base/task_r...
File chromecast/base/task_runner_impl.h (right):

https://codereview.chromium.org/1257013003/diff/180001/chromecast/base/task_r...
chromecast/base/task_runner_impl.h:25: bool PostTask(Task* task, uint64_t
milliseconds) override;
On 2015/07/30 17:44:18, servolk wrote:
> Nit: please rename the second parameter to delay_milliseconds to make it
easier
> to understand its meaning and for consistency with the function definition in
> the interface and .cc file.

Done.

https://codereview.chromium.org/1257013003/diff/180001/chromecast/public/medi...
File chromecast/public/media/media_clock_device.h (right):

https://codereview.chromium.org/1257013003/diff/180001/chromecast/public/medi...
chromecast/public/media/media_clock_device.h:57: virtual int64_t GetTime() = 0;
On 2015/07/30 17:50:29, servolk wrote:
> Now that it returns just int64_t, let's rename it into GetTimeMicroseconds to
> make it clear what value is being returned.

Done.

https://codereview.chromium.org/1257013003/diff/180001/chromecast/public/medi...
File chromecast/public/media/media_component_device.h (right):

https://codereview.chromium.org/1257013003/diff/180001/chromecast/public/medi...
chromecast/public/media/media_component_device.h:85: int64_t delay;
On 2015/07/30 17:50:29, servolk wrote:
> Rename into delay_microseconds and timestamp_microseconds to make clear what
> units are being used here?
> I'd also suggest renaming constructor input params as well to make it clear
that
> these are in microseconds.

Done.

https://codereview.chromium.org/1257013003/diff/180001/chromecast/public/task...
File chromecast/public/task_runner.h (right):

https://codereview.chromium.org/1257013003/diff/180001/chromecast/public/task...
chromecast/public/task_runner.h:27: virtual ~TaskRunner() {}
On 2015/07/30 17:19:06, byungchul wrote:
> Should it be protected since it won't be destroyed by vendor?

Done.

https://codereview.chromium.org/1257013003/diff/180001/chromecast/public/task...
chromecast/public/task_runner.h:33: virtual bool PostTask(Task* task, uint64_t
delay_milliseconds) = 0;
On 2015/07/30 17:44:18, servolk wrote:
> We are using uint64_t values to represent number of microseconds in other
parts
> of public API (MediaComponentDevice), so maybe let's switch to microseconds
here
> as well for API consistency?

Disagree, task posting has always been done in milliseconds and buffer
timestamps in microseconds.  Changing this would require lots of code changing
and risk of errors since there's no type checking, just numbers.

[halliwell, 2015-07-30 19:55:04]

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

[commit-bot: I haz the power, 2015-07-30 19:55:35]

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

[commit-bot: I haz the power, 2015-07-30 19:59:46]

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

[commit-bot: I haz the power, 2015-07-30 19:59:48]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)
  linux_arm_compile on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_arm_compi...)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[halliwell, 2015-07-30 20:08:03]

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

[commit-bot: I haz the power, 2015-07-30 20:08:48]

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

[commit-bot: I haz the power, 2015-07-30 20:31:08]

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

[commit-bot: I haz the power, 2015-07-30 20:31:09]

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

[halliwell, 2015-07-30 20:38:25]

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

[commit-bot: I haz the power, 2015-07-30 20:39:15]

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

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

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

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

Dry run: This issue passed the CQ dry run.

[halliwell, 2015-08-06 17:30:32]

On 2015/07/30 21:07:23, commit-bot: I haz the power wrote:
> Dry run: This issue passed the CQ dry run.

Had to re-introduce DecryptContext for the sake of one backend implementation. 
Put a base interface into chromecast/public/media and renamed the previous base
class to DecryptContextImpl.

[gunsch, 2015-08-06 18:57:23]

I re-reviewed only the DecryptContext changes, which lgtm.

[halliwell, 2015-08-07 15:49:04]

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

[commit-bot: I haz the power, 2015-08-07 15:49:16]

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

[commit-bot: I haz the power, 2015-08-07 16:21:51]

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

[commit-bot: I haz the power, 2015-08-07 16:21:52]

Dry run: This issue passed the CQ dry run.

[halliwell, 2015-08-07 17:17:23]

The CQ bit was checked by halliwell@chromium.org

[halliwell, 2015-08-07 17:17:24]

The patchset sent to the CQ was uploaded after l-g-t-m from gunsch@google.com,
byungchul@chromium.org, gunsch@chromium.org
Link to the patchset: https://codereview.chromium.org/1257013003/#ps320001
(title: "Rebase")

[commit-bot: I haz the power, 2015-08-07 17:18:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1257013003/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1257013003/320001

[commit-bot: I haz the power, 2015-08-07 17:24:19]

Committed patchset #17 (id:320001)

[commit-bot: I haz the power, 2015-08-07 17:24:53]

Patchset 17 (id:??) landed as
https://crrev.com/dad541d95f2df33f23d52dce79151291918a7a67
Cr-Commit-Position: refs/heads/master@{#342370}

[alokp, 2015-08-10 20:30:24]

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

[alokp, 2015-08-10 20:30:25]

documentation nits for cast media api.

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

https://codereview.chromium.org/1257013003/diff/320001/chromecast/public/cast...
chromecast/public/cast_media_shlib.h:43: static VideoPlane* GetVideoPlane();
Please document ownership for the returned VideoPlane object. I am assuming that
the backend owns the video plane?

https://codereview.chromium.org/1257013003/diff/320001/chromecast/public/cast...
chromecast/public/cast_media_shlib.h:48: static MediaPipelineBackend*
CreateMediaPipelineBackend(
Who owns the returned MediaPipelineBackend? I am assuming the caller, in which
case, shouldn't it return a scoped_ptr? If the motivation is to not depend on
chrome base, we should at least document the ownership or use std::unique_ptr.

[alokp, 2015-08-10 22:06:56]

https://codereview.chromium.org/1257013003/diff/320001/chromecast/media/cma/b...
File chromecast/media/cma/base/decoder_buffer_adapter.h (right):

https://codereview.chromium.org/1257013003/diff/320001/chromecast/media/cma/b...
chromecast/media/cma/base/decoder_buffer_adapter.h:22: class
DecoderBufferAdapter : public DecoderBufferBase {
Would it be better to derive DecoderBufferAdapter from CastDecoderBuffer?

With this API I have to allocate too many tiny objects when I need to pass a
media::DecoderBuffer.

media::DecoderBuffer -> DecoderBufferAdapter -> CastDecoderBufferImpl

If DecoderBufferAdapter derived from CastDecoderBuffer directly, we would save
one allocation, which may be significant since it is being done for each media
frame. We still hold a scoped_refptr to ::media::DecoderBuffer, so there is no
change to memory management.

[alokp, 2015-08-10 22:20:09]

https://codereview.chromium.org/1257013003/diff/320001/chromecast/public/medi...
File chromecast/public/media/media_component_device.h (right):

https://codereview.chromium.org/1257013003/diff/320001/chromecast/public/medi...
chromecast/public/media/media_component_device.h:58: class FrameStatusCB {
This is one more allocation per frame if I just need to wrap a base::Callback.
Can we simply use an std::function?

[gunsch, 2015-08-10 22:20:52]

https://codereview.chromium.org/1257013003/diff/320001/chromecast/public/medi...
File chromecast/public/media/media_component_device.h (right):

https://codereview.chromium.org/1257013003/diff/320001/chromecast/public/medi...
chromecast/public/media/media_component_device.h:58: class FrameStatusCB {
On 2015/08/10 22:20:09, Alok Priyadarshi wrote:
> This is one more allocation per frame if I just need to wrap a base::Callback.
> Can we simply use an std::function?

No C++11 library use.

[alokp, 2015-08-10 22:23:35]

On 2015/08/10 22:20:52, gunsch wrote:
>
https://codereview.chromium.org/1257013003/diff/320001/chromecast/public/medi...
> File chromecast/public/media/media_component_device.h (right):
> 
>
https://codereview.chromium.org/1257013003/diff/320001/chromecast/public/medi...
> chromecast/public/media/media_component_device.h:58: class FrameStatusCB {
> On 2015/08/10 22:20:09, Alok Priyadarshi wrote:
> > This is one more allocation per frame if I just need to wrap a
base::Callback.
> > Can we simply use an std::function?
> 
> No C++11 library use.

Didn't we decide that shlib interface will be exempted from this guideline?

[halliwell, 2015-08-10 22:27:48]

On 2015/08/10 22:23:35, Alok Priyadarshi wrote:
> On 2015/08/10 22:20:52, gunsch wrote:
> >
>
https://codereview.chromium.org/1257013003/diff/320001/chromecast/public/medi...
> > File chromecast/public/media/media_component_device.h (right):
> > 
> >
>
https://codereview.chromium.org/1257013003/diff/320001/chromecast/public/medi...
> > chromecast/public/media/media_component_device.h:58: class FrameStatusCB {
> > On 2015/08/10 22:20:09, Alok Priyadarshi wrote:
> > > This is one more allocation per frame if I just need to wrap a
> base::Callback.
> > > Can we simply use an std::function?
> > 
> > No C++11 library use.
> 
> Didn't we decide that shlib interface will be exempted from this guideline?

Implementations are free to use whatever they want internally, but in our APIs,
we must follow Chromium rules.

[halliwell, 2015-08-10 22:28:04]

https://codereview.chromium.org/1257013003/diff/320001/chromecast/media/cma/b...
File chromecast/media/cma/base/decoder_buffer_adapter.h (right):

https://codereview.chromium.org/1257013003/diff/320001/chromecast/media/cma/b...
chromecast/media/cma/base/decoder_buffer_adapter.h:22: class
DecoderBufferAdapter : public DecoderBufferBase {
On 2015/08/10 22:06:56, Alok Priyadarshi wrote:
> Would it be better to derive DecoderBufferAdapter from CastDecoderBuffer?
> 
> With this API I have to allocate too many tiny objects when I need to pass a
> media::DecoderBuffer.
> 
> media::DecoderBuffer -> DecoderBufferAdapter -> CastDecoderBufferImpl
> 
> If DecoderBufferAdapter derived from CastDecoderBuffer directly, we would save
> one allocation, which may be significant since it is being done for each media
> frame. We still hold a scoped_refptr to ::media::DecoderBuffer, so there is no
> change to memory management.

Not possible right now: this would in fact be a change in memory management.  We
still rely on passing a refcounted DecoderBufferBase around.

Of course, it's not an ideal design, and is something we could look at fixing in
the future.

[gunsch, 2015-08-10 22:29:15]

Not sure that we did so. I think we acknowledged that due to vendor toolchain
requirements it _should_ be supported, but we currently haven't set that
precedent by doing so yet.

[alokp, 2015-08-10 22:39:56]

On 2015/08/10 22:28:04, halliwell wrote:
>
https://codereview.chromium.org/1257013003/diff/320001/chromecast/media/cma/b...
> File chromecast/media/cma/base/decoder_buffer_adapter.h (right):
> 
>
https://codereview.chromium.org/1257013003/diff/320001/chromecast/media/cma/b...
> chromecast/media/cma/base/decoder_buffer_adapter.h:22: class
> DecoderBufferAdapter : public DecoderBufferBase {
> On 2015/08/10 22:06:56, Alok Priyadarshi wrote:
> > Would it be better to derive DecoderBufferAdapter from CastDecoderBuffer?
> > 
> > With this API I have to allocate too many tiny objects when I need to pass a
> > media::DecoderBuffer.
> > 
> > media::DecoderBuffer -> DecoderBufferAdapter -> CastDecoderBufferImpl
> > 
> > If DecoderBufferAdapter derived from CastDecoderBuffer directly, we would
save
> > one allocation, which may be significant since it is being done for each
media
> > frame. We still hold a scoped_refptr to ::media::DecoderBuffer, so there is
no
> > change to memory management.
> 
> Not possible right now: this would in fact be a change in memory management. 
We
> still rely on passing a refcounted DecoderBufferBase around.
> 
> Of course, it's not an ideal design, and is something we could look at fixing
in
> the future.

Luke: What problem do you see deriving DecoderBufferAdapter from
CastDecoderBuffer and wrapping ::media::DecoderBuffer directly? I can send a
patch and we can discuss it there.

[erickung1, 2015-08-10 22:44:12]

On 2015/08/10 22:39:56, Alok Priyadarshi wrote:
> On 2015/08/10 22:28:04, halliwell wrote:
> >
>
https://codereview.chromium.org/1257013003/diff/320001/chromecast/media/cma/b...
> > File chromecast/media/cma/base/decoder_buffer_adapter.h (right):
> > 
> >
>
https://codereview.chromium.org/1257013003/diff/320001/chromecast/media/cma/b...
> > chromecast/media/cma/base/decoder_buffer_adapter.h:22: class
> > DecoderBufferAdapter : public DecoderBufferBase {
> > On 2015/08/10 22:06:56, Alok Priyadarshi wrote:
> > > Would it be better to derive DecoderBufferAdapter from CastDecoderBuffer?
> > > 
> > > With this API I have to allocate too many tiny objects when I need to pass
a
> > > media::DecoderBuffer.
> > > 
> > > media::DecoderBuffer -> DecoderBufferAdapter -> CastDecoderBufferImpl
> > > 
> > > If DecoderBufferAdapter derived from CastDecoderBuffer directly, we would
> save
> > > one allocation, which may be significant since it is being done for each
> media
> > > frame. We still hold a scoped_refptr to ::media::DecoderBuffer, so there
is
> no
> > > change to memory management.
> > 
> > Not possible right now: this would in fact be a change in memory management.

> We
> > still rely on passing a refcounted DecoderBufferBase around.
> > 
> > Of course, it's not an ideal design, and is something we could look at
fixing
> in
> > the future.
> 
> Luke: What problem do you see deriving DecoderBufferAdapter from
> CastDecoderBuffer and wrapping ::media::DecoderBuffer directly? I can send a
> patch and we can discuss it there.

We need to do memcpy anyways through IPC to browser process
https://code.google.com/p/chromium/codesearch#chromium/src/chromecast/media/c...

The other thing we can save on the browser process side is to convery data to
CastDecoderBufferImpl so that we don't need to new this wrapper every frame when
passing to CMA backend

Do I miss something?

[halliwell, 2015-08-10 22:45:10]

On 2015/08/10 22:39:56, Alok Priyadarshi wrote:
> On 2015/08/10 22:28:04, halliwell wrote:
> >
>
https://codereview.chromium.org/1257013003/diff/320001/chromecast/media/cma/b...
> > File chromecast/media/cma/base/decoder_buffer_adapter.h (right):
> > 
> >
>
https://codereview.chromium.org/1257013003/diff/320001/chromecast/media/cma/b...
> > chromecast/media/cma/base/decoder_buffer_adapter.h:22: class
> > DecoderBufferAdapter : public DecoderBufferBase {
> > On 2015/08/10 22:06:56, Alok Priyadarshi wrote:
> > > Would it be better to derive DecoderBufferAdapter from CastDecoderBuffer?
> > > 
> > > With this API I have to allocate too many tiny objects when I need to pass
a
> > > media::DecoderBuffer.
> > > 
> > > media::DecoderBuffer -> DecoderBufferAdapter -> CastDecoderBufferImpl
> > > 
> > > If DecoderBufferAdapter derived from CastDecoderBuffer directly, we would
> save
> > > one allocation, which may be significant since it is being done for each
> media
> > > frame. We still hold a scoped_refptr to ::media::DecoderBuffer, so there
is
> no
> > > change to memory management.
> > 
> > Not possible right now: this would in fact be a change in memory management.

> We
> > still rely on passing a refcounted DecoderBufferBase around.
> > 
> > Of course, it's not an ideal design, and is something we could look at
fixing
> in
> > the future.
> 
> Luke: What problem do you see deriving DecoderBufferAdapter from
> CastDecoderBuffer and wrapping ::media::DecoderBuffer directly? I can send a
> patch and we can discuss it there.

You won't be able to pass it around the cma pipeline (which all requires
scoped_refptr<DecoderBufferBase>).  There are places where we depend on
refcounting still.

[alokp, 2015-08-10 23:12:28]

On 2015/08/10 22:45:10, halliwell wrote:
> On 2015/08/10 22:39:56, Alok Priyadarshi wrote:
> > On 2015/08/10 22:28:04, halliwell wrote:
> > >
> >
>
https://codereview.chromium.org/1257013003/diff/320001/chromecast/media/cma/b...
> > > File chromecast/media/cma/base/decoder_buffer_adapter.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1257013003/diff/320001/chromecast/media/cma/b...
> > > chromecast/media/cma/base/decoder_buffer_adapter.h:22: class
> > > DecoderBufferAdapter : public DecoderBufferBase {
> > > On 2015/08/10 22:06:56, Alok Priyadarshi wrote:
> > > > Would it be better to derive DecoderBufferAdapter from
CastDecoderBuffer?
> > > > 
> > > > With this API I have to allocate too many tiny objects when I need to
pass
> a
> > > > media::DecoderBuffer.
> > > > 
> > > > media::DecoderBuffer -> DecoderBufferAdapter -> CastDecoderBufferImpl
> > > > 
> > > > If DecoderBufferAdapter derived from CastDecoderBuffer directly, we
would
> > save
> > > > one allocation, which may be significant since it is being done for each
> > media
> > > > frame. We still hold a scoped_refptr to ::media::DecoderBuffer, so there
> is
> > no
> > > > change to memory management.
> > > 
> > > Not possible right now: this would in fact be a change in memory
management.
> 
> > We
> > > still rely on passing a refcounted DecoderBufferBase around.
> > > 
> > > Of course, it's not an ideal design, and is something we could look at
> fixing
> > in
> > > the future.
> > 
> > Luke: What problem do you see deriving DecoderBufferAdapter from
> > CastDecoderBuffer and wrapping ::media::DecoderBuffer directly? I can send a
> > patch and we can discuss it there.
> 
> You won't be able to pass it around the cma pipeline (which all requires
> scoped_refptr<DecoderBufferBase>).  There are places where we depend on
> refcounting still.

I looked at DecoderBufferBase usage with erickung@. I think the root of the
problem here is that with the creation of the new public API in
CastDecoderBuffer, the old DecoderBufferBase is redundant. Can we use
::media::DecoderBuffer or CastDecoderBuffer everywhere instead of
DecoderBufferBase? Can DecoderBufferBase be deprecated now that we have a new
API?

[halliwell, 2015-08-10 23:20:55]

On 2015/08/10 23:12:28, Alok Priyadarshi wrote:
> On 2015/08/10 22:45:10, halliwell wrote:
> > On 2015/08/10 22:39:56, Alok Priyadarshi wrote:
> > > On 2015/08/10 22:28:04, halliwell wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/1257013003/diff/320001/chromecast/media/cma/b...
> > > > File chromecast/media/cma/base/decoder_buffer_adapter.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1257013003/diff/320001/chromecast/media/cma/b...
> > > > chromecast/media/cma/base/decoder_buffer_adapter.h:22: class
> > > > DecoderBufferAdapter : public DecoderBufferBase {
> > > > On 2015/08/10 22:06:56, Alok Priyadarshi wrote:
> > > > > Would it be better to derive DecoderBufferAdapter from
> CastDecoderBuffer?
> > > > > 
> > > > > With this API I have to allocate too many tiny objects when I need to
> pass
> > a
> > > > > media::DecoderBuffer.
> > > > > 
> > > > > media::DecoderBuffer -> DecoderBufferAdapter -> CastDecoderBufferImpl
> > > > > 
> > > > > If DecoderBufferAdapter derived from CastDecoderBuffer directly, we
> would
> > > save
> > > > > one allocation, which may be significant since it is being done for
each
> > > media
> > > > > frame. We still hold a scoped_refptr to ::media::DecoderBuffer, so
there
> > is
> > > no
> > > > > change to memory management.
> > > > 
> > > > Not possible right now: this would in fact be a change in memory
> management.
> > 
> > > We
> > > > still rely on passing a refcounted DecoderBufferBase around.
> > > > 
> > > > Of course, it's not an ideal design, and is something we could look at
> > fixing
> > > in
> > > > the future.
> > > 
> > > Luke: What problem do you see deriving DecoderBufferAdapter from
> > > CastDecoderBuffer and wrapping ::media::DecoderBuffer directly? I can send
a
> > > patch and we can discuss it there.
> > 
> > You won't be able to pass it around the cma pipeline (which all requires
> > scoped_refptr<DecoderBufferBase>).  There are places where we depend on
> > refcounting still.
> 
> I looked at DecoderBufferBase usage with erickung@. I think the root of the
> problem here is that with the creation of the new public API in
> CastDecoderBuffer, the old DecoderBufferBase is redundant. Can we use
> ::media::DecoderBuffer or CastDecoderBuffer everywhere instead of
> DecoderBufferBase? Can DecoderBufferBase be deprecated now that we have a new
> API?

Like I say, I don't believe we can (at least, not without substantial changes). 
CastDecoderBuffer is not refcounted, so it's not a drop-in replacement for
DecoderBufferBase.  DecoderBufferBase is definitely not redundant.

[byungchul, 2015-08-18 00:06:36]

https://codereview.chromium.org/1257013003/diff/320001/chromecast/base/task_r...
File chromecast/base/task_runner_impl.h (right):

https://codereview.chromium.org/1257013003/diff/320001/chromecast/base/task_r...
chromecast/base/task_runner_impl.h:27: const
scoped_refptr<base::SingleThreadTaskRunner>& runner() const {
It should return scoped_refptr<base::SingleThreadTaskRunner>. Otherwise, the
caller may not increase ref-count like:

const scoped_refptr<>& runner = task_runner->runner();