Add PP_VideoFrame, PPB_VideoReader, and PPB_VideoWriter APIs to Pepper.

Add PP_VideoFrame, PPB_VideoReader, and PPB_VideoWriter APIs to Pepper.
Defines a PP_VideoFrame struct, and 2 new APIs,PPB_ VideoReader to consume a video stream, and
PPB_VideoWriter to generate a video stream.

BUG=none
TEST=none


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=192455

[darin1, 2013-04-02 18:08:38]

I just did a quick read over the IDL files, and they look great to me.  Just one
naming suggestion.

https://codereview.chromium.org/13461010/diff/1/ppapi/api/ppb_video_writer.idl
File ppapi/api/ppb_video_writer.idl (right):

https://codereview.chromium.org/13461010/diff/1/ppapi/api/ppb_video_writer.id...
ppapi/api/ppb_video_writer.idl:85: int32_t EmitFrame([in] PP_Resource writer,
bikeshed-nit: What do you think about calling this PutNextFrame to parallel
GetNextFrame?

[yzshen1, 2013-04-02 19:28:39]

Thanks, Bill!

A few nits + one high-level suggestion:
With the current API definition, in order to reduce the overhead of
creating/destroying many ImageData resources, we need to add caching support,
just as what we have done for ReplaceContents() (in ppb_image_data_proxy.cc).

Or shall we consider an API that allocate several buffers and recycle them
explicitly? (PPB_VideoCapture_Dev does that.)

https://codereview.chromium.org/13461010/diff/1/ppapi/api/ppb_video_reader.idl
File ppapi/api/ppb_video_reader.idl (right):

https://codereview.chromium.org/13461010/diff/1/ppapi/api/ppb_video_reader.id...
ppapi/api/ppb_video_reader.idl:48: * identifying the stream.
It is good to comment how to get this ID.

https://codereview.chromium.org/13461010/diff/1/ppapi/api/ppb_video_reader.id...
ppapi/api/ppb_video_reader.idl:54: * Returns PP_ERROR_INPROGRESS if the writer
has already opened a stream.
writer -> reader?

https://codereview.chromium.org/13461010/diff/1/ppapi/api/ppb_video_reader.id...
ppapi/api/ppb_video_reader.idl:70: int32_t Close([in] PP_Resource reader);
nit:
(1) So far, all Close() methods (FileIO, URLLoader, etc.) have void as output.
Do we need the return value? It doesn't sound very useful.

(2) It looks more consistent to order the methods so that Close() is the last.

https://codereview.chromium.org/13461010/diff/1/ppapi/api/ppb_video_writer.idl
File ppapi/api/ppb_video_writer.idl (right):

https://codereview.chromium.org/13461010/diff/1/ppapi/api/ppb_video_writer.id...
ppapi/api/ppb_video_writer.idl:70: int32_t Close([in] PP_Resource writer);
Please see my comments of Close() in ppb_video_reader.h

https://codereview.chromium.org/13461010/diff/1/ppapi/cpp/video.cc
File ppapi/cpp/video.cc (right):

https://codereview.chromium.org/13461010/diff/1/ppapi/cpp/video.cc#newcode62
ppapi/cpp/video.cc:62: video_frame_.image_data = image_data_.pp_resource();
It seems line 62 is not needed because line 57 has done the work. Or we could
replace 57-62 using set_image_data() and set_timestamp().

https://codereview.chromium.org/13461010/diff/1/ppapi/cpp/video.cc#newcode115
ppapi/cpp/video.cc:115: 
The writer implementation is missing.

https://codereview.chromium.org/13461010/diff/1/ppapi/cpp/video.h
File ppapi/cpp/video.h (right):

https://codereview.chromium.org/13461010/diff/1/ppapi/cpp/video.h#newcode23
ppapi/cpp/video.h:23: // TrueTypeFontDesc_Dev
--------------------------------------------------------
Please update the comments.

https://codereview.chromium.org/13461010/diff/1/ppapi/cpp/video.h#newcode79
ppapi/cpp/video.h:79: VideoReader(const InstanceHandle& instance);
explicit, please.

https://codereview.chromium.org/13461010/diff/1/ppapi/cpp/video.h#newcode120
ppapi/cpp/video.h:120: class VideoWriter : public Resource {
Shall we split them into separate files?

[bbudge, 2013-04-02 20:07:34]

https://codereview.chromium.org/13461010/diff/1/ppapi/api/ppb_video_reader.idl
File ppapi/api/ppb_video_reader.idl (right):

https://codereview.chromium.org/13461010/diff/1/ppapi/api/ppb_video_reader.id...
ppapi/api/ppb_video_reader.idl:48: * identifying the stream.
On 2013/04/02 19:28:39, yzshen1 wrote:
> It is good to comment how to get this ID.

Done.

https://codereview.chromium.org/13461010/diff/1/ppapi/api/ppb_video_reader.id...
ppapi/api/ppb_video_reader.idl:54: * Returns PP_ERROR_INPROGRESS if the writer
has already opened a stream.
On 2013/04/02 19:28:39, yzshen1 wrote:
> writer -> reader?

Done.

https://codereview.chromium.org/13461010/diff/1/ppapi/api/ppb_video_reader.id...
ppapi/api/ppb_video_reader.idl:70: int32_t Close([in] PP_Resource reader);
On 2013/04/02 19:28:39, yzshen1 wrote:
> nit:
> (1) So far, all Close() methods (FileIO, URLLoader, etc.) have void as output.
> Do we need the return value? It doesn't sound very useful.
> 
> (2) It looks more consistent to order the methods so that Close() is the last.

Done.

https://codereview.chromium.org/13461010/diff/1/ppapi/api/ppb_video_writer.idl
File ppapi/api/ppb_video_writer.idl (right):

https://codereview.chromium.org/13461010/diff/1/ppapi/api/ppb_video_writer.id...
ppapi/api/ppb_video_writer.idl:70: int32_t Close([in] PP_Resource writer);
On 2013/04/02 19:28:39, yzshen1 wrote:
> Please see my comments of Close() in ppb_video_reader.h

Done.

https://codereview.chromium.org/13461010/diff/1/ppapi/api/ppb_video_writer.id...
ppapi/api/ppb_video_writer.idl:85: int32_t EmitFrame([in] PP_Resource writer,
On 2013/04/02 18:08:38, darin1 wrote:
> bikeshed-nit: What do you think about calling this PutNextFrame to parallel
> GetNextFrame?

I think it helps to understand how the two APIs interact. Done.

https://codereview.chromium.org/13461010/diff/1/ppapi/cpp/video.h
File ppapi/cpp/video.h (right):

https://codereview.chromium.org/13461010/diff/1/ppapi/cpp/video.h#newcode23
ppapi/cpp/video.h:23: // TrueTypeFontDesc_Dev
--------------------------------------------------------
On 2013/04/02 19:28:39, yzshen1 wrote:
> Please update the comments.

Done.

https://codereview.chromium.org/13461010/diff/1/ppapi/cpp/video.h#newcode79
ppapi/cpp/video.h:79: VideoReader(const InstanceHandle& instance);
On 2013/04/02 19:28:39, yzshen1 wrote:
> explicit, please.

Done.

https://codereview.chromium.org/13461010/diff/1/ppapi/cpp/video.h#newcode120
ppapi/cpp/video.h:120: class VideoWriter : public Resource {
I thought about this. If we put each class in its own file, then we should
probably put the VideoFrame struct in its own file too. I decided to follow the
precedent of InputEvent, where closely related APIs are bundled together. The
file names 'video.h' and 'video.cc' are a bit too generic and thought about
calling them 'video_stream.*' but that would suggest we name these APIs
VideoStreamReader and VideoStreamWriter, and that seems too much. I'll split
these out into separate files if people feel strongly about it.

On 2013/04/02 19:28:39, yzshen1 wrote:
> Shall we split them into separate files?

[bbudge, 2013-04-03 10:42:29]

https://codereview.chromium.org/13461010/diff/1/ppapi/cpp/video.cc
File ppapi/cpp/video.cc (right):

https://codereview.chromium.org/13461010/diff/1/ppapi/cpp/video.cc#newcode62
ppapi/cpp/video.cc:62: video_frame_.image_data = image_data_.pp_resource();
On 2013/04/02 19:28:39, yzshen1 wrote:
> It seems line 62 is not needed because line 57 has done the work. Or we could
> replace 57-62 using set_image_data() and set_timestamp().

Done.

https://codereview.chromium.org/13461010/diff/1/ppapi/cpp/video.cc#newcode115
ppapi/cpp/video.cc:115: 
On 2013/04/02 19:28:39, yzshen1 wrote:
> The writer implementation is missing.

Done.

[bbudge, 2013-04-03 21:29:14]

Comment on semantics issue.

https://codereview.chromium.org/13461010/diff/5002/ppapi/api/ppb_video_writer...
File ppapi/api/ppb_video_writer.idl (right):

https://codereview.chromium.org/13461010/diff/5002/ppapi/api/ppb_video_writer...
ppapi/api/ppb_video_writer.idl:66: * write to the open stream.
We need a comment here about who owns the frame. Obviously if the plugin writes
to this it can cause coherency problems. Probably the plugin should release the
ImageData resource. For a similar case, see the PPB_Graphics2d ReplaceContents
function.

[dmichael (off chromium), 2013-04-03 22:17:06]

https://chromiumcodereview.appspot.com/13461010/diff/8001/ppapi/cpp/video.h
File ppapi/cpp/video.h (right):

https://chromiumcodereview.appspot.com/13461010/diff/8001/ppapi/cpp/video.h#n...
ppapi/cpp/video.h:69: class VideoReader : public Resource {
I second moving these to their own files. video.h and video.cc just seems so
broad that I'm afraid it might be confusing. Especially since we have other
interfaces that also deal with video (video decoding, video capture, and content
decryption modules)

https://chromiumcodereview.appspot.com/13461010/diff/8001/ppapi/cpp/video.h#n...
ppapi/cpp/video.h:96: int32_t Open(const pp::Var& stream_id,
I think we could just use std::string here.

https://chromiumcodereview.appspot.com/13461010/diff/8001/ppapi/cpp/video.h#n...
ppapi/cpp/video.h:141: int32_t Open(const pp::Var& stream_id,
ditto std::string

[bbudge, 2013-04-04 17:58:05]

https://codereview.chromium.org/13461010/diff/8001/ppapi/cpp/video.h
File ppapi/cpp/video.h (right):

https://codereview.chromium.org/13461010/diff/8001/ppapi/cpp/video.h#newcode69
ppapi/cpp/video.h:69: class VideoReader : public Resource {
On 2013/04/03 22:17:06, dmichael wrote:
> I second moving these to their own files. video.h and video.cc just seems so
> broad that I'm afraid it might be confusing. Especially since we have other
> interfaces that also deal with video (video decoding, video capture, and
content
> decryption modules)

Done.

https://codereview.chromium.org/13461010/diff/8001/ppapi/cpp/video.h#newcode96
ppapi/cpp/video.h:96: int32_t Open(const pp::Var& stream_id,
On 2013/04/03 22:17:06, dmichael wrote:
> I think we could just use std::string here.

Done.

https://codereview.chromium.org/13461010/diff/8001/ppapi/cpp/video.h#newcode141
ppapi/cpp/video.h:141: int32_t Open(const pp::Var& stream_id,
On 2013/04/03 22:17:06, dmichael wrote:
> ditto std::string

Done.

[bbudge, 2013-04-04 19:57:17]

Addressed comments from yesterday's meetings and reviews. PTAL

[dmichael (off chromium), 2013-04-04 20:18:41]

https://codereview.chromium.org/13461010/diff/22001/ppapi/api/ppb_video_reade...
File ppapi/api/ppb_video_reader.idl (right):

https://codereview.chromium.org/13461010/diff/22001/ppapi/api/ppb_video_reade...
ppapi/api/ppb_video_reader.idl:12: M28 = 1.0
We usually start out with 0.1 for Dev stuff

https://codereview.chromium.org/13461010/diff/22001/ppapi/cpp/video_frame.cc
File ppapi/cpp/video_frame.cc (right):

https://codereview.chromium.org/13461010/diff/22001/ppapi/cpp/video_frame.cc#...
ppapi/cpp/video_frame.cc:18: const PP_VideoFrame& pp_video_frame) {
I think you probably need to default-initialize pp_video_frame_ in both
constructors

[bbudge, 2013-04-04 21:15:16]

https://codereview.chromium.org/13461010/diff/22001/ppapi/api/ppb_video_reade...
File ppapi/api/ppb_video_reader.idl (right):

https://codereview.chromium.org/13461010/diff/22001/ppapi/api/ppb_video_reade...
ppapi/api/ppb_video_reader.idl:12: M28 = 1.0
Done for both APIs.
On 2013/04/04 20:18:41, dmichael wrote:
> We usually start out with 0.1 for Dev stuff

https://codereview.chromium.org/13461010/diff/22001/ppapi/cpp/video_frame.cc
File ppapi/cpp/video_frame.cc (right):

https://codereview.chromium.org/13461010/diff/22001/ppapi/cpp/video_frame.cc#...
ppapi/cpp/video_frame.cc:18: const PP_VideoFrame& pp_video_frame) {
On 2013/04/04 20:18:41, dmichael wrote:
> I think you probably need to default-initialize pp_video_frame_ in both
> constructors

Done.

[dmichael (off chromium), 2013-04-04 21:23:02]

lgtm

https://codereview.chromium.org/13461010/diff/22003/ppapi/cpp/video_frame.cc
File ppapi/cpp/video_frame.cc (right):

https://codereview.chromium.org/13461010/diff/22003/ppapi/cpp/video_frame.cc#...
ppapi/cpp/video_frame.cc:28: VideoFrame::VideoFrame(const VideoFrame& other) {
Also need:
: video_frame_()
here

[bbudge, 2013-04-04 21:26:47]

https://codereview.chromium.org/13461010/diff/22003/ppapi/cpp/video_frame.cc
File ppapi/cpp/video_frame.cc (right):

https://codereview.chromium.org/13461010/diff/22003/ppapi/cpp/video_frame.cc#...
ppapi/cpp/video_frame.cc:28: VideoFrame::VideoFrame(const VideoFrame& other) {
Whoops. Done.
On 2013/04/04 21:23:03, dmichael wrote:
> Also need:
> : video_frame_()
> here

[yzshen1, 2013-04-04 23:48:12]

LGTM with two nits.

https://codereview.chromium.org/13461010/diff/32002/ppapi/cpp/video_reader.cc
File ppapi/cpp/video_reader.cc (right):

https://codereview.chromium.org/13461010/diff/32002/ppapi/cpp/video_reader.cc...
ppapi/cpp/video_reader.cc:19: template <> const char*
interface_name<PPB_VideoReader_1_0>() {
Please update the version number.

https://codereview.chromium.org/13461010/diff/32002/ppapi/cpp/video_writer.cc
File ppapi/cpp/video_writer.cc (right):

https://codereview.chromium.org/13461010/diff/32002/ppapi/cpp/video_writer.cc...
ppapi/cpp/video_writer.cc:19: template <> const char*
interface_name<PPB_VideoWriter_1_0>() {
Please update the version number.

[bbudge, 2013-04-04 23:53:36]

Good eye Yuzhu!

https://codereview.chromium.org/13461010/diff/32002/ppapi/cpp/video_reader.cc
File ppapi/cpp/video_reader.cc (right):

https://codereview.chromium.org/13461010/diff/32002/ppapi/cpp/video_reader.cc...
ppapi/cpp/video_reader.cc:19: template <> const char*
interface_name<PPB_VideoReader_1_0>() {
On 2013/04/04 23:48:12, yzshen1 wrote:
> Please update the version number.

Done.

https://codereview.chromium.org/13461010/diff/32002/ppapi/cpp/video_writer.cc
File ppapi/cpp/video_writer.cc (right):

https://codereview.chromium.org/13461010/diff/32002/ppapi/cpp/video_writer.cc...
ppapi/cpp/video_writer.cc:19: template <> const char*
interface_name<PPB_VideoWriter_1_0>() {
On 2013/04/04 23:48:12, yzshen1 wrote:
> Please update the version number.

Done.

[commit-bot: I haz the power, 2013-04-04 23:53:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bbudge@chromium.org/13461010/30003

[commit-bot: I haz the power, 2013-04-05 02:35:55]

Change committed as 192455

[juberti, 2013-04-05 18:46:12]

drive-by lgtm

https://codereview.chromium.org/13461010/diff/30003/ppapi/api/ppb_video_reade...
File ppapi/api/ppb_video_reader.idl (right):

https://codereview.chromium.org/13461010/diff/30003/ppapi/api/ppb_video_reade...
ppapi/api/ppb_video_reader.idl:57: [in] PP_Var stream_id,
In the latest proposal I changed this to be an [out] param, e.g. browser
generates the id for a new track.

[bbudge, 2013-04-05 20:50:24]

https://codereview.chromium.org/13461010/diff/30003/ppapi/api/ppb_video_reade...
File ppapi/api/ppb_video_reader.idl (right):

https://codereview.chromium.org/13461010/diff/30003/ppapi/api/ppb_video_reade...
ppapi/api/ppb_video_reader.idl:57: [in] PP_Var stream_id,
I'll change it in a follow on patch.
On 2013/04/05 18:46:12, juberti wrote:
> In the latest proposal I changed this to be an [out] param, e.g. browser
> generates the id for a new track.