Define AudioBuffer and VideoFrame for mojo

Define AudioBuffer and VideoFrame for mojo

Adds support so they can be converted to and from the matching classes
in media::

BUG=510088
TEST=new tests pass

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

[jrummell, 2015-11-23 19:41:26]

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

[jrummell, 2015-11-23 19:41:27]

PTAL.

[xhwang, 2015-11-23 22:22:19]

Looking pretty good. Just a few comments.

https://chromiumcodereview.appspot.com/1468153003/diff/1/media/base/audio_buf...
File media/base/audio_buffer.cc (right):

https://chromiumcodereview.appspot.com/1468153003/diff/1/media/base/audio_buf...
media/base/audio_buffer.cc:50: int data_size = frame_count * bytes_per_channel;
this should be something like data_size_per_channel; it's confusing to have
data_size and data_size_ with different meanings.

https://chromiumcodereview.appspot.com/1468153003/diff/1/media/base/audio_buf...
media/base/audio_buffer.cc:54: return;
Move this up to l.47?

https://chromiumcodereview.appspot.com/1468153003/diff/1/media/base/audio_buf...
File media/base/audio_buffer.h (right):

https://chromiumcodereview.appspot.com/1468153003/diff/1/media/base/audio_buf...
media/base/audio_buffer.h:137: scoped_refptr<AudioBuffer>>;
Add a comment why we need this friend here.

https://chromiumcodereview.appspot.com/1468153003/diff/1/media/mojo/interface...
File media/mojo/interfaces/media_types.mojom (right):

https://chromiumcodereview.appspot.com/1468153003/diff/1/media/mojo/interface...
media/mojo/interfaces/media_types.mojom:259: array<uint8>? data;
"?" makes it nullable. In what case can we have null data? EOS buffer? It'd be
great to make this clear in this interface.

https://chromiumcodereview.appspot.com/1468153003/diff/1/media/mojo/interface...
media/mojo/interfaces/media_types.mojom:262: // This defines a mojo transport
format for media::VideoFrame.
Add a TODO to support shared memory based VideoFrame. When we do that, we'll
probably also need StorageType in the interface.

https://chromiumcodereview.appspot.com/1468153003/diff/1/media/mojo/interface...
media/mojo/interfaces/media_types.mojom:264: // Format of the decrypted frame.
"decrypted" not needed as this is a generic VideoFrame.

https://chromiumcodereview.appspot.com/1468153003/diff/1/media/mojo/interface...
media/mojo/interfaces/media_types.mojom:269: int32 height;
mojo.Rect coded_size?

"coded" would be consistent with media::VideoFrame.

https://chromiumcodereview.appspot.com/1468153003/diff/1/media/mojo/interface...
media/mojo/interfaces/media_types.mojom:273: int32 visible_height;
Use mojo.Rect

https://chromiumcodereview.appspot.com/1468153003/diff/1/media/mojo/interface...
media/mojo/interfaces/media_types.mojom:277: int32 natural_height;
Use mojo.Rect

https://chromiumcodereview.appspot.com/1468153003/diff/1/media/mojo/interface...
media/mojo/interfaces/media_types.mojom:286: array<uint8>? y_data;
ditto about nullable data.

https://chromiumcodereview.appspot.com/1468153003/diff/1/media/mojo/services/...
File media/mojo/services/media_type_converters.cc (right):

https://chromiumcodereview.appspot.com/1468153003/diff/1/media/mojo/services/...
media/mojo/services/media_type_converters.cc:550: // TODO(jrummell): Use a
shared buffer rather than copying the data.
We don't use shared memory for AudioBuffer yet. So this TODO is not needed.

