media: Create Mojo StructTraits for VideoFrame

media/mojo/interfaces/video_frame_struct_traits.cc

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "media/mojo/interfaces/video_frame_struct_traits.h"
+
+#include "base/logging.h"
+#include "media/mojo/common/mojo_shared_buffer_video_frame.h"
+
+namespace mojo {
+
+// static
+template <>
+media::mojom::VideoFrameDataPtr StructTraits<media::mojom::VideoFrameDataView,
+                                             scoped_refptr<media::VideoFrame>>::
+    data(const scoped_refptr<media::VideoFrame>& input) {

[dcheng, 2017/06/05 20:33:01]

I think we'll need to use a context object for this. Otherwise, we create a
shared buffer twice =/

See
https://chromium.googlesource.com/chromium/src/mojo/+/master/public/cpp/bindi...

[sandersd (OOO until July 31), 2017/06/05 20:46:13]

Ack :-(

[sandersd (OOO until July 31), 2017/06/06 01:05:47]

I was unable to make this work because a SharedBufferHandle is not copyable.

[dcheng, 2017/06/08 09:41:08]

Hmm... so I guess this is tricky. Out of curiosity, what happens if you return a
const media::mojom::VideoFrameDataPtr&? Does Mojo handle this correctly?

+yzshen for any additional insight in the best way to handle this situation.

[yzshen1, 2017/06/08 17:45:14]

Daniel is right that we should probably put this in a context object.

You could return a media::mojom::VideoFrameDataPtr& (i.e., a non-const
reference), it will be accessed twice, the first time is used to calculate the
size and unchanged; the second time will take the contents.

[sandersd (OOO until July 31), 2017/06/08 20:10:09]

Done.

+  // EOS frames contain no data.
+  if (input->metadata()->IsTrue(media::VideoFrameMetadata::END_OF_STREAM))
+    return nullptr;
+
+  if (input->storage_type() == media::VideoFrame::STORAGE_MOJO_SHARED_BUFFER) {
+    media::MojoSharedBufferVideoFrame* mojo_frame =
+        static_cast<media::MojoSharedBufferVideoFrame*>(input.get());
+
+    // TODO(jrummell): Should this really be a writable clone?
+    mojo::ScopedSharedBufferHandle duplicated_handle =
+        mojo_frame->Handle().Clone();
+    DCHECK(duplicated_handle.is_valid());
+
+    return media::mojom::VideoFrameData::NewSharedBufferData(
+        media::mojom::SharedBufferVideoFrameData::New(
+            std::move(duplicated_handle), mojo_frame->MappedSize(),
+            mojo_frame->stride(media::VideoFrame::kYPlane),
+            mojo_frame->stride(media::VideoFrame::kUPlane),
+            mojo_frame->stride(media::VideoFrame::kVPlane),
+            mojo_frame->PlaneOffset(media::VideoFrame::kYPlane),
+            mojo_frame->PlaneOffset(media::VideoFrame::kUPlane),
+            mojo_frame->PlaneOffset(media::VideoFrame::kVPlane)));
+  }
+
+  if (input->HasTextures()) {
+    // TODO(sandersd): Friend VideoFrame so that we can access the array?
+    std::vector<gpu::MailboxHolder> mailbox_holder(
+        media::VideoFrame::kMaxPlanes);
+    for (size_t i = 0; i < media::VideoFrame::kMaxPlanes; i++)
+      mailbox_holder.push_back(input->mailbox_holder(i));
+    return media::mojom::VideoFrameData::NewMailboxData(
+        media::mojom::MailboxVideoFrameData::New(std::move(mailbox_holder)));
+  }
+
+  NOTREACHED() << "Unsupported VideoFrame conversion";
+  return nullptr;
+}
+
+// static
+template <>
+bool StructTraits<media::mojom::VideoFrameDataView,
+                  scoped_refptr<media::VideoFrame>>::
+    Read(media::mojom::VideoFrameDataView input,
+         scoped_refptr<media::VideoFrame>* output) {
+  // View of the |data| member of the input media::mojom::VideoFrame.
+  media::mojom::VideoFrameDataDataView data;
+  input.GetDataDataView(&data);
+
+  DCHECK_EQ(input.end_of_stream(), data.is_null());
+  if (input.end_of_stream()) {
+    *output = media::VideoFrame::CreateEOSFrame();
+    return true;
+  }
+
+  media::VideoPixelFormat format;
+  if (!input.ReadFormat(&format))
+    return false;
+
+  gfx::Size coded_size;
+  if (!input.ReadCodedSize(&coded_size))
+    return false;
+
+  gfx::Rect visible_rect;
+  if (!input.ReadVisibleRect(&visible_rect))
+    return false;
+
+  gfx::Size natural_size;
+  if (!input.ReadNaturalSize(&natural_size))
+    return false;
+
+  base::TimeDelta timestamp;
+  if (!input.ReadTimestamp(&timestamp))
+    return false;
+
+  if (data.is_shared_buffer_data()) {
+    media::mojom::SharedBufferVideoFrameDataDataView shared_buffer_data;
+    data.GetSharedBufferDataDataView(&shared_buffer_data);
+
+    *output = media::MojoSharedBufferVideoFrame::Create(
+        format, coded_size, visible_rect, natural_size,
+        shared_buffer_data.TakeFrameData(),
+        shared_buffer_data.frame_data_size(), shared_buffer_data.y_offset(),

[dcheng, 2017/06/05 20:33:01]

Do we need to do any basic range checking here?

[sandersd (OOO until July 31), 2017/06/06 01:05:47]

This should be checked by media::MojoSharedBufferVideoFrame::Create(), and the
existing TypeConverter relies on that for its safety.

It turns out the media::MojoSharedBufferVideoFrame::Create() was not actually
checking this.

+        shared_buffer_data.u_offset(), shared_buffer_data.v_offset(),
+        shared_buffer_data.y_stride(), shared_buffer_data.u_stride(),
+        shared_buffer_data.v_stride(), timestamp);
+    return true;
+  }
+
+  if (data.is_mailbox_data()) {
+    media::mojom::MailboxVideoFrameDataDataView mailbox_data;
+    data.GetMailboxDataDataView(&mailbox_data);
+
+    std::vector<gpu::MailboxHolder> mailbox_holder;
+    if (!mailbox_data.ReadMailboxHolder(&mailbox_holder))
+      return false;
+
+    DCHECK_EQ(mailbox_holder.size(), media::VideoFrame::kMaxPlanes);
+    gpu::MailboxHolder mailbox_holder_array[media::VideoFrame::kMaxPlanes];
+    for (size_t i = 0; i < media::VideoFrame::kMaxPlanes; i++)
+      mailbox_holder_array[i] = mailbox_holder[i];
+
+    *output = media::VideoFrame::WrapNativeTextures(
+        format, mailbox_holder_array, media::VideoFrame::ReleaseMailboxCB(),
+        coded_size, visible_rect, natural_size, timestamp);

[dcheng, 2017/06/05 20:33:01]

Similar question about range checks here--anything interesting needed here?

[sandersd (OOO until July 31), 2017/06/06 01:05:47]

This one is fine. I added a check for visible_rect < coded_size regardless.

+    return true;
+  }
+
+  // TODO(sandersd): Switch on the union tag to avoid this ugliness?
+  NOTREACHED();
+  return false;
+}
+
+}  // namespace mojo