Chromecast: Add stream id interface in DecoderBufferBase class

Chromecast: Add stream id interface in DecoderBufferBase class

Committed: https://crrev.com/c6491c6ac6e493d48399cb1f662ddb0bcfb03596
Cr-Commit-Position: refs/heads/master@{#331173}

[erickung1, 2015-05-21 21:04:05]

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

[gunsch, 2015-05-21 21:06:34]

https://codereview.chromium.org/1149263002/diff/1/chromecast/media/cma/base/d...
File chromecast/media/cma/base/decoder_buffer_adapter.cc (right):

https://codereview.chromium.org/1149263002/diff/1/chromecast/media/cma/base/d...
chromecast/media/cma/base/decoder_buffer_adapter.cc:18: int stream_id, const
scoped_refptr< ::media::DecoderBuffer>& buffer)
style nit (post C++11 migration): "< ::" --> "<::"

https://codereview.chromium.org/1149263002/diff/1/chromecast/media/cma/base/d...
File chromecast/media/cma/base/decoder_buffer_adapter.h (right):

https://codereview.chromium.org/1149263002/diff/1/chromecast/media/cma/base/d...
chromecast/media/cma/base/decoder_buffer_adapter.h:23: explicit
DecoderBufferAdapter(
why not just change the primary ctor instead of having two ctors?

https://codereview.chromium.org/1149263002/diff/1/chromecast/media/cma/base/d...
chromecast/media/cma/base/decoder_buffer_adapter.h:26: int type, const
scoped_refptr< ::media::DecoderBuffer>& buffer);
type --> stream_id

https://codereview.chromium.org/1149263002/diff/1/chromecast/media/cma/base/d...
chromecast/media/cma/base/decoder_buffer_adapter.h:29: int stream_id() const
override;
what values can a stream_id have?

if only 2-3 possible values, could this be an enum?

[erickung1, 2015-05-21 21:38:34]

https://codereview.chromium.org/1149263002/diff/1/chromecast/media/cma/base/d...
File chromecast/media/cma/base/decoder_buffer_adapter.cc (right):

https://codereview.chromium.org/1149263002/diff/1/chromecast/media/cma/base/d...
chromecast/media/cma/base/decoder_buffer_adapter.cc:18: int stream_id, const
scoped_refptr< ::media::DecoderBuffer>& buffer)
On 2015/05/21 21:06:34, gunsch wrote:
> style nit (post C++11 migration): "< ::" --> "<::"

Done.

https://codereview.chromium.org/1149263002/diff/1/chromecast/media/cma/base/d...
File chromecast/media/cma/base/decoder_buffer_adapter.h (right):

https://codereview.chromium.org/1149263002/diff/1/chromecast/media/cma/base/d...
chromecast/media/cma/base/decoder_buffer_adapter.h:23: explicit
DecoderBufferAdapter(
On 2015/05/21 21:06:34, gunsch wrote:
> why not just change the primary ctor instead of having two ctors?

I found out some test code and component which doesn't really require stream_id
in the decoder buffer. It makes thing odd to force all of them adding something
like new DecoderBufferAdapter(-1, buffer) in the code

But Please let me know. I don't have strong opinion on both solutions.

https://codereview.chromium.org/1149263002/diff/1/chromecast/media/cma/base/d...
chromecast/media/cma/base/decoder_buffer_adapter.h:26: int type, const
scoped_refptr< ::media::DecoderBuffer>& buffer);
On 2015/05/21 21:06:34, gunsch wrote:
> type --> stream_id

Done.

https://codereview.chromium.org/1149263002/diff/1/chromecast/media/cma/base/d...
chromecast/media/cma/base/decoder_buffer_adapter.h:29: int stream_id() const
override;
On 2015/05/21 21:06:34, gunsch wrote:
> what values can a stream_id have?
> 
> if only 2-3 possible values, could this be an enum?

we'll use two possible value.
However, I think about leaving it more flexible which can be scale later on.

[gunsch, 2015-05-21 21:45:29]

https://codereview.chromium.org/1149263002/diff/1/chromecast/media/cma/base/d...
File chromecast/media/cma/base/decoder_buffer_adapter.h (right):

https://codereview.chromium.org/1149263002/diff/1/chromecast/media/cma/base/d...
chromecast/media/cma/base/decoder_buffer_adapter.h:29: int stream_id() const
override;
On 2015/05/21 21:38:34, erickung1 wrote:
> On 2015/05/21 21:06:34, gunsch wrote:
> > what values can a stream_id have?
> > 
> > if only 2-3 possible values, could this be an enum?
> 
> we'll use two possible value.
> However, I think about leaving it more flexible which can be scale later on. 

what is inflexible about enum? you can always add more enum values...

"-1" has no inherently semantic indication as far as "stream ids" go, so it's
completely unclear to the reader what it means when seeing it in the code.
Having "-1" littered around the code seems like a pretty clear anti-pattern.

[erickung1, 2015-05-21 23:32:21]