https://chromiumcodereview.appspot.com/1468153003/diff/1/media/mojo/services/...
media/mojo/services/media_type_converters.cc:551: std::vector<uint8_t>
input_data(input->data_.get(),
how about EOS buffer?

https://chromiumcodereview.appspot.com/1468153003/diff/1/media/mojo/services/...
media/mojo/services/media_type_converters.cc:569: const int size_per_channel =
storage.size() / input->channel_count;
DCHECK that storage.size() % input->channel_count is 0?

https://chromiumcodereview.appspot.com/1468153003/diff/1/media/mojo/services/...
media/mojo/services/media_type_converters.cc:571: channel_ptrs[i] =
vector_as_array(&storage) + i * size_per_channel;
you can use storate.data() now:

https://chromium-cpp.appspot.com/#library-whitelist

https://chromiumcodereview.appspot.com/1468153003/diff/1/media/mojo/services/...
media/mojo/services/media_type_converters.cc:588:
input->metadata()->IsTrue(media::VideoFrameMetadata::END_OF_STREAM);
If it's a EOS frame, we probably don't need anything below...

https://chromiumcodereview.appspot.com/1468153003/diff/1/media/mojo/services/...
media/mojo/services/media_type_converters.cc:600: // each plane.
We probably will need to support both mode depending on the StorageType.

https://chromiumcodereview.appspot.com/1468153003/diff/1/media/mojo/services/...
media/mojo/services/media_type_converters.cc:641:
vector_as_array(&input->y_data.storage()),
.data()

https://chromiumcodereview.appspot.com/1468153003/diff/1/media/mojo/services/...
File media/mojo/services/media_type_converters_unittest.cc (right):

https://chromiumcodereview.appspot.com/1468153003/diff/1/media/mojo/services/...
media/mojo/services/media_type_converters_unittest.cc:19: void
CompareBytes(uint8* original_data, uint8* result_data, size_t length) {
Can we just use memcmp? Since all bytes should match, probably we don't care at
which byte it start to differ?

https://chromiumcodereview.appspot.com/1468153003/diff/1/media/mojo/services/...
media/mojo/services/media_type_converters_unittest.cc:19: void
CompareBytes(uint8* original_data, uint8* result_data, size_t length) {
move these to an anonymous namespace

https://chromiumcodereview.appspot.com/1468153003/diff/1/media/mojo/services/...
media/mojo/services/media_type_converters_unittest.cc:43: sample_format ==
kSampleFormatPlanarS32) {
Seems like we should have a function IsPlanar(SampleFormat) in samplr_format.h

https://chromiumcodereview.appspot.com/1468153003/diff/1/media/mojo/services/...
media/mojo/services/media_type_converters_unittest.cc:59: void
ComparePlane(size_t plane,
how about CompareVideoPlane?

https://chromiumcodereview.appspot.com/1468153003/diff/1/media/mojo/services/...
media/mojo/services/media_type_converters_unittest.cc:70: const
scoped_refptr<VideoFrame>& result) {
To be more generic, can you extend this to support EOS frames as well?

https://chromiumcodereview.appspot.com/1468153003/diff/1/media/mojo/services/...
media/mojo/services/media_type_converters_unittest.cc:300:
result->metadata()->IsTrue(media::VideoFrameMetadata::END_OF_STREAM));
+dalecurtis

Wondering why we don't have a simple call VideoFrame::is_end_of_stream()?

https://code.google.com/p/chromium/codesearch#search/&q=VideoFrameMetadata::E...

[jrummell, 2015-11-24 02:37:52]

jrummell@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[jrummell, 2015-11-24 02:37:53]

Updated. Added dalecurtis@.

https://codereview.chromium.org/1468153003/diff/1/media/base/audio_buffer.cc
File media/base/audio_buffer.cc (right):

https://codereview.chromium.org/1468153003/diff/1/media/base/audio_buffer.cc#...
media/base/audio_buffer.cc:50: int data_size = frame_count * bytes_per_channel;
On 2015/11/23 22:22:18, xhwang wrote:
> this should be something like data_size_per_channel; it's confusing to have
> data_size and data_size_ with different meanings.

Done.

https://codereview.chromium.org/1468153003/diff/1/media/base/audio_buffer.cc#...
media/base/audio_buffer.cc:54: return;
On 2015/11/23 22:22:18, xhwang wrote:
> Move this up to l.47?

But it would then miss the DCHECK on line 49. Moved it after that.

https://codereview.chromium.org/1468153003/diff/1/media/base/audio_buffer.h
File media/base/audio_buffer.h (right):

https://codereview.chromium.org/1468153003/diff/1/media/base/audio_buffer.h#n...
media/base/audio_buffer.h:137: scoped_refptr<AudioBuffer>>;
On 2015/11/23 22:22:18, xhwang wrote:
> Add a comment why we need this friend here.

Done.

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

https://codereview.chromium.org/1468153003/diff/1/media/mojo/interfaces/media...
media/mojo/interfaces/media_types.mojom:259: array<uint8>? data;
On 2015/11/23 22:22:18, xhwang wrote:
> "?" makes it nullable. In what case can we have null data? EOS buffer? It'd be
> great to make this clear in this interface.

Done.

https://codereview.chromium.org/1468153003/diff/1/media/mojo/interfaces/media...
media/mojo/interfaces/media_types.mojom:262: // This defines a mojo transport
format for media::VideoFrame.
On 2015/11/23 22:22:18, xhwang wrote:
> Add a TODO to support shared memory based VideoFrame. When we do that, we'll
> probably also need StorageType in the interface.

Done.

https://codereview.chromium.org/1468153003/diff/1/media/mojo/interfaces/media...
media/mojo/interfaces/media_types.mojom:264: // Format of the decrypted frame.
On 2015/11/23 22:22:18, xhwang wrote:
> "decrypted" not needed as this is a generic VideoFrame.

Done.

https://codereview.chromium.org/1468153003/diff/1/media/mojo/interfaces/media...
media/mojo/interfaces/media_types.mojom:269: int32 height;
On 2015/11/23 22:22:18, xhwang wrote:
> mojo.Rect coded_size?
> 
> "coded" would be consistent with media::VideoFrame. 

Done.

https://codereview.chromium.org/1468153003/diff/1/media/mojo/interfaces/media...
media/mojo/interfaces/media_types.mojom:273: int32 visible_height;
On 2015/11/23 22:22:18, xhwang wrote:
> Use mojo.Rect

Done.

https://codereview.chromium.org/1468153003/diff/1/media/mojo/interfaces/media...
media/mojo/interfaces/media_types.mojom:277: int32 natural_height;
On 2015/11/23 22:22:18, xhwang wrote:
> Use mojo.Rect

It's actually mojo.Size. Done.

https://codereview.chromium.org/1468153003/diff/1/media/mojo/interfaces/media...
media/mojo/interfaces/media_types.mojom:286: array<uint8>? y_data;
On 2015/11/23 22:22:18, xhwang wrote:
> ditto about nullable data.

Done.

https://codereview.chromium.org/1468153003/diff/1/media/mojo/services/media_t...
File media/mojo/services/media_type_converters.cc (right):

https://codereview.chromium.org/1468153003/diff/1/media/mojo/services/media_t...
media/mojo/services/media_type_converters.cc:550: // TODO(jrummell): Use a
shared buffer rather than copying the data.
On 2015/11/23 22:22:18, xhwang wrote:
> We don't use shared memory for AudioBuffer yet. So this TODO is not needed.

Done.

https://codereview.chromium.org/1468153003/diff/1/media/mojo/services/media_t...
media/mojo/services/media_type_converters.cc:551: std::vector<uint8_t>
input_data(input->data_.get(),
On 2015/11/23 22:22:18, xhwang wrote:
> how about EOS buffer?

EOSBuffer has 0 bytes, but we should avoid allocating anything. Done.

https://codereview.chromium.org/1468153003/diff/1/media/mojo/services/media_t...
media/mojo/services/media_type_converters.cc:569: const int size_per_channel =
storage.size() / input->channel_count;
On 2015/11/23 22:22:19, xhwang wrote:
> DCHECK that storage.size() % input->channel_count is 0?

Done.

https://codereview.chromium.org/1468153003/diff/1/media/mojo/services/media_t...
media/mojo/services/media_type_converters.cc:571: channel_ptrs[i] =
vector_as_array(&storage) + i * size_per_channel;
On 2015/11/23 22:22:19, xhwang wrote:
> you can use storate.data() now:
> 
> https://chromium-cpp.appspot.com/#library-whitelist

Done.

https://codereview.chromium.org/1468153003/diff/1/media/mojo/services/media_t...
media/mojo/services/media_type_converters.cc:588:
input->metadata()->IsTrue(media::VideoFrameMetadata::END_OF_STREAM);
On 2015/11/23 22:22:18, xhwang wrote:
> If it's a EOS frame, we probably don't need anything below...

Done.

https://codereview.chromium.org/1468153003/diff/1/media/mojo/services/media_t...
media/mojo/services/media_type_converters.cc:600: // each plane.
On 2015/11/23 22:22:18, xhwang wrote:
> We probably will need to support both mode depending on the StorageType.

Agreed. Haven't figured out whether STORAGE_SHMEM is pepper specific or generic
yet.

https://codereview.chromium.org/1468153003/diff/1/media/mojo/services/media_t...
media/mojo/services/media_type_converters.cc:641:
vector_as_array(&input->y_data.storage()),
On 2015/11/23 22:22:18, xhwang wrote:
> .data()

Done.

https://codereview.chromium.org/1468153003/diff/1/media/mojo/services/media_t...
File media/mojo/services/media_type_converters_unittest.cc (right):

https://codereview.chromium.org/1468153003/diff/1/media/mojo/services/media_t...
media/mojo/services/media_type_converters_unittest.cc:19: void
CompareBytes(uint8* original_data, uint8* result_data, size_t length) {
On 2015/11/23 22:22:19, xhwang wrote:
> Can we just use memcmp? Since all bytes should match, probably we don't care
at
> which byte it start to differ?

Done.

https://codereview.chromium.org/1468153003/diff/1/media/mojo/services/media_t...
media/mojo/services/media_type_converters_unittest.cc:19: void
CompareBytes(uint8* original_data, uint8* result_data, size_t length) {
On 2015/11/23 22:22:19, xhwang wrote:
> move these to an anonymous namespace

Done.

https://codereview.chromium.org/1468153003/diff/1/media/mojo/services/media_t...
media/mojo/services/media_type_converters_unittest.cc:43: sample_format ==
kSampleFormatPlanarS32) {
On 2015/11/23 22:22:19, xhwang wrote:
> Seems like we should have a function IsPlanar(SampleFormat) in samplr_format.h

Done.

https://codereview.chromium.org/1468153003/diff/1/media/mojo/services/media_t...
media/mojo/services/media_type_converters_unittest.cc:59: void
ComparePlane(size_t plane,
On 2015/11/23 22:22:19, xhwang wrote:
> how about CompareVideoPlane?

Done.

https://codereview.chromium.org/1468153003/diff/1/media/mojo/services/media_t...
media/mojo/services/media_type_converters_unittest.cc:70: const
scoped_refptr<VideoFrame>& result) {
On 2015/11/23 22:22:19, xhwang wrote:
> To be more generic, can you extend this to support EOS frames as well? 

Not sure it is that beneficial since we only check one field, but done anyway.

[xhwang, 2015-11-24 05:59:13]

lgtm % nits

https://chromiumcodereview.appspot.com/1468153003/diff/20001/media/base/audio...
File media/base/audio_buffer.cc (right):

https://chromiumcodereview.appspot.com/1468153003/diff/20001/media/base/audio...
media/base/audio_buffer.cc:83: sample_format_ == kSampleFormatF32) <<
sample_format_;
!IsPlanar()?

Actually we could also have kUnknownSampleFormat, so maybe we need an
IsInterleaved() method.

https://chromiumcodereview.appspot.com/1468153003/diff/20001/media/mojo/servi...
File media/mojo/services/media_type_converters.cc (right):

https://chromiumcodereview.appspot.com/1468153003/diff/20001/media/mojo/servi...
media/mojo/services/media_type_converters.cc:593: if (!buffer->end_of_stream) {
nit: return early here

if (buffer->end_of_stream)
  return buffer.Pass();

// handle non EOS buffer

https://chromiumcodereview.appspot.com/1468153003/diff/20001/media/mojo/servi...
File media/mojo/services/media_type_converters_unittest.cc (right):

https://chromiumcodereview.appspot.com/1468153003/diff/20001/media/mojo/servi...
media/mojo/services/media_type_converters_unittest.cc:52: sample_format ==
kSampleFormatS32 || sample_format == kSampleFormatF32)
IsInterleaved()

[jrummell, 2015-11-24 20:51:05]

Updated.

https://codereview.chromium.org/1468153003/diff/20001/media/base/audio_buffer.cc
File media/base/audio_buffer.cc (right):

https://codereview.chromium.org/1468153003/diff/20001/media/base/audio_buffer...
media/base/audio_buffer.cc:83: sample_format_ == kSampleFormatF32) <<
sample_format_;
On 2015/11/24 05:59:13, xhwang wrote:
> !IsPlanar()?
> 
> Actually we could also have kUnknownSampleFormat, so maybe we need an
> IsInterleaved() method.

Done.

https://codereview.chromium.org/1468153003/diff/20001/media/mojo/services/med...
File media/mojo/services/media_type_converters.cc (right):

https://codereview.chromium.org/1468153003/diff/20001/media/mojo/services/med...
media/mojo/services/media_type_converters.cc:593: if (!buffer->end_of_stream) {
On 2015/11/24 05:59:13, xhwang wrote:
> nit: return early here
> 
> if (buffer->end_of_stream)
>   return buffer.Pass();
> 
> // handle non EOS buffer

Done.

https://codereview.chromium.org/1468153003/diff/20001/media/mojo/services/med...
File media/mojo/services/media_type_converters_unittest.cc (right):

https://codereview.chromium.org/1468153003/diff/20001/media/mojo/services/med...
media/mojo/services/media_type_converters_unittest.cc:52: sample_format ==
kSampleFormatS32 || sample_format == kSampleFormatF32)
On 2015/11/24 05:59:13, xhwang wrote:
> IsInterleaved()

Done.

[DaleCurtis, 2015-11-24 21:40:08]

lgtm % optional change

https://codereview.chromium.org/1468153003/diff/40001/media/base/sample_forma...
File media/base/sample_format.cc (right):

https://codereview.chromium.org/1468153003/diff/40001/media/base/sample_forma...
media/base/sample_format.cc:54: bool IsPlanar(SampleFormat sample_format) {
Might be worth making these both fully-qualified switch statements so when
formats are added in the future a compile time error will occur.

[jrummell, 2015-11-24 22:34:41]

Thanks for the reviews.

https://codereview.chromium.org/1468153003/diff/40001/media/base/sample_forma...
File media/base/sample_format.cc (right):

https://codereview.chromium.org/1468153003/diff/40001/media/base/sample_forma...
media/base/sample_format.cc:54: bool IsPlanar(SampleFormat sample_format) {
On 2015/11/24 21:40:08, DaleCurtis wrote:
> Might be worth making these both fully-qualified switch statements so when
> formats are added in the future a compile time error will occur.

Done.

[jrummell, 2015-11-24 22:34:53]

The CQ bit was checked by jrummell@chromium.org

[jrummell, 2015-11-24 22:34:53]

The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org,
dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/1468153003/#ps60001
(title: "switch statement")

[commit-bot: I haz the power, 2015-11-24 22:37:05]

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

[commit-bot: I haz the power, 2015-11-25 00:23:02]

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

[commit-bot: I haz the power, 2015-11-25 00:23:03]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jrummell, 2015-11-25 01:04:59]

The CQ bit was checked by jrummell@chromium.org

[commit-bot: I haz the power, 2015-11-25 01:11:08]

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

[commit-bot: I haz the power, 2015-11-25 03:10:17]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-11-25 03:12:02]

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

[raymes, 2015-11-25 03:36:09]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/1474843003/ by raymes@chromium.org.

The reason for reverting is: This looks like it broke the arm buildbot:
https://build.chromium.org/p/chromium.linux/builders/Android%20Arm64%20Builde....

[raymes, 2015-11-25 03:43:35]

On 2015/11/25 03:36:09, raymes wrote:
> A revert of this CL (patchset #4 id:60001) has been created in
> https://codereview.chromium.org/1474843003/ by mailto:raymes@chromium.org.
> 
> The reason for reverting is: This looks like it broke the arm buildbot:
>
https://build.chromium.org/p/chromium.linux/builders/Android%20Arm64%20Builde....

It looks like this conflicted with https://codereview.chromium.org/1422113002
which landed at almost the same time.