Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
842293003:
Pepper: Define PPB_VideoEncoder API. (Closed)
|
Created:
5 years, 11 months ago by bbudge Modified:
5 years, 10 months ago CC:
chromium-reviews, yusukes+watch_chromium.org, tzik, binji+watch_chromium.org, raymes+watch_chromium.org, teravest+watch_chromium.org, nfullagar1, piman+watch_chromium.org, noelallen1, ihf+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPepper: Define PPB_VideoEncoder API.
Since this bumps the milestone definition in pp_macros.h
NOPRESUBMIT=true
BUG=417589
Committed: https://crrev.com/3abfda6fae03fb234224df6e69337a818b786b3a
Cr-Commit-Position: refs/heads/master@{#314645}
Patch Set 1 #Patch Set 2 : C++ Wrapper Class #
Total comments: 26
Patch Set 3 : Update and add comments. #
Total comments: 13
Patch Set 4 : Add GetFrameCodedSize function, comments. #
Total comments: 3
Patch Set 5 : Add |acceleration| field to PP_SupportedVideoProfile. #
Total comments: 19
Patch Set 6 : #
Messages
Total messages: 45 (10 generated)
lionel.g.landwerlin@intel.com changed reviewers: + lionel.g.landwerlin@intel.com
lionel.g.landwerlin@intel.com changed reviewers: + lionel.g.landwerlin@intel.com
https://codereview.chromium.org/842293003/diff/20001/ppapi/api/ppb_video_enco... File ppapi/api/ppb_video_encoder.idl (right): https://codereview.chromium.org/842293003/diff/20001/ppapi/api/ppb_video_enco... ppapi/api/ppb_video_encoder.idl:118: int32_t Encode( PPB_VideoDecoder has a |decode_id| parameter on its Decode() method : https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/c/ppb_video_... Any reason why the encoder wouldn't have that parameter too? https://codereview.chromium.org/842293003/diff/20001/ppapi/api/ppb_video_enco... ppapi/api/ppb_video_encoder.idl:120: [in] PP_Resource video_frame, Will Encode() allow to pass video frame coming out of a PPB_MediaStreamVideoTrack? https://codereview.chromium.org/842293003/diff/20001/ppapi/api/ppb_video_enco... ppapi/api/ppb_video_encoder.idl:137: }; Should there be a Reset() method? We could use Destroy() on media::VideoEncodeAccelerator to implement it.
https://codereview.chromium.org/842293003/diff/20001/ppapi/api/ppb_video_enco... File ppapi/api/ppb_video_encoder.idl (right): https://codereview.chromium.org/842293003/diff/20001/ppapi/api/ppb_video_enco... ppapi/api/ppb_video_encoder.idl:118: int32_t Encode( PPB_VideoDecoder has a |decode_id| parameter on its Decode() method : https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/c/ppb_video_... Any reason why the encoder wouldn't have that parameter too? https://codereview.chromium.org/842293003/diff/20001/ppapi/api/ppb_video_enco... ppapi/api/ppb_video_encoder.idl:120: [in] PP_Resource video_frame, Will Encode() allow to pass video frame coming out of a PPB_MediaStreamVideoTrack? https://codereview.chromium.org/842293003/diff/20001/ppapi/api/ppb_video_enco... ppapi/api/ppb_video_encoder.idl:137: }; Should there be a Reset() method? We could use Destroy() on media::VideoEncodeAccelerator to implement it.
https://codereview.chromium.org/842293003/diff/20001/ppapi/api/ppb_video_enco... File ppapi/api/ppb_video_encoder.idl (right): https://codereview.chromium.org/842293003/diff/20001/ppapi/api/ppb_video_enco... ppapi/api/ppb_video_encoder.idl:118: int32_t Encode( On 2015/01/12 11:48:04, llandwerlin wrote: > PPB_VideoDecoder has a |decode_id| parameter on its Decode() method : > > https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/c/ppb_video_... > Any reason why the encoder wouldn't have that parameter too? I don't see any way to pass in such id's to the encoder, so I'm not sure how we'd associate bitstream buffers with video frames. I'd be surprised if they were in 1:1 correspondence. https://code.google.com/p/chromium/codesearch#chromium/src/media/video/video_... https://codereview.chromium.org/842293003/diff/20001/ppapi/api/ppb_video_enco... ppapi/api/ppb_video_encoder.idl:120: [in] PP_Resource video_frame, On 2015/01/12 11:48:04, llandwerlin wrote: > Will Encode() allow to pass video frame coming out of a > PPB_MediaStreamVideoTrack? Great question. This seems like a common use case. I suppose we should allow the PPB_VideoEncoder implementation to accept PPB_VideoFrame resources that are not part of its own pool. I still think it should create its own pool though so the plugin doesn't have to create and manage pools of VideoFrame resources (as implied by the VEA::Client interface). Perhaps the implementation can lazily create the pool so we don't waste system resources if the plugin Configures the MSVT with enough frames to satisfy the Encoder. We would only create the pool of frames when the plugin called GetVideoFrame. https://codereview.chromium.org/842293003/diff/20001/ppapi/api/ppb_video_enco... ppapi/api/ppb_video_encoder.idl:137: }; On 2015/01/12 11:48:04, llandwerlin wrote: > Should there be a Reset() method? > We could use Destroy() on media::VideoEncodeAccelerator to implement it. I tried to match 1:1 with VideoDecodeAccelerator where possible. Is there a use case for this? 'Reset' and 'Flush' add a lot of complexity to PPB_VideoDecoder and it would be nice to avoid that.
https://codereview.chromium.org/842293003/diff/20001/ppapi/api/ppb_video_enco... File ppapi/api/ppb_video_encoder.idl (right): https://codereview.chromium.org/842293003/diff/20001/ppapi/api/ppb_video_enco... ppapi/api/ppb_video_encoder.idl:118: int32_t Encode( On 2015/01/12 18:38:53, bbudge wrote: > On 2015/01/12 11:48:04, llandwerlin wrote: > > PPB_VideoDecoder has a |decode_id| parameter on its Decode() method : > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/c/ppb_video_... > > Any reason why the encoder wouldn't have that parameter too? > > I don't see any way to pass in such id's to the encoder, so I'm not sure how > we'd associate bitstream buffers with video frames. I'd be surprised if they > were in 1:1 correspondence. > https://code.google.com/p/chromium/codesearch#chromium/src/media/video/video_... Looking at the Vaapi (Intel accelerator) code, I thought that would be the case, but I can't really tell for the other drivers. So nope. https://codereview.chromium.org/842293003/diff/20001/ppapi/api/ppb_video_enco... ppapi/api/ppb_video_encoder.idl:120: [in] PP_Resource video_frame, On 2015/01/12 18:38:53, bbudge wrote: > On 2015/01/12 11:48:04, llandwerlin wrote: > > Will Encode() allow to pass video frame coming out of a > > PPB_MediaStreamVideoTrack? > > Great question. > > This seems like a common use case. I suppose we should allow the > PPB_VideoEncoder implementation to accept PPB_VideoFrame resources that are not > part of its own pool. I still think it should create its own pool though so the > plugin doesn't have to create and manage pools of VideoFrame resources (as > implied by the VEA::Client interface). Perhaps the implementation can lazily > create the pool so we don't waste system resources if the plugin Configures the > MSVT with enough frames to satisfy the Encoder. We would only create the pool of > frames when the plugin called GetVideoFrame. Sounds good. https://codereview.chromium.org/842293003/diff/20001/ppapi/api/ppb_video_enco... ppapi/api/ppb_video_encoder.idl:137: }; On 2015/01/12 18:38:53, bbudge wrote: > On 2015/01/12 11:48:04, llandwerlin wrote: > > Should there be a Reset() method? > > We could use Destroy() on media::VideoEncodeAccelerator to implement it. > > I tried to match 1:1 with VideoDecodeAccelerator where possible. Is there a use > case for this? 'Reset' and 'Flush' add a lot of complexity to PPB_VideoDecoder > and it would be nice to avoid that. I'm thinking that if we allow frames from the MediaStreamVideoTrack to be submitted and at some point we want to stop the encoding and get the encoder to drop all the resources it's currently working with, we might need to call the Reset() method of the encoder before with return the video frame to the MediaStreamVideoTrack with RecycleFrame().
https://codereview.chromium.org/842293003/diff/20001/ppapi/api/ppb_video_enco... File ppapi/api/ppb_video_encoder.idl (right): https://codereview.chromium.org/842293003/diff/20001/ppapi/api/ppb_video_enco... ppapi/api/ppb_video_encoder.idl:113: int32_t GetVideoFrame( How would we like to have GetVideoFrame() behave when called too often? There will be a limited amount of frames allocated. In the case where all the frames have been submitted to the encoder, but none has returned, should we return PP_ERROR_NOMEMORY or PP_OK_COMPLETIONPENDING? I thinking the later would be better. Also, should we expose the number of frame allocated to the application?
dmichael@chromium.org changed reviewers: + dmichael@chromium.org
(haven't looked at C++) https://codereview.chromium.org/842293003/diff/20001/ppapi/api/pp_codecs.idl File ppapi/api/pp_codecs.idl (right): https://codereview.chromium.org/842293003/diff/20001/ppapi/api/pp_codecs.idl#... ppapi/api/pp_codecs.idl:140: uint32_t max_framerate_denominator; What's the reason for specifying the framerate this way? https://codereview.chromium.org/842293003/diff/20001/ppapi/api/pp_codecs.idl#... ppapi/api/pp_codecs.idl:155: mem_t buffer; So, you'll fill in this pointer, and it's probably shared memory? Recycle knows which shared buffer it can reuse based on the value of |buffer|? I can't think of a problem with it, just want to make sure I'm understanding. https://codereview.chromium.org/842293003/diff/20001/ppapi/api/ppb_video_enco... File ppapi/api/ppb_video_encoder.idl (right): https://codereview.chromium.org/842293003/diff/20001/ppapi/api/ppb_video_enco... ppapi/api/ppb_video_encoder.idl:115: [out] PP_Resource video_frame, I take it this is a PPB_VideoFrame? https://codereview.chromium.org/842293003/diff/20001/ppapi/api/ppb_video_enco... ppapi/api/ppb_video_encoder.idl:133: void RequestEncodingParametersChange( Does there need to be a CompletionCallback for this, so you know if it succeeds and when it takes effect?
https://codereview.chromium.org/842293003/diff/20001/ppapi/api/pp_codecs.idl File ppapi/api/pp_codecs.idl (right): https://codereview.chromium.org/842293003/diff/20001/ppapi/api/pp_codecs.idl#... ppapi/api/pp_codecs.idl:140: uint32_t max_framerate_denominator; On 2015/01/14 00:25:20, dmichael wrote: > What's the reason for specifying the framerate this way? That's how it's specified by media::VideoEncodeAccelerator. I think right now denominator is always 1 but I'm guessing that won't always be true: https://code.google.com/p/chromium/codesearch#chromium/src/media/video/video_... https://codereview.chromium.org/842293003/diff/20001/ppapi/api/pp_codecs.idl#... ppapi/api/pp_codecs.idl:155: mem_t buffer; On 2015/01/14 00:25:20, dmichael wrote: > So, you'll fill in this pointer, and it's probably shared memory? Recycle knows > which shared buffer it can reuse based on the value of |buffer|? I can't think > of a problem with it, just want to make sure I'm understanding. Yes. We could add a |buffer_id| field but I left it out since it's purely an implementation detail, with no real utility for the plugin. The buffer address should work as a UID. https://codereview.chromium.org/842293003/diff/20001/ppapi/api/ppb_video_enco... File ppapi/api/ppb_video_encoder.idl (right): https://codereview.chromium.org/842293003/diff/20001/ppapi/api/ppb_video_enco... ppapi/api/ppb_video_encoder.idl:113: int32_t GetVideoFrame( On 2015/01/13 12:43:55, llandwerlin wrote: > How would we like to have GetVideoFrame() behave when called too often? > There will be a limited amount of frames allocated. In the case where all the > frames have been submitted to the encoder, but none has returned, should we > return PP_ERROR_NOMEMORY or PP_OK_COMPLETIONPENDING? > > I thinking the later would be better. > > Also, should we expose the number of frame allocated to the application? This should work like PPB_VIdeoDecoder or PPB_MediaStreamVideoTrack, which throttle the plugin by returning PP_OK_COMPLETIONPENDING when no more calls to a function can be satisfied. In this case, the API would throttle the plugin's calls to GetVideoFrame by returning PP_OK_COMPLETIONPENDING when the last available frame is committed. The plugin gets unblocked when Encodes complete. It's not clear to me how VEA notifies the client about this though. As for returning the number of frames that the encoder requires, that points to a potential problem with sharing frames between PPB_MediaStreamVideoTrack and PPB_VideoEncoder. MSVT currently allows a maximum of 8 frames and it's possible that an encoder might require more than that number. In that case, the plugin would have to copy data from MSVT frames to VE frames (otherwise it could hang with all MSVT frames committed and no encodes complete.) That might imply that it's too difficult to allow sharing. I still need to ask whether this is a real-world use case. https://codereview.chromium.org/842293003/diff/20001/ppapi/api/ppb_video_enco... ppapi/api/ppb_video_encoder.idl:115: [out] PP_Resource video_frame, On 2015/01/14 00:25:20, dmichael wrote: > I take it this is a PPB_VideoFrame? Yes, owned by the proxy and loaned to the plugin, similarly to PPB_MediaStreamVideoTrack. https://codereview.chromium.org/842293003/diff/20001/ppapi/api/ppb_video_enco... ppapi/api/ppb_video_encoder.idl:133: void RequestEncodingParametersChange( On 2015/01/14 00:25:20, dmichael wrote: > Does there need to be a CompletionCallback for this, so you know if it succeeds > and when it takes effect? media::VideoEncodeAccelerator doesn't return a value or have a callback and the docs state it's a request only: https://code.google.com/p/chromium/codesearch#chromium/src/media/video/video_... https://codereview.chromium.org/842293003/diff/20001/ppapi/api/ppb_video_enco... ppapi/api/ppb_video_encoder.idl:137: }; On 2015/01/13 12:02:11, llandwerlin wrote: > On 2015/01/12 18:38:53, bbudge wrote: > > On 2015/01/12 11:48:04, llandwerlin wrote: > > > Should there be a Reset() method? > > > We could use Destroy() on media::VideoEncodeAccelerator to implement it. > > > > I tried to match 1:1 with VideoDecodeAccelerator where possible. Is there a > use > > case for this? 'Reset' and 'Flush' add a lot of complexity to PPB_VideoDecoder > > and it would be nice to avoid that. > > I'm thinking that if we allow frames from the MediaStreamVideoTrack to be > submitted and at some point we want to stop the encoding and get the encoder to > drop all the resources it's currently working with, we might need to call the > Reset() method of the encoder before with return the video frame to the > MediaStreamVideoTrack with RecycleFrame(). > I don't think VEA has a Reset method. See above for other difficulties w.r.t sharing frames.
https://codereview.chromium.org/842293003/diff/20001/ppapi/api/ppb_video_enco... File ppapi/api/ppb_video_encoder.idl (right): https://codereview.chromium.org/842293003/diff/20001/ppapi/api/ppb_video_enco... ppapi/api/ppb_video_encoder.idl:113: int32_t GetVideoFrame( On 2015/01/14 02:01:13, bbudge wrote: > On 2015/01/13 12:43:55, llandwerlin wrote: > > How would we like to have GetVideoFrame() behave when called too often? > > There will be a limited amount of frames allocated. In the case where all the > > frames have been submitted to the encoder, but none has returned, should we > > return PP_ERROR_NOMEMORY or PP_OK_COMPLETIONPENDING? > > > > I thinking the later would be better. > > > > Also, should we expose the number of frame allocated to the application? > > This should work like PPB_VIdeoDecoder or PPB_MediaStreamVideoTrack, which > throttle the plugin by returning PP_OK_COMPLETIONPENDING when no more calls to a > function can be satisfied. In this case, the API would throttle the plugin's > calls to GetVideoFrame by returning PP_OK_COMPLETIONPENDING when the last > available frame is committed. The plugin gets unblocked when Encodes complete. > It's not clear to me how VEA notifies the client about this though. > > As for returning the number of frames that the encoder requires, that points to > a potential problem with sharing frames between PPB_MediaStreamVideoTrack and > PPB_VideoEncoder. MSVT currently allows a maximum of 8 frames and it's possible > that an encoder might require more than that number. In that case, the plugin > would have to copy data from MSVT frames to VE frames (otherwise it could hang > with all MSVT frames committed and no encodes complete.) > > That might imply that it's too difficult to allow sharing. I still need to ask > whether this is a real-world use case. As far I as I can tell, the Vaapi/V4L2/Android encoder accelerators all copy the input frames into a format suitable for them (NV12), so with the current implementations we shouldn't encounter any blocking situation.
https://codereview.chromium.org/842293003/diff/20001/ppapi/api/ppb_video_enco... File ppapi/api/ppb_video_encoder.idl (right): https://codereview.chromium.org/842293003/diff/20001/ppapi/api/ppb_video_enco... ppapi/api/ppb_video_encoder.idl:113: int32_t GetVideoFrame( On 2015/01/14 11:56:13, llandwerlin wrote: > On 2015/01/14 02:01:13, bbudge wrote: > > On 2015/01/13 12:43:55, llandwerlin wrote: > > > How would we like to have GetVideoFrame() behave when called too often? > > > There will be a limited amount of frames allocated. In the case where all > the > > > frames have been submitted to the encoder, but none has returned, should we > > > return PP_ERROR_NOMEMORY or PP_OK_COMPLETIONPENDING? > > > > > > I thinking the later would be better. > > > > > > Also, should we expose the number of frame allocated to the application? > > > > This should work like PPB_VIdeoDecoder or PPB_MediaStreamVideoTrack, which > > throttle the plugin by returning PP_OK_COMPLETIONPENDING when no more calls to > a > > function can be satisfied. In this case, the API would throttle the plugin's > > calls to GetVideoFrame by returning PP_OK_COMPLETIONPENDING when the last > > available frame is committed. The plugin gets unblocked when Encodes complete. > > It's not clear to me how VEA notifies the client about this though. > > > > As for returning the number of frames that the encoder requires, that points > to > > a potential problem with sharing frames between PPB_MediaStreamVideoTrack and > > PPB_VideoEncoder. MSVT currently allows a maximum of 8 frames and it's > possible > > that an encoder might require more than that number. In that case, the plugin > > would have to copy data from MSVT frames to VE frames (otherwise it could hang > > with all MSVT frames committed and no encodes complete.) > > > > That might imply that it's too difficult to allow sharing. I still need to ask > > whether this is a real-world use case. > > As far I as I can tell, the Vaapi/V4L2/Android encoder accelerators all copy the > input frames into a format suitable for them (NV12), so with the current > implementations we shouldn't encounter any blocking situation. Here's a VEA that shares the frame with the GPU process, implying that we have to hold the frame for a while: https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu... It's not clear to me how the client can tell when the frame is safe to use again.
https://codereview.chromium.org/842293003/diff/20001/ppapi/api/ppb_video_enco... File ppapi/api/ppb_video_encoder.idl (right): https://codereview.chromium.org/842293003/diff/20001/ppapi/api/ppb_video_enco... ppapi/api/ppb_video_encoder.idl:113: int32_t GetVideoFrame( On 2015/01/14 16:38:10, bbudge wrote: > On 2015/01/14 11:56:13, llandwerlin wrote: > > On 2015/01/14 02:01:13, bbudge wrote: > > > On 2015/01/13 12:43:55, llandwerlin wrote: > > > > How would we like to have GetVideoFrame() behave when called too often? > > > > There will be a limited amount of frames allocated. In the case where all > > the > > > > frames have been submitted to the encoder, but none has returned, should > we > > > > return PP_ERROR_NOMEMORY or PP_OK_COMPLETIONPENDING? > > > > > > > > I thinking the later would be better. > > > > > > > > Also, should we expose the number of frame allocated to the application? > > > > > > This should work like PPB_VIdeoDecoder or PPB_MediaStreamVideoTrack, which > > > throttle the plugin by returning PP_OK_COMPLETIONPENDING when no more calls > to > > a > > > function can be satisfied. In this case, the API would throttle the plugin's > > > calls to GetVideoFrame by returning PP_OK_COMPLETIONPENDING when the last > > > available frame is committed. The plugin gets unblocked when Encodes > complete. > > > It's not clear to me how VEA notifies the client about this though. > > > > > > As for returning the number of frames that the encoder requires, that points > > to > > > a potential problem with sharing frames between PPB_MediaStreamVideoTrack > and > > > PPB_VideoEncoder. MSVT currently allows a maximum of 8 frames and it's > > possible > > > that an encoder might require more than that number. In that case, the > plugin > > > would have to copy data from MSVT frames to VE frames (otherwise it could > hang > > > with all MSVT frames committed and no encodes complete.) > > > > > > That might imply that it's too difficult to allow sharing. I still need to > ask > > > whether this is a real-world use case. > > > > As far I as I can tell, the Vaapi/V4L2/Android encoder accelerators all copy > the > > input frames into a format suitable for them (NV12), so with the current > > implementations we shouldn't encounter any blocking situation. > > Here's a VEA that shares the frame with the GPU process, implying that we have > to hold the frame for a while: > https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu... > > It's not clear to me how the client can tell when the frame is safe to use > again. The frame is kept in a hash table : https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu... It is released in GpuVideoEncodeAcceleratorHost::OnNotifyInputDone media::VideoFrame is created with a callback that is fired upon destruction, that's what the VEAClient should use to know when to recycle them.
https://codereview.chromium.org/842293003/diff/20001/ppapi/api/ppb_video_enco... File ppapi/api/ppb_video_encoder.idl (right): https://codereview.chromium.org/842293003/diff/20001/ppapi/api/ppb_video_enco... ppapi/api/ppb_video_encoder.idl:113: int32_t GetVideoFrame( On 2015/01/14 16:57:08, llandwerlin wrote: > On 2015/01/14 16:38:10, bbudge wrote: > > On 2015/01/14 11:56:13, llandwerlin wrote: > > > On 2015/01/14 02:01:13, bbudge wrote: > > > > On 2015/01/13 12:43:55, llandwerlin wrote: > > > > > How would we like to have GetVideoFrame() behave when called too often? > > > > > There will be a limited amount of frames allocated. In the case where > all > > > the > > > > > frames have been submitted to the encoder, but none has returned, should > > we > > > > > return PP_ERROR_NOMEMORY or PP_OK_COMPLETIONPENDING? > > > > > > > > > > I thinking the later would be better. > > > > > > > > > > Also, should we expose the number of frame allocated to the application? > > > > > > > > This should work like PPB_VIdeoDecoder or PPB_MediaStreamVideoTrack, which > > > > throttle the plugin by returning PP_OK_COMPLETIONPENDING when no more > calls > > to > > > a > > > > function can be satisfied. In this case, the API would throttle the > plugin's > > > > calls to GetVideoFrame by returning PP_OK_COMPLETIONPENDING when the last > > > > available frame is committed. The plugin gets unblocked when Encodes > > complete. > > > > It's not clear to me how VEA notifies the client about this though. > > > > > > > > As for returning the number of frames that the encoder requires, that > points > > > to > > > > a potential problem with sharing frames between PPB_MediaStreamVideoTrack > > and > > > > PPB_VideoEncoder. MSVT currently allows a maximum of 8 frames and it's > > > possible > > > > that an encoder might require more than that number. In that case, the > > plugin > > > > would have to copy data from MSVT frames to VE frames (otherwise it could > > hang > > > > with all MSVT frames committed and no encodes complete.) > > > > > > > > That might imply that it's too difficult to allow sharing. I still need to > > ask > > > > whether this is a real-world use case. > > > > > > As far I as I can tell, the Vaapi/V4L2/Android encoder accelerators all copy > > the > > > input frames into a format suitable for them (NV12), so with the current > > > implementations we shouldn't encounter any blocking situation. > > > > Here's a VEA that shares the frame with the GPU process, implying that we have > > to hold the frame for a while: > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu... > > > > It's not clear to me how the client can tell when the frame is safe to use > > again. > > The frame is kept in a hash table : > > https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu... > > It is released in GpuVideoEncodeAcceleratorHost::OnNotifyInputDone > > media::VideoFrame is created with a callback that is fired upon destruction, > that's what the VEAClient should use to know when to recycle them. Thanks for the explanation! Then we can distinguish between our frame or a MSVT frame. I need to add an 'out' param for Encode so the plugin can know when to recycle a MSVT frame.
https://codereview.chromium.org/842293003/diff/20001/ppapi/api/ppb_video_enco... File ppapi/api/ppb_video_encoder.idl (right): https://codereview.chromium.org/842293003/diff/20001/ppapi/api/ppb_video_enco... ppapi/api/ppb_video_encoder.idl:113: int32_t GetVideoFrame( On 2015/01/14 17:20:11, bbudge wrote: > On 2015/01/14 16:57:08, llandwerlin wrote: > > On 2015/01/14 16:38:10, bbudge wrote: > > > On 2015/01/14 11:56:13, llandwerlin wrote: > > > > On 2015/01/14 02:01:13, bbudge wrote: > > > > > On 2015/01/13 12:43:55, llandwerlin wrote: > > > > > > How would we like to have GetVideoFrame() behave when called too > often? > > > > > > There will be a limited amount of frames allocated. In the case where > > all > > > > the > > > > > > frames have been submitted to the encoder, but none has returned, > should > > > we > > > > > > return PP_ERROR_NOMEMORY or PP_OK_COMPLETIONPENDING? > > > > > > > > > > > > I thinking the later would be better. > > > > > > > > > > > > Also, should we expose the number of frame allocated to the > application? > > > > > > > > > > This should work like PPB_VIdeoDecoder or PPB_MediaStreamVideoTrack, > which > > > > > throttle the plugin by returning PP_OK_COMPLETIONPENDING when no more > > calls > > > to > > > > a > > > > > function can be satisfied. In this case, the API would throttle the > > plugin's > > > > > calls to GetVideoFrame by returning PP_OK_COMPLETIONPENDING when the > last > > > > > available frame is committed. The plugin gets unblocked when Encodes > > > complete. > > > > > It's not clear to me how VEA notifies the client about this though. > > > > > > > > > > As for returning the number of frames that the encoder requires, that > > points > > > > to > > > > > a potential problem with sharing frames between > PPB_MediaStreamVideoTrack > > > and > > > > > PPB_VideoEncoder. MSVT currently allows a maximum of 8 frames and it's > > > > possible > > > > > that an encoder might require more than that number. In that case, the > > > plugin > > > > > would have to copy data from MSVT frames to VE frames (otherwise it > could > > > hang > > > > > with all MSVT frames committed and no encodes complete.) > > > > > > > > > > That might imply that it's too difficult to allow sharing. I still need > to > > > ask > > > > > whether this is a real-world use case. > > > > > > > > As far I as I can tell, the Vaapi/V4L2/Android encoder accelerators all > copy > > > the > > > > input frames into a format suitable for them (NV12), so with the current > > > > implementations we shouldn't encounter any blocking situation. > > > > > > Here's a VEA that shares the frame with the GPU process, implying that we > have > > > to hold the frame for a while: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu... > > > > > > It's not clear to me how the client can tell when the frame is safe to use > > > again. > > > > The frame is kept in a hash table : > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu... > > > > It is released in GpuVideoEncodeAcceleratorHost::OnNotifyInputDone > > > > media::VideoFrame is created with a callback that is fired upon destruction, > > that's what the VEAClient should use to know when to recycle them. > > Thanks for the explanation! Then we can distinguish between our frame or a MSVT > frame. I need to add an 'out' param for Encode so the plugin can know when to > recycle a MSVT frame. Maybe I'm missing something, but I would have thought the callback was enough. We just need to map the frames to the callbacks in the proxy and call accordingly.
Added a 'key_frame' param to PP_BitstreamBuffer. Added a GetInputFramesRequired function. More comments. https://codereview.chromium.org/842293003/diff/20001/ppapi/api/ppb_video_enco... File ppapi/api/ppb_video_encoder.idl (right): https://codereview.chromium.org/842293003/diff/20001/ppapi/api/ppb_video_enco... ppapi/api/ppb_video_encoder.idl:113: int32_t GetVideoFrame( On 2015/01/14 20:21:26, llandwerlin wrote: > On 2015/01/14 17:20:11, bbudge wrote: > > On 2015/01/14 16:57:08, llandwerlin wrote: > > > On 2015/01/14 16:38:10, bbudge wrote: > > > > On 2015/01/14 11:56:13, llandwerlin wrote: > > > > > On 2015/01/14 02:01:13, bbudge wrote: > > > > > > On 2015/01/13 12:43:55, llandwerlin wrote: > > > > > > > How would we like to have GetVideoFrame() behave when called too > > often? > > > > > > > There will be a limited amount of frames allocated. In the case > where > > > all > > > > > the > > > > > > > frames have been submitted to the encoder, but none has returned, > > should > > > > we > > > > > > > return PP_ERROR_NOMEMORY or PP_OK_COMPLETIONPENDING? > > > > > > > > > > > > > > I thinking the later would be better. > > > > > > > > > > > > > > Also, should we expose the number of frame allocated to the > > application? > > > > > > > > > > > > This should work like PPB_VIdeoDecoder or PPB_MediaStreamVideoTrack, > > which > > > > > > throttle the plugin by returning PP_OK_COMPLETIONPENDING when no more > > > calls > > > > to > > > > > a > > > > > > function can be satisfied. In this case, the API would throttle the > > > plugin's > > > > > > calls to GetVideoFrame by returning PP_OK_COMPLETIONPENDING when the > > last > > > > > > available frame is committed. The plugin gets unblocked when Encodes > > > > complete. > > > > > > It's not clear to me how VEA notifies the client about this though. > > > > > > > > > > > > As for returning the number of frames that the encoder requires, that > > > points > > > > > to > > > > > > a potential problem with sharing frames between > > PPB_MediaStreamVideoTrack > > > > and > > > > > > PPB_VideoEncoder. MSVT currently allows a maximum of 8 frames and it's > > > > > possible > > > > > > that an encoder might require more than that number. In that case, the > > > > plugin > > > > > > would have to copy data from MSVT frames to VE frames (otherwise it > > could > > > > hang > > > > > > with all MSVT frames committed and no encodes complete.) > > > > > > > > > > > > That might imply that it's too difficult to allow sharing. I still > need > > to > > > > ask > > > > > > whether this is a real-world use case. > > > > > > > > > > As far I as I can tell, the Vaapi/V4L2/Android encoder accelerators all > > copy > > > > the > > > > > input frames into a format suitable for them (NV12), so with the current > > > > > implementations we shouldn't encounter any blocking situation. > > > > > > > > Here's a VEA that shares the frame with the GPU process, implying that we > > have > > > > to hold the frame for a while: > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu... > > > > > > > > It's not clear to me how the client can tell when the frame is safe to use > > > > again. > > > > > > The frame is kept in a hash table : > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu... > > > > > > It is released in GpuVideoEncodeAcceleratorHost::OnNotifyInputDone > > > > > > media::VideoFrame is created with a callback that is fired upon destruction, > > > that's what the VEAClient should use to know when to recycle them. > > > > Thanks for the explanation! Then we can distinguish between our frame or a > MSVT > > frame. I need to add an 'out' param for Encode so the plugin can know when to > > recycle a MSVT frame. > > Maybe I'm missing something, but I would have thought the callback was enough. > We just need to map the frames to the callbacks in the proxy and call > accordingly. You're correct, the plugin can keep track of which frame it sent with the callback.
https://codereview.chromium.org/842293003/diff/40001/ppapi/api/ppb_video_enco... File ppapi/api/ppb_video_encoder.idl (right): https://codereview.chromium.org/842293003/diff/40001/ppapi/api/ppb_video_enco... ppapi/api/ppb_video_encoder.idl:79: int32_t GetSupportedProfiles( Do we need to prevent concurrent calls to this method? https://codereview.chromium.org/842293003/diff/40001/ppapi/api/ppb_video_enco... ppapi/api/ppb_video_encoder.idl:177: */ We should add that multiple calls without waiting for the callback to be fired will trigger an error, like GetPicture() on PPB_VideoDecoder : https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/api/ppb_vide...
https://codereview.chromium.org/842293003/diff/40001/ppapi/api/ppb_video_enco... File ppapi/api/ppb_video_encoder.idl (right): https://codereview.chromium.org/842293003/diff/40001/ppapi/api/ppb_video_enco... ppapi/api/ppb_video_encoder.idl:125: int32_t GetInputFramesRequired( We should also add here that it's going to fail unless Initialize() has succeeded. https://codereview.chromium.org/842293003/diff/40001/ppapi/api/ppb_video_enco... ppapi/api/ppb_video_encoder.idl:140: int32_t GetVideoFrame( We should also add here that it's going to fail unless Initialize() has succeeded. https://codereview.chromium.org/842293003/diff/40001/ppapi/api/ppb_video_enco... ppapi/api/ppb_video_encoder.idl:159: int32_t Encode( Will fail unless Initialize succeeds.
https://codereview.chromium.org/842293003/diff/40001/ppapi/api/ppb_video_enco... File ppapi/api/ppb_video_encoder.idl (right): https://codereview.chromium.org/842293003/diff/40001/ppapi/api/ppb_video_enco... ppapi/api/ppb_video_encoder.idl:109: [in] PP_Size input_visible_size, I forgot, but I think we might also need to add a method to report the input_coded_size from media::VideoEncodeAccelerator : https://code.google.com/p/chromium/codesearch#chromium/src/media/video/video_...
https://codereview.chromium.org/842293003/diff/40001/ppapi/api/ppb_video_enco... File ppapi/api/ppb_video_encoder.idl (right): https://codereview.chromium.org/842293003/diff/40001/ppapi/api/ppb_video_enco... ppapi/api/ppb_video_encoder.idl:79: int32_t GetSupportedProfiles( On 2015/01/15 11:19:15, llandwerlin wrote: > Do we need to prevent concurrent calls to this method? It's simpler, so yes. We can return INPROGRESS if they call again before completion. https://codereview.chromium.org/842293003/diff/40001/ppapi/api/ppb_video_enco... ppapi/api/ppb_video_encoder.idl:109: [in] PP_Size input_visible_size, On 2015/01/15 11:32:53, llandwerlin wrote: > I forgot, but I think we might also need to add a method to report the > input_coded_size from media::VideoEncodeAccelerator : > > https://code.google.com/p/chromium/codesearch#chromium/src/media/video/video_... This brings up some interesting questions: 1) Can VEA::Client::RequireBitstreamBuffers be called multiple times (i.e. not just at Initialization)? 2) Is input_coded_size useful if the client is encoding frames from MediaStreamVideoTrack? 3) Does PPB_VideoFrame expose this or would we have to modify it?
https://codereview.chromium.org/842293003/diff/40001/ppapi/api/ppb_video_enco... File ppapi/api/ppb_video_encoder.idl (right): https://codereview.chromium.org/842293003/diff/40001/ppapi/api/ppb_video_enco... ppapi/api/ppb_video_encoder.idl:109: [in] PP_Size input_visible_size, On 2015/01/15 12:31:50, bbudge wrote: > On 2015/01/15 11:32:53, llandwerlin wrote: > > I forgot, but I think we might also need to add a method to report the > > input_coded_size from media::VideoEncodeAccelerator : > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/media/video/video_... > > This brings up some interesting questions: > 1) Can VEA::Client::RequireBitstreamBuffers be called multiple times (i.e. not > just at Initialization)? > 2) Is input_coded_size useful if the client is encoding frames from > MediaStreamVideoTrack? > 3) Does PPB_VideoFrame expose this or would we have to modify it? 1) Only at initialization. 2) My understanding is that the hardware is going to work on frames of the same size or slightly larger (depending on the constraints) to not loose any content from the input frames. The bitstream will contain informations about how to crop decoded frames so the decoder can display frames correctly. It seems we need to match the hardware's constraints. The thing is, the frames are going to be copied again (in the gpu-process) because most hardwares don't work with I420 (which is what chromium always uses), but NV12 instead. So in practice, the constraints probably don't matter. That being said, input_coded_size is useful to reconfigure the MediaStreamVideoTrack at the right size : https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/api/ppb_medi... 3) With the current implementation, the width of the PPB_VideoFrame is equal to its stride, so I don't think we need more information about that class.
https://codereview.chromium.org/842293003/diff/40001/ppapi/api/ppb_video_enco... File ppapi/api/ppb_video_encoder.idl (right): https://codereview.chromium.org/842293003/diff/40001/ppapi/api/ppb_video_enco... ppapi/api/ppb_video_encoder.idl:125: int32_t GetInputFramesRequired( On 2015/01/15 11:22:27, llandwerlin wrote: > We should also add here that it's going to fail unless Initialize() has > succeeded. Done. https://codereview.chromium.org/842293003/diff/40001/ppapi/api/ppb_video_enco... ppapi/api/ppb_video_encoder.idl:140: int32_t GetVideoFrame( On 2015/01/15 11:22:28, llandwerlin wrote: > We should also add here that it's going to fail unless Initialize() has > succeeded. Done. https://codereview.chromium.org/842293003/diff/40001/ppapi/api/ppb_video_enco... ppapi/api/ppb_video_encoder.idl:159: int32_t Encode( On 2015/01/15 11:22:28, llandwerlin wrote: > Will fail unless Initialize succeeds. Done. https://codereview.chromium.org/842293003/diff/40001/ppapi/api/ppb_video_enco... ppapi/api/ppb_video_encoder.idl:177: */ On 2015/01/15 11:19:15, llandwerlin wrote: > We should add that multiple calls without waiting for the callback to be fired > will trigger an error, like GetPicture() on PPB_VideoDecoder : > > https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/api/ppb_vide... Done.
https://codereview.chromium.org/842293003/diff/60001/ppapi/api/pp_codecs.idl File ppapi/api/pp_codecs.idl (right): https://codereview.chromium.org/842293003/diff/60001/ppapi/api/pp_codecs.idl#... ppapi/api/pp_codecs.idl:123: struct PP_SupportedVideoProfile { While implementing this, I figured that maybe we should indicate which profiles are either hardware or software accelerated. Should we add a boolean in this struct?
https://codereview.chromium.org/842293003/diff/60001/ppapi/api/pp_codecs.idl File ppapi/api/pp_codecs.idl (right): https://codereview.chromium.org/842293003/diff/60001/ppapi/api/pp_codecs.idl#... ppapi/api/pp_codecs.idl:123: struct PP_SupportedVideoProfile { On 2015/01/26 19:25:47, llandwerlin wrote: > While implementing this, I figured that maybe we should indicate which profiles > are either hardware or software accelerated. > > Should we add a boolean in this struct? That seems reasonable. VEA implements GetSupportedProfiles, but we need to figure out how the software encoder stuff in Chromium can implement the same thing.
https://codereview.chromium.org/842293003/diff/60001/ppapi/api/pp_codecs.idl File ppapi/api/pp_codecs.idl (right): https://codereview.chromium.org/842293003/diff/60001/ppapi/api/pp_codecs.idl#... ppapi/api/pp_codecs.idl:123: struct PP_SupportedVideoProfile { On 2015/01/26 20:34:32, bbudge wrote: > On 2015/01/26 19:25:47, llandwerlin wrote: > > While implementing this, I figured that maybe we should indicate which > profiles > > are either hardware or software accelerated. > > > > Should we add a boolean in this struct? > > That seems reasonable. VEA implements GetSupportedProfiles, but we need to > figure out how the software encoder stuff in Chromium can implement the same > thing. Instead of a PP_Bool, I added a PP_HardwareAcceleration value, to convey whether the profile is available in HW, SW, or both.
https://codereview.chromium.org/842293003/diff/80001/ppapi/api/pp_codecs.idl File ppapi/api/pp_codecs.idl (right): https://codereview.chromium.org/842293003/diff/80001/ppapi/api/pp_codecs.idl#... ppapi/api/pp_codecs.idl:137: uint32_t max_framerate_numerator; Why a fraction? Would a float or double suffice? I notice that Android uses a float or integer... http://developer.android.com/reference/android/media/MediaFormat.html#KEY_FRA... https://codereview.chromium.org/842293003/diff/80001/ppapi/api/ppb_video_enco... File ppapi/api/ppb_video_encoder.idl (right): https://codereview.chromium.org/842293003/diff/80001/ppapi/api/ppb_video_enco... ppapi/api/ppb_video_encoder.idl:51: [in] PP_Instance instance); tiny nit: I don't know why we do this style in so many of the IDL files, but I'd rather we styled them the same as we do C/C++ code. I.e., if the parameter fits, put it on the same line as the function declaration. https://codereview.chromium.org/842293003/diff/80001/ppapi/api/ppb_video_enco... ppapi/api/ppb_video_encoder.idl:71: * @param[in] output A <code>PP_ArrayOutput</code> to hold the supported nit: s/hold/receive? The ArrayOutput doesn't really hold it, so that wording might be confusing... https://codereview.chromium.org/842293003/diff/80001/ppapi/api/ppb_video_enco... ppapi/api/ppb_video_encoder.idl:132: * |input_visible_size|, as requested in the call to Initialize(). Does that mean that the plugin is responsible for scaling the frames? It would be good to not require that, but if the scaling is done by the browser, I'm not sure why this function would be needed. https://codereview.chromium.org/842293003/diff/80001/ppapi/api/ppb_video_enco... ppapi/api/ppb_video_encoder.idl:167: * @param[in] video_frame The <code>PPB_VideoFrame</code> to be encoded. Do we "neuter" the frame resource so that the plugin can't do anything with it anymore? That would seem like a nice property, so that they can't write to the frame while it's in transit? https://codereview.chromium.org/842293003/diff/80001/ppapi/api/ppb_video_enco... ppapi/api/ppb_video_encoder.idl:229: }; We may want a Close() (or Reset) to make it easy to stop encoding and cause callbacks to Abort. It seems to be a useful pattern in other APIs, since it might not always be as convenient for the plugin to get the ref-count to zero (e.g., if a ref-count is owned on another thread). https://codereview.chromium.org/842293003/diff/80001/ppapi/c/pp_codecs.h File ppapi/c/pp_codecs.h (right): https://codereview.chromium.org/842293003/diff/80001/ppapi/c/pp_codecs.h#newc... ppapi/c/pp_codecs.h:139: struct PP_SupportedVideoProfile { nit: I think whether it's "Supported" is more a property of a value returned by GetSupportedProfiles than it is the type (Though I'm not sure if we'd use the type for anything else). Nonetheless... a name like PP_VidepProfileDescription might be better? https://codereview.chromium.org/842293003/diff/80001/ppapi/cpp/video_encoder.cc File ppapi/cpp/video_encoder.cc (right): https://codereview.chromium.org/842293003/diff/80001/ppapi/cpp/video_encoder.... ppapi/cpp/video_encoder.cc:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. super nitty nit: prefer no (c) (here and other new files)
https://codereview.chromium.org/842293003/diff/80001/ppapi/api/ppb_video_enco... File ppapi/api/ppb_video_encoder.idl (right): https://codereview.chromium.org/842293003/diff/80001/ppapi/api/ppb_video_enco... ppapi/api/ppb_video_encoder.idl:132: * |input_visible_size|, as requested in the call to Initialize(). On 2015/02/03 23:26:08, dmichael wrote: > Does that mean that the plugin is responsible for scaling the frames? It would > be good to not require that, but if the scaling is done by the browser, I'm not > sure why this function would be needed. That means that frames returned through GetVideoFrame might be a bit bigger to accommodate the hardware constraints. Meaning that the remaining width/height will be cropped. Therefore, when you write into frames returned from GetVideoFrame, you should take care of some kind of "stride" which is the coded size. https://codereview.chromium.org/842293003/diff/80001/ppapi/api/ppb_video_enco... ppapi/api/ppb_video_encoder.idl:167: * @param[in] video_frame The <code>PPB_VideoFrame</code> to be encoded. On 2015/02/03 23:26:08, dmichael wrote: > Do we "neuter" the frame resource so that the plugin can't do anything with it > anymore? That would seem like a nice property, so that they can't write to the > frame while it's in transit? That's currently what the implementation does, the frame is invalidated after Encode() returns. While this prevents any call on the video_frame resource to return any valid value, if you've keep a pointer to the buffer prior to call Encode, you might still be able to write to the frame before it's actually encoded. https://codereview.chromium.org/842293003/diff/80001/ppapi/api/ppb_video_enco... ppapi/api/ppb_video_encoder.idl:229: }; On 2015/02/03 23:26:08, dmichael wrote: > We may want a Close() (or Reset) to make it easy to stop encoding and cause > callbacks to Abort. It seems to be a useful pattern in other APIs, since it > might not always be as convenient for the plugin to get the ref-count to zero > (e.g., if a ref-count is owned on another thread). I've argued for that, but it's up to bbudge@. https://codereview.chromium.org/842293003/diff/80001/ppapi/c/pp_codecs.h File ppapi/c/pp_codecs.h (right): https://codereview.chromium.org/842293003/diff/80001/ppapi/c/pp_codecs.h#newc... ppapi/c/pp_codecs.h:139: struct PP_SupportedVideoProfile { On 2015/02/03 23:26:08, dmichael wrote: > nit: I think whether it's "Supported" is more a property of a value returned by > GetSupportedProfiles than it is the type (Though I'm not sure if we'd use the > type for anything else). > > Nonetheless... a name like PP_VidepProfileDescription might be better? Acknowledged.
https://codereview.chromium.org/842293003/diff/80001/ppapi/api/pp_codecs.idl File ppapi/api/pp_codecs.idl (right): https://codereview.chromium.org/842293003/diff/80001/ppapi/api/pp_codecs.idl#... ppapi/api/pp_codecs.idl:137: uint32_t max_framerate_numerator; On 2015/02/03 23:26:07, dmichael wrote: > Why a fraction? Would a float or double suffice? > > I notice that Android uses a float or integer... > http://developer.android.com/reference/android/media/MediaFormat.html#KEY_FRA... This is copied from the VEA interface. I think it's the correct way to specify frame rate because the encoder uses exact arithmetic internally, and floats or even doubles can't represent these ratios well. See: http://avisynth.nl/index.php/FPS https://codereview.chromium.org/842293003/diff/80001/ppapi/api/ppb_video_enco... File ppapi/api/ppb_video_encoder.idl (right): https://codereview.chromium.org/842293003/diff/80001/ppapi/api/ppb_video_enco... ppapi/api/ppb_video_encoder.idl:51: [in] PP_Instance instance); On 2015/02/03 23:26:08, dmichael wrote: > tiny nit: I don't know why we do this style in so many of the IDL files, but I'd > rather we styled them the same as we do C/C++ code. I.e., if the parameter fits, > put it on the same line as the function declaration. Done. https://codereview.chromium.org/842293003/diff/80001/ppapi/api/ppb_video_enco... ppapi/api/ppb_video_encoder.idl:71: * @param[in] output A <code>PP_ArrayOutput</code> to hold the supported On 2015/02/03 23:26:08, dmichael wrote: > nit: s/hold/receive? > The ArrayOutput doesn't really hold it, so that wording might be confusing... Done. https://codereview.chromium.org/842293003/diff/80001/ppapi/api/ppb_video_enco... ppapi/api/ppb_video_encoder.idl:229: }; On 2015/02/03 23:26:08, dmichael wrote: > We may want a Close() (or Reset) to make it easy to stop encoding and cause > callbacks to Abort. It seems to be a useful pattern in other APIs, since it > might not always be as convenient for the plugin to get the ref-count to zero > (e.g., if a ref-count is owned on another thread). We've talked about a Destroy() method, but that isn't consistent with Pepper conventions. Reset would be confusing, as the VideoDecoder api has that method, but it means something else. I added a Close method, which can call the VEA::Destroy method. https://codereview.chromium.org/842293003/diff/80001/ppapi/c/pp_codecs.h File ppapi/c/pp_codecs.h (right): https://codereview.chromium.org/842293003/diff/80001/ppapi/c/pp_codecs.h#newc... ppapi/c/pp_codecs.h:139: struct PP_SupportedVideoProfile { On 2015/02/03 23:26:08, dmichael wrote: > nit: I think whether it's "Supported" is more a property of a value returned by > GetSupportedProfiles than it is the type (Though I'm not sure if we'd use the > type for anything else). > > Nonetheless... a name like PP_VidepProfileDescription might be better? This tries to follow the VEA api, where the class is VEA::SupportedProfile. https://code.google.com/p/chromium/codesearch#chromium/src/media/video/video_... This type may in future be used by VideoDecoder, as there's an outstanding bug to add similar querying there. https://code.google.com/p/chromium/issues/detail?id=436213 PP_VideoProfileDescription is a good name too. Done.
lgtm, thanks! https://codereview.chromium.org/842293003/diff/80001/ppapi/api/ppb_video_enco... File ppapi/api/ppb_video_encoder.idl (right): https://codereview.chromium.org/842293003/diff/80001/ppapi/api/ppb_video_enco... ppapi/api/ppb_video_encoder.idl:167: * @param[in] video_frame The <code>PPB_VideoFrame</code> to be encoded. On 2015/02/04 01:01:52, llandwerlin wrote: > On 2015/02/03 23:26:08, dmichael wrote: > > Do we "neuter" the frame resource so that the plugin can't do anything with it > > anymore? That would seem like a nice property, so that they can't write to the > > frame while it's in transit? > > That's currently what the implementation does, the frame is invalidated after > Encode() returns. > While this prevents any call on the video_frame resource to return any valid > value, if you've keep a pointer to the buffer prior to call Encode, you might > still be able to write to the frame before it's actually encoded. Makes sense. I'm glad there's at least some protection against mistakes.
The CQ bit was checked by bbudge@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/842293003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by bbudge@chromium.org
The CQ bit was unchecked by bbudge@chromium.org
bbudge@chromium.org changed reviewers: + sbc@chromium.org
+Sam for nacl_sdk
bbudge@chromium.org changed reviewers: + binji@chromium.org
or Ben for nacl_sdk
On 2015/02/04 18:34:24, bbudge wrote: > or Ben for nacl_sdk native_client_sdk lgtm
The CQ bit was checked by bbudge@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/842293003/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/3abfda6fae03fb234224df6e69337a818b786b3a Cr-Commit-Position: refs/heads/master@{#314645}
Message was sent while issue was closed.
https://codereview.chromium.org/842293003/diff/80001/ppapi/cpp/video_encoder.cc File ppapi/cpp/video_encoder.cc (right): https://codereview.chromium.org/842293003/diff/80001/ppapi/cpp/video_encoder.... ppapi/cpp/video_encoder.cc:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. On 2015/02/03 23:26:08, dmichael wrote: > super nitty nit: prefer no (c) (here and other new files) Whoops, I'll get this in a follow-on CL. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