https://codereview.chromium.org/1149263002/diff/1/chromecast/media/cma/base/d...
File chromecast/media/cma/base/decoder_buffer_adapter.h (right):

https://codereview.chromium.org/1149263002/diff/1/chromecast/media/cma/base/d...
chromecast/media/cma/base/decoder_buffer_adapter.h:29: int stream_id() const
override;
On 2015/05/21 21:45:29, gunsch wrote:
> On 2015/05/21 21:38:34, erickung1 wrote:
> > On 2015/05/21 21:06:34, gunsch wrote:
> > > what values can a stream_id have?
> > > 
> > > if only 2-3 possible values, could this be an enum?
> > 
> > we'll use two possible value.
> > However, I think about leaving it more flexible which can be scale later on.

> 
> what is inflexible about enum? you can always add more enum values...
> 
> "-1" has no inherently semantic indication as far as "stream ids" go, so it's
> completely unclear to the reader what it means when seeing it in the code.
> Having "-1" littered around the code seems like a pretty clear anti-pattern.

Make sense. I change the type to enum

[gunsch, 2015-05-22 01:45:26]

lgtm with a few nits

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

https://codereview.chromium.org/1149263002/diff/60001/chromecast/media/cma/ba...
chromecast/media/cma/base/decoder_buffer_adapter.cc:14: :
DecoderBufferAdapter(kPrimary, buffer) {
+1 for delegated constructors!

https://codereview.chromium.org/1149263002/diff/60001/chromecast/media/cma/ba...
File chromecast/media/cma/base/decoder_buffer_adapter.h (right):

https://codereview.chromium.org/1149263002/diff/60001/chromecast/media/cma/ba...
chromecast/media/cma/base/decoder_buffer_adapter.h:23: explicit
DecoderBufferAdapter(
Can you document that this uses kPrimary as the default stream_id value?

https://codereview.chromium.org/1149263002/diff/60001/chromecast/media/cma/ip...
File chromecast/media/cma/ipc_streamer/decoder_buffer_base_marshaller.cc
(right):

https://codereview.chromium.org/1149263002/diff/60001/chromecast/media/cma/ip...
chromecast/media/cma/ipc_streamer/decoder_buffer_base_marshaller.cc:41: //
Stream Id this decoder buffer belonging to.
wording: "Stream this decoder buffer belongs to."

https://codereview.chromium.org/1149263002/diff/60001/chromecast/media/cma/ip...
chromecast/media/cma/ipc_streamer/decoder_buffer_base_marshaller.cc:110: return
stream_id_;
you could just return kPrimary here and not have the member variable

[erickung1, 2015-05-22 17:33:22]

https://codereview.chromium.org/1149263002/diff/60001/chromecast/media/cma/ba...
File chromecast/media/cma/base/decoder_buffer_adapter.h (right):

https://codereview.chromium.org/1149263002/diff/60001/chromecast/media/cma/ba...
chromecast/media/cma/base/decoder_buffer_adapter.h:23: explicit
DecoderBufferAdapter(
On 2015/05/22 01:45:26, gunsch wrote:
> Can you document that this uses kPrimary as the default stream_id value?

Done.

https://codereview.chromium.org/1149263002/diff/60001/chromecast/media/cma/ip...
File chromecast/media/cma/ipc_streamer/decoder_buffer_base_marshaller.cc
(right):

https://codereview.chromium.org/1149263002/diff/60001/chromecast/media/cma/ip...
chromecast/media/cma/ipc_streamer/decoder_buffer_base_marshaller.cc:41: //
Stream Id this decoder buffer belonging to.
On 2015/05/22 01:45:26, gunsch wrote:
> wording: "Stream this decoder buffer belongs to."

Done.

https://codereview.chromium.org/1149263002/diff/60001/chromecast/media/cma/ip...
chromecast/media/cma/ipc_streamer/decoder_buffer_base_marshaller.cc:110: return
stream_id_;
On 2015/05/22 01:45:26, gunsch wrote:
> you could just return kPrimary here and not have the member variable

Although right now all code uses kPrimary, we should still honer the data  IPC
message, so that in the future we don't have to go back to make the change here
if other stream id will be used

[erickung1, 2015-05-22 17:34:02]

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

[erickung1, 2015-05-22 17:34:02]

The patchset sent to the CQ was uploaded after l-g-t-m from gunsch@chromium.org
Link to the patchset: https://codereview.chromium.org/1149263002/#ps80001
(title: "Update comment message")

[commit-bot: I haz the power, 2015-05-22 17:34:11]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1149263002/80001

[commit-bot: I haz the power, 2015-05-22 19:14:04]

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

[commit-bot: I haz the power, 2015-05-22 19:14:05]

Dry run: This issue passed the CQ dry run.

[erickung1, 2015-05-22 21:09:52]

The CQ bit was checked by erickung@chromium.org

[commit-bot: I haz the power, 2015-05-22 21:11:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1149263002/80001

[commit-bot: I haz the power, 2015-05-22 21:23:36]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-05-22 21:25:31]

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