media: Support transporting 0-byte DecoderBuffer over mojo

media: Support transporting 0-byte DecoderBuffer over mojo

BUG=663438
TEST=Added a new unitttest.

Committed: https://crrev.com/98da7afb903cd39d1c695b33c5fa23e59d9da0e8
Cr-Commit-Position: refs/heads/master@{#430883}

[xhwang, 2016-11-08 20:03:10]

xhwang@chromium.org changed reviewers:
+ alokp@chromium.org, wolenetz@chromium.org

[xhwang, 2016-11-08 20:03:11]

PTAL

[alokp, 2016-11-08 21:34:08]

non-owner lgtm

https://codereview.chromium.org/2484763005/diff/1/media/mojo/interfaces/media...
File media/mojo/interfaces/media_types.mojom (right):

https://codereview.chromium.org/2484763005/diff/1/media/mojo/interfaces/media...
media/mojo/interfaces/media_types.mojom:108: bool is_end_of_stream;
If "data_size == 0 && side_data_size == 0", would it be correct to conclude that
this is eos buffer?

[wolenetz, 2016-11-08 21:34:12]

wolenetz@chromium.org changed reviewers:
+ chcunningham@chromium.org

[wolenetz, 2016-11-08 21:34:12]

Over to chcunningham@ - I'm heads down on ffmpeg roll currently.

[chcunningham, 2016-11-08 22:00:38]

lgtm

https://codereview.chromium.org/2484763005/diff/1/media/mojo/common/mojo_deco...
File media/mojo/common/mojo_decoder_buffer_converter.cc (right):

https://codereview.chromium.org/2484763005/diff/1/media/mojo/common/mojo_deco...
media/mojo/common/mojo_decoder_buffer_converter.cc:107: if
(media_buffer->end_of_stream() || media_buffer->data_size() == 0) {
Could these be simplified to simply if (!media_buffer->data_size())?

[xhwang, 2016-11-08 22:13:51]

https://codereview.chromium.org/2484763005/diff/1/media/mojo/common/mojo_deco...
File media/mojo/common/mojo_decoder_buffer_converter.cc (right):

https://codereview.chromium.org/2484763005/diff/1/media/mojo/common/mojo_deco...
media/mojo/common/mojo_decoder_buffer_converter.cc:107: if
(media_buffer->end_of_stream() || media_buffer->data_size() == 0) {
On 2016/11/08 22:00:38, chcunningham wrote:
> Could these be simplified to simply if (!media_buffer->data_size())?

We can't call anything (including data_size()) if the buffer is EOS:

https://cs.chromium.org/chromium/src/media/base/decoder_buffer.h?rcl=0&l=106

https://codereview.chromium.org/2484763005/diff/1/media/mojo/interfaces/media...
File media/mojo/interfaces/media_types.mojom (right):

https://codereview.chromium.org/2484763005/diff/1/media/mojo/interfaces/media...
media/mojo/interfaces/media_types.mojom:108: bool is_end_of_stream;
On 2016/11/08 21:34:08, alokp wrote:
> If "data_size == 0 && side_data_size == 0", would it be correct to conclude
that
> this is eos buffer?

We could, but I feel it's really hacky and confusing. A dedicated flag here
seems more clear to me.

[xhwang, 2016-11-08 22:14:16]

xhwang@chromium.org changed reviewers:
+ dcheng@chromium.org

[xhwang, 2016-11-08 22:14:16]

dcheng: mojom OWNERS review please!

[alokp, 2016-11-08 22:17:30]

https://codereview.chromium.org/2484763005/diff/1/media/mojo/interfaces/media...
File media/mojo/interfaces/media_types.mojom (right):

https://codereview.chromium.org/2484763005/diff/1/media/mojo/interfaces/media...
media/mojo/interfaces/media_types.mojom:108: bool is_end_of_stream;
On 2016/11/08 22:13:51, xhwang wrote:
> On 2016/11/08 21:34:08, alokp wrote:
> > If "data_size == 0 && side_data_size == 0", would it be correct to conclude
> that
> > this is eos buffer?
> 
> We could, but I feel it's really hacky and confusing. A dedicated flag here
> seems more clear to me.

I agree. Should we add this flag to media::DecoderBuffer as well to keep things
consistent? "data_ == NULL" is equally hacky.

[xhwang, 2016-11-08 22:28:35]

https://codereview.chromium.org/2484763005/diff/1/media/mojo/interfaces/media...
File media/mojo/interfaces/media_types.mojom (right):

https://codereview.chromium.org/2484763005/diff/1/media/mojo/interfaces/media...
media/mojo/interfaces/media_types.mojom:108: bool is_end_of_stream;
On 2016/11/08 22:17:30, alokp wrote:
> On 2016/11/08 22:13:51, xhwang wrote:
> > On 2016/11/08 21:34:08, alokp wrote:
> > > If "data_size == 0 && side_data_size == 0", would it be correct to
conclude
> > that
> > > this is eos buffer?
> > 
> > We could, but I feel it's really hacky and confusing. A dedicated flag here
> > seems more clear to me.
> 
> I agree. Should we add this flag to media::DecoderBuffer as well to keep
things
> consistent? "data_ == NULL" is equally hacky.

Agreed. I tried refactor DecoderBuffer last week but it's more involved than
what I thought. This code is pretty old and needs some non-trivial work.

I added a comment in the bug:
https://bugs.chromium.org/p/chromium/issues/detail?id=663438#c2

[dcheng, 2016-11-08 22:31:00]

mojo lgtm, but this seems pretty complex to reason about =/

[xhwang, 2016-11-08 22:32:19]

On 2016/11/08 22:31:00, dcheng wrote:
> mojo lgtm, but this seems pretty complex to reason about =/

Agreed. That's why I filed the bug. I want to remove the 0-byte-buffer usage
altogether but apparently we have tests generating such buffer :(

[xhwang, 2016-11-08 22:32:30]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2016-11-08 22:33:24]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-09 02:21:36]

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

[commit-bot: I haz the power, 2016-11-09 02:21:37]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[xhwang, 2016-11-09 05:51:34]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2016-11-09 05:51:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-09 07:18:45]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-11-09 07:20:29]

Description was changed from

==========
media: Support transporting 0-byte DecoderBuffer over mojo

BUG=663438
TEST=Added a new unitttest.
==========

to

==========
media: Support transporting 0-byte DecoderBuffer over mojo

BUG=663438
TEST=Added a new unitttest.

Committed: https://crrev.com/98da7afb903cd39d1c695b33c5fa23e59d9da0e8
Cr-Commit-Position: refs/heads/master@{#430883}
==========

[commit-bot: I haz the power, 2016-11-09 07:20:30]

Patchset 1 (id:??) landed as
https://crrev.com/98da7afb903cd39d1c695b33c5fa23e59d9da0e8
Cr-Commit-Position: refs/heads/master@{#430883}