Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2827052:
Add pepper C API for video decode (Closed)
|
Created:
10 years, 5 months ago by wjia(left Chromium) Modified:
9 years, 7 months ago CC:
chromium-reviews Base URL:
http://ppapi.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionAdd pepper C API for video decode
BUG=none
TEST=compiles
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=178
Patch Set 1 #
Total comments: 51
Patch Set 2 : '' #
Total comments: 11
Patch Set 3 : '' #
Total comments: 16
Patch Set 4 : '' #
Total comments: 7
Patch Set 5 : add PP_Instance in GetConfig, more change from review #Patch Set 6 : several change from review #Messages
Total messages: 20 (0 generated)
The first draft of video decode pepper API. Feel free to comment on it.
So this is to expose software video decode, right? And then our hardware accelerated video would use totally separate code paths? Did you have any particular use case in mind? This could be useful if a plugin author doesn't want to ship codecs with their plugin, I just want to be sure everybody is on the same page. http://codereview.chromium.org/2827052/diff/1/2 File c/pp_video.h (right): http://codereview.chromium.org/2827052/diff/1/2#newcode13 c/pp_video.h:13: PP_VIDEODECODEID_NONE, Can you give enums explicit values? We want this to be consistent across all compilers, so just starting with the first one "= 0" should be sufficient. Can you make sure the enum values match the name of the enum? In this case, it would be PP_VIDEOCODECID_... http://codereview.chromium.org/2827052/diff/1/2#newcode27 c/pp_video.h:27: PP_VIDEOCODECPROFIL_NONE, Forgot an E at the end of PROFILE for all these names. http://codereview.chromium.org/2827052/diff/1/2#newcode141 c/pp_video.h:141: int32_t count; What is the likelihood do you think we'll add more stuff here to the config? Once the API is "frozen" then we can'd add more stuff to this structure anyway, and would require a new config function and struct on a new interface. Because of this, if you don't have immediate plans to add more stuff here, I'd just pass buffer_count as an arg when you need it and delete this struct. http://codereview.chromium.org/2827052/diff/1/3 File c/ppb_video_decode.h (right): http://codereview.chromium.org/2827052/diff/1/3#newcode26 c/ppb_video_decode.h:26: // Returns true if the given resource is a video decoder. Returns false if the Looks like this comment should go on an Is... function. http://codereview.chromium.org/2827052/diff/1/3#newcode42 c/ppb_video_decode.h:42: PP_VideoFrameInfo* frame_info); We should make the asynchronous completion here work the same as URLLoader. In that case, it takes a completion callback as an argument (this makes it very easy for the plugin to route the notifications properly). The callback just notifies you that video is ready, and then you call a function to actually get the data. This may mean you can delete the while PPP_ interface. http://codereview.chromium.org/2827052/diff/1/4 File c/ppp_video_decode.h (right): http://codereview.chromium.org/2827052/diff/1/4#newcode13 c/ppp_video_decode.h:13: typedef struct _ppp_VideoDecode { I think the classes/files should be called "VideoDecoder" (rather than "VideoDecode") which matches the existing "URLLoader"
http://codereview.chromium.org/2827052/diff/1/2 File c/pp_video.h (right): http://codereview.chromium.org/2827052/diff/1/2#newcode16 c/pp_video.h:16: PP_VIDEODECODEID_MPEG2, Do we want to support MPEG2 and VC1? The browser now only supports decoding of H264, VP8 and Theora officially, are we going to provide more codecs for pepper? http://codereview.chromium.org/2827052/diff/1/2#newcode84 c/pp_video.h:84: PP_VIDEOPAYLOADFORMAT_BYTESTREAM, Instead of just having bytestream I think we should limit our scope of support and be explicit about the bytestream format too, like H264 is Annex B, VP8 is whatever is standard. http://codereview.chromium.org/2827052/diff/1/2#newcode85 c/pp_video.h:85: PP_VIDEOPAYLOADFORMAT_RTPPAYLOAD, Is it part of the plan for pepper video to support RTP payload? I feel like the pepper video decoder should support only byte stream and plugin will do the necessary conversion. http://codereview.chromium.org/2827052/diff/1/2#newcode92 c/pp_video.h:92: PP_VIDEOFRAMECOLORTYPE_YUV, I don't think YUV is explicit enough. http://codereview.chromium.org/2827052/diff/1/2#newcode102 c/pp_video.h:102: PP_VIDEOFRAMESURFACETYPE_PIXELMAP, Do you mean Pixmap? http://codereview.chromium.org/2827052/diff/1/2#newcode108 c/pp_video.h:108: PP_VIDEOFRAMEINFOFLAG_NOEMIT = 1 << 1, Can you put a bit more comments about these flags? http://codereview.chromium.org/2827052/diff/1/2#newcode122 c/pp_video.h:122: bool accelerated; Is it part of the plan to provide SW decoder if HW decoder is not available for specified codec / profile / level? I think we should let the plugin to provide SW decoder. http://codereview.chromium.org/2827052/diff/1/2#newcode141 c/pp_video.h:141: int32_t count; Does this mean each input buffer can contains multiple frames? http://codereview.chromium.org/2827052/diff/1/3 File c/ppb_video_decode.h (right): http://codereview.chromium.org/2827052/diff/1/3#newcode31 c/ppb_video_decode.h:31: PP_VideoDecodeInputBufferConfig* (*GetVideoDecodeInputBufferConfig)( InputBufferConfig only returns count, how is it used? http://codereview.chromium.org/2827052/diff/1/3#newcode40 c/ppb_video_decode.h:40: bool (*Decode)(PP_Resource resource, This looks like Decode() will take ownership of |input_buffer| and there is not recycling or fixed amount of input buffers? right? Also how can we get a signal that a frame is ready? http://codereview.chromium.org/2827052/diff/1/4 File c/ppp_video_decode.h (right): http://codereview.chromium.org/2827052/diff/1/4#newcode19 c/ppp_video_decode.h:19: // Return compressed data buffer to plugin. This is optional. It is not very clear how this method is used? If the default way of using the decoder is that it takes ownership of input buffer, we shouldn't have a second mode of operation since it creates complication.
http://codereview.chromium.org/2827052/diff/1/2 File c/pp_video.h (right): http://codereview.chromium.org/2827052/diff/1/2#newcode23 c/pp_video.h:23: PP_VIDEOCODECOPERATION_ENCODE, if we ever support encode ,should not we call these interface videoservice or videocodec instead of videodecode. http://codereview.chromium.org/2827052/diff/1/2#newcode85 c/pp_video.h:85: PP_VIDEOPAYLOADFORMAT_RTPPAYLOAD, RTP seems have multiple formats. http://codereview.chromium.org/2827052/diff/1/2#newcode92 c/pp_video.h:92: PP_VIDEOFRAMECOLORTYPE_YUV, Packed YUV format have UYVY, YUYV, NV12 http://codereview.chromium.org/2827052/diff/1/2#newcode156 c/pp_video.h:156: PP_Resource buffer; which API create this? http://codereview.chromium.org/2827052/diff/1/3 File c/ppb_video_decode.h (right): http://codereview.chromium.org/2827052/diff/1/3#newcode17 c/ppb_video_decode.h:17: // Returns a list of _pp_VideoCodecDesc for given video codec id. who is responsible for this memory? or static? http://codereview.chromium.org/2827052/diff/1/3#newcode32 c/ppb_video_decode.h:32: PP_Resource resource); who is responsible for this returned memory and I do not see how this is used. http://codereview.chromium.org/2827052/diff/1/3#newcode39 c/ppb_video_decode.h:39: // is not a video decode, or browser can't queue input buffer. I wonder if Pepper have consistent error return code for some common thing , such as ERROR_RESOURCE. just like HRESULT. http://codereview.chromium.org/2827052/diff/1/3#newcode44 c/ppb_video_decode.h:44: } PPB_VideoDecode; we should add comment for how do we drain decoder at end of stream. input_buffer == NULL? and we need Flush function. but you decide if you want to add that later. http://codereview.chromium.org/2827052/diff/1/4 File c/ppp_video_decode.h (right): http://codereview.chromium.org/2827052/diff/1/4#newcode22 c/ppp_video_decode.h:22: } PPP_VideoDecode; what about event notification, such as error, device lost. etc.
http://codereview.chromium.org/2827052/diff/1/4 File c/ppp_video_decode.h (right): http://codereview.chromium.org/2827052/diff/1/4#newcode19 c/ppp_video_decode.h:19: // Return compressed data buffer to plugin. This is optional. What kind of flow control is expected with these interface? we should not just pump the input into pepper as fast as URL load.
Since video is a developing area, we need extendability. In this sense, it's better to use dictionary (key/value pairs), instead of fixed structure. That will add some complexity on both sides. Any comments on that? http://codereview.chromium.org/2827052/diff/1/2 File c/pp_video.h (right): http://codereview.chromium.org/2827052/diff/1/2#newcode13 c/pp_video.h:13: PP_VIDEODECODEID_NONE, Done. That's a typo. It should be PP_VIDEOCODECID_*. On 2010/07/15 22:38:10, brettw wrote: > Can you give enums explicit values? We want this to be consistent across all > compilers, so just starting with the first one "= 0" should be sufficient. > > Can you make sure the enum values match the name of the enum? In this case, it > would be PP_VIDEOCODECID_... http://codereview.chromium.org/2827052/diff/1/2#newcode16 c/pp_video.h:16: PP_VIDEODECODEID_MPEG2, Do we want to limit ourselves to currently existing web codec's? We can put codec names here and plugin can query browser's video decoding capacity anyway. On 2010/07/15 22:52:02, Alpha wrote: > Do we want to support MPEG2 and VC1? The browser now only supports decoding of > H264, VP8 and Theora officially, are we going to provide more codecs for pepper? > http://codereview.chromium.org/2827052/diff/1/2#newcode23 c/pp_video.h:23: PP_VIDEOCODECOPERATION_ENCODE, This enum is called VideoCodec, not VideoDecode. On 2010/07/15 22:52:53, jiesun wrote: > if we ever support encode ,should not we call these interface videoservice or > videocodec instead of videodecode. http://codereview.chromium.org/2827052/diff/1/2#newcode27 c/pp_video.h:27: PP_VIDEOCODECPROFIL_NONE, On 2010/07/15 22:38:10, brettw wrote: > Forgot an E at the end of PROFILE for all these names. Done. Thanks for catching that. It's easy to have typo in upper case. http://codereview.chromium.org/2827052/diff/1/2#newcode84 c/pp_video.h:84: PP_VIDEOPAYLOADFORMAT_BYTESTREAM, I will dig more into this. I thought byte stream is the raw stream format with 0x000001?? as separator. On 2010/07/15 22:52:02, Alpha wrote: > Instead of just having bytestream I think we should limit our scope of support > and be explicit about the bytestream format too, like H264 is Annex B, VP8 is > whatever is standard. > http://codereview.chromium.org/2827052/diff/1/2#newcode85 c/pp_video.h:85: PP_VIDEOPAYLOADFORMAT_RTPPAYLOAD, RTP payload means "length" followed by "data", possibly repeating the pair many times. The size of "length" could be 1/2/4 bytes. In case of 1/2 bytes, plugin has to copy "data" in order to convert it to byte stream format. If it's ok to always ask plugin to copy data, I am fine with supporting only byte stream only. On 2010/07/15 22:52:02, Alpha wrote: > Is it part of the plan for pepper video to support RTP payload? I feel like the > pepper video decoder should support only byte stream and plugin will do the > necessary conversion. http://codereview.chromium.org/2827052/diff/1/2#newcode92 c/pp_video.h:92: PP_VIDEOFRAMECOLORTYPE_YUV, There are way too many color formats/types. I didn't put all of them here. YUV is a request from plugin when it asks for YUV format, because it doesn't know which YUV (420/422/444) the decoded result is. Of course, plugin can always parse header data and figure it out. On 2010/07/15 22:52:53, jiesun wrote: > Packed YUV format have UYVY, YUYV, NV12 http://codereview.chromium.org/2827052/diff/1/2#newcode102 c/pp_video.h:102: PP_VIDEOFRAMESURFACETYPE_PIXELMAP, On 2010/07/15 22:52:02, Alpha wrote: > Do you mean Pixmap? Yes. http://codereview.chromium.org/2827052/diff/1/2#newcode108 c/pp_video.h:108: PP_VIDEOFRAMEINFOFLAG_NOEMIT = 1 << 1, On 2010/07/15 22:52:02, Alpha wrote: > Can you put a bit more comments about these flags? Done. http://codereview.chromium.org/2827052/diff/1/2#newcode122 c/pp_video.h:122: bool accelerated; Providing SW decoder is needed during development. It's also useful to provide more video decoding capability to plugin developer if those decoders are easy to get. In some cases, it's convenient for plugin developers so that they don't have to maintain 2 code paths. On 2010/07/15 22:52:02, Alpha wrote: > Is it part of the plan to provide SW decoder if HW decoder is not available for > specified codec / profile / level? I think we should let the plugin to provide > SW decoder. http://codereview.chromium.org/2827052/diff/1/2#newcode141 c/pp_video.h:141: int32_t count; I will remove this one. It's over done. On 2010/07/15 22:38:10, brettw wrote: > What is the likelihood do you think we'll add more stuff here to the config? > Once the API is "frozen" then we can'd add more stuff to this structure anyway, > and would require a new config function and struct on a new interface. Because > of this, if you don't have immediate plans to add more stuff here, I'd just pass > buffer_count as an arg when you need it and delete this struct. http://codereview.chromium.org/2827052/diff/1/2#newcode141 c/pp_video.h:141: int32_t count; On 2010/07/15 22:52:02, Alpha wrote: > Does this mean each input buffer can contains multiple frames? No. This is the structure used by plugin to query how many input buffers it can send to decoder. It's added for input flow control. But input flow control can be done by returned value from |Decode|. So I will remove it. http://codereview.chromium.org/2827052/diff/1/2#newcode156 c/pp_video.h:156: PP_Resource buffer; On 2010/07/15 22:52:53, jiesun wrote: > which API create this? That's a good question. This buffer is the decoded frame in requested format. I intentionally keep it vague here, because I don't know what plugin will do with this buffer. Just passing it to renderer, or trying to access pixel data? So the real structures are not defined yet. http://codereview.chromium.org/2827052/diff/1/3 File c/ppb_video_decode.h (right): http://codereview.chromium.org/2827052/diff/1/3#newcode17 c/ppb_video_decode.h:17: // Returns a list of _pp_VideoCodecDesc for given video codec id. It's dynamically allocated memory. Ownership is transfered to caller. On 2010/07/15 22:52:53, jiesun wrote: > who is responsible for this memory? or static? http://codereview.chromium.org/2827052/diff/1/3#newcode26 c/ppb_video_decode.h:26: // Returns true if the given resource is a video decoder. Returns false if the On 2010/07/15 22:38:10, brettw wrote: > Looks like this comment should go on an Is... function. This is the de-init function, not a query function. http://codereview.chromium.org/2827052/diff/1/3#newcode31 c/ppb_video_decode.h:31: PP_VideoDecodeInputBufferConfig* (*GetVideoDecodeInputBufferConfig)( On 2010/07/15 22:52:02, Alpha wrote: > InputBufferConfig only returns count, how is it used? This one is over-designed. The input buffer flow control is done by returning value from |Decode| function. I will remove this. http://codereview.chromium.org/2827052/diff/1/3#newcode32 c/ppb_video_decode.h:32: PP_Resource resource); Ownership is transferred to caller. On 2010/07/15 22:52:53, jiesun wrote: > who is responsible for this returned memory and I do not see how this is used. http://codereview.chromium.org/2827052/diff/1/3#newcode40 c/ppb_video_decode.h:40: bool (*Decode)(PP_Resource resource, It's asynchronous. Decoder calls an API from plugin (DecodeDone) to return decoded frame. Input buffer can be recycled if plugin provides an API function in ppp_video_decode. On 2010/07/15 22:52:02, Alpha wrote: > This looks like Decode() will take ownership of |input_buffer| and there is not > recycling or fixed amount of input buffers? right? > > Also how can we get a signal that a frame is ready? http://codereview.chromium.org/2827052/diff/1/3#newcode42 c/ppb_video_decode.h:42: PP_VideoFrameInfo* frame_info); yes, that's another way to do it. On 2010/07/15 22:38:10, brettw wrote: > We should make the asynchronous completion here work the same as URLLoader. In > that case, it takes a completion callback as an argument (this makes it very > easy for the plugin to route the notifications properly). The callback just > notifies you that video is ready, and then you call a function to actually get > the data. This may mean you can delete the while PPP_ interface. http://codereview.chromium.org/2827052/diff/1/3#newcode44 c/ppb_video_decode.h:44: } PPB_VideoDecode; EOS flag is the way to tell decoder to finish decoding. Flush function will be added. I also need to figure out whether to explicitly recycle frame buffer, not let it done implicitly. On 2010/07/15 22:52:53, jiesun wrote: > we should add comment for how do we drain decoder at end of stream. input_buffer > == NULL? > and we need Flush function. but you decide if you want to add that later. http://codereview.chromium.org/2827052/diff/1/4 File c/ppp_video_decode.h (right): http://codereview.chromium.org/2827052/diff/1/4#newcode13 c/ppp_video_decode.h:13: typedef struct _ppp_VideoDecode { On 2010/07/15 22:38:10, brettw wrote: > I think the classes/files should be called "VideoDecoder" (rather than > "VideoDecode") which matches the existing "URLLoader" Done. http://codereview.chromium.org/2827052/diff/1/4#newcode19 c/ppp_video_decode.h:19: // Return compressed data buffer to plugin. This is optional. Plugin should observe returned value from |Decode| function call. That will prevent it from overwhelming decoder. On 2010/07/15 23:00:30, jiesun wrote: > What kind of flow control is expected with these interface? we should not just > pump the input into pepper as fast as URL load. > > http://codereview.chromium.org/2827052/diff/1/4#newcode19 c/ppp_video_decode.h:19: // Return compressed data buffer to plugin. This is optional. This is the plugin API to return decoded frame, not input buffer. Returning input buffer is optional if plugin wants to recycle those buffers and provide |ReturnCompressedDataBuffer|. On 2010/07/15 22:52:02, Alpha wrote: > It is not very clear how this method is used? If the default way of using the > decoder is that it takes ownership of input buffer, we shouldn't have a second > mode of operation since it creates complication. http://codereview.chromium.org/2827052/diff/1/4#newcode22 c/ppp_video_decode.h:22: } PPP_VideoDecode; FrameInfo has a member |has_error|. Other events can be added over there, too. On 2010/07/15 22:52:53, jiesun wrote: > what about event notification, such as error, device lost. etc.
On Thu, Jul 15, 2010 at 6:43 PM, <wjia@chromium.org> wrote: > Since video is a developing area, we need extendability. In this sense, it's > better to use dictionary (key/value pairs), instead of fixed structure. That > will add some complexity on both sides. > Any comments on that? This sounds like a good idea. We should make sure it looks the same as what we want to do for the 3D API, which will use something similar to configure the surfaces. Brett
http://codereview.chromium.org/2827052/diff/9001/10001 File c/pp_video.h (right): http://codereview.chromium.org/2827052/diff/9001/10001#newcode117 c/pp_video.h:117: typedef struct _pp_VideoCodecDesc { please spell out Description http://codereview.chromium.org/2827052/diff/9001/10001#newcode139 c/pp_video.h:139: typedef struct _pp_VideoCompressedDataBuffer { perhaps we should try to reuse PPB_Buffer for any kind of bulk data? PPB_Buffer will be backed by shared memory to facilitate efficient transport between the NEXE (running in an external process) and the renderer. http://codereview.chromium.org/2827052/diff/9001/10001#newcode145 c/pp_video.h:145: uint64_t time_stamp_us; what clock is this relative to? any reason not to use PP_Time? http://codereview.chromium.org/2827052/diff/9001/10001#newcode156 c/pp_video.h:156: PP_Resource buffer; can you comment what type of object this is? does this correspond to PPB_Buffer? http://codereview.chromium.org/2827052/diff/9001/10002 File c/ppb_video_decoder.h (right): http://codereview.chromium.org/2827052/diff/9001/10002#newcode19 c/ppb_video_decoder.h:19: PP_VideoCodecDesc* (*GetVideoDecodeCapacity)(PP_VideoCodecId codec); how about making PP_VideoCodecDesc be an output parameter? that way the plugin does not have to remember to call PPB_Core::MemFree on this pointer. http://codereview.chromium.org/2827052/diff/9001/10002#newcode24 c/ppb_video_decoder.h:24: PP_VideoCompressedDataFormat* input_format, can these input parameters be const pointers? http://codereview.chromium.org/2827052/diff/9001/10002#newcode29 c/ppb_video_decoder.h:29: bool (*Destroy)(PP_Resource resource); this comment seems to correspond to an IsVideoDecoder method, but since the method is named Destroy, it seems like it is something else. why do you need an explicit Destroy method? PP_Resource is reference counted. when the last reference is dropped, the video decoder will be destroyed. or, do you need a way to interrupt any pending decoding? http://codereview.chromium.org/2827052/diff/9001/10002#newcode37 c/ppb_video_decoder.h:37: bool (*Decode)(PP_Resource resource, please name the PP_Resource parameter "video_decoder" so that it is clear that you intend for people to pass a PP_Resource that was returned from PPB_VideoDecoder::Create. http://codereview.chromium.org/2827052/diff/9001/10002#newcode39 c/ppb_video_decoder.h:39: PP_VideoFrameInfo* frame_info); can you change this so that it uses PP_CompletionCallback to report when decoding completes? then you can add a method on PPB_VideoDecoder to access the last decoded frame. when the completion callback is notified, the plugin would ask the video decoder for the last decoded frame. http://codereview.chromium.org/2827052/diff/9001/10002#newcode39 c/ppb_video_decoder.h:39: PP_VideoFrameInfo* frame_info); if frame_info is pure input, then please make it be a const parameter. http://codereview.chromium.org/2827052/diff/9001/10002#newcode43 c/ppb_video_decoder.h:43: void (*Flush)(PP_Resource resource); Please use PP_CompletionCallback to report that flush completed.
This patch has features: 1. support extendability. 2. major functions are reduced to a pair, one from decoder, the other from plugin. New functions can be added easily by using new command id or event id. 3. input and output buffers are defined now. 4. buffer recycling is in place.
http://codereview.chromium.org/2827052/diff/18001/4003 File c/ppb_video_decoder.h (right): http://codereview.chromium.org/2827052/diff/18001/4003#newcode30 c/ppb_video_decoder.h:30: PP_FreeDescription_Func free_func); I'd like to find a way to avoid the PP_FreeDescription_Func parameter. Also, it is a bit confusing that this method is named GetCapacity. Did you mean GetCapability? "capacity" refers to the size of something. This seems similar to what we need for the DeviceContext3D API. Al Patrick had devised a nice way to query device capabilities that was IIRC based on how EGL works. Can we use that method here? http://codereview.chromium.org/2827052/diff/18001/4003#newcode41 c/ppb_video_decoder.h:41: bool (*Create)(PP_Instance instance, this should just return PP_Resource. if the result is 0, then it means failure, so there is no need for the bool return value. http://codereview.chromium.org/2827052/diff/18001/4003#newcode49 c/ppb_video_decoder.h:49: bool (*Destroy)(PP_Resource decoder); This method should not be necessary since PP_Resource objects are reference counted. You should be able to delete this method. http://codereview.chromium.org/2827052/diff/18001/4003#newcode53 c/ppb_video_decoder.h:53: // When event == PP_VIDEODECODEPLUGINEVENT_DECODE, how about just defining three methods here instead of trying to compact them down into a single uber function?
Some thoughts and questions about Darin's review. http://codereview.chromium.org/2827052/diff/18001/4003 File c/ppb_video_decoder.h (right): http://codereview.chromium.org/2827052/diff/18001/4003#newcode30 c/ppb_video_decoder.h:30: PP_FreeDescription_Func free_func); I will rename it to GetCapability. Is there any reason that we need avoid PP_FreeDescription_Func? I can move to EGL-like way to return config's. But that put some coding burden on plugin since plugin has to call the same function several times till it returns NULL. On 2010/07/21 18:22:55, darin wrote: > I'd like to find a way to avoid the PP_FreeDescription_Func parameter. Also, it > is a bit confusing that this method is named GetCapacity. Did you mean > GetCapability? "capacity" refers to the size of something. > > This seems similar to what we need for the DeviceContext3D API. Al Patrick had > devised a nice way to query device capabilities that was IIRC based on how EGL > works. Can we use that method here? http://codereview.chromium.org/2827052/diff/18001/4003#newcode53 c/ppb_video_decoder.h:53: // When event == PP_VIDEODECODEPLUGINEVENT_DECODE, This is a question about extendability. So far I know we need these 3 functions. What if we need add more API functions in the future since video is a developing field? Using a single API function (or at least there is one such function in API) with command ID's gives us much flexibilities. On 2010/07/21 18:22:55, darin wrote: > how about just defining three methods here instead of trying to compact them > down into a single uber function?
On Wed, Jul 21, 2010 at 2:33 PM, <wjia@chromium.org> wrote: > Some thoughts and questions about Darin's review. > > > > http://codereview.chromium.org/2827052/diff/18001/4003 > File c/ppb_video_decoder.h (right): > > http://codereview.chromium.org/2827052/diff/18001/4003#newcode30 > c/ppb_video_decoder.h:30: PP_FreeDescription_Func free_func); > I will rename it to GetCapability. Is there any reason that we need > avoid PP_FreeDescription_Func? I can move to EGL-like way to return > config's. But that put some coding burden on plugin since plugin has to > call the same function several times till it returns NULL. I'd like Al Patrick's review on this part. He had a system for this kind of thing that he was intending for us to use with the DeviceContext3D interface. I think it is valuable for this video interface to use a consistent mechanism. PP_FreeDescription_Func is undesirable because it introduces a magic function that needs to be used to free a specific resource. We have tried very hard to avoid functions like that. One option is to try to use PPB_Core::MemFree, but if we can avoid that it would be even better IMO. (I'm pretty sure that Al's solution avoids that.) > > On 2010/07/21 18:22:55, darin wrote: > >> I'd like to find a way to avoid the PP_FreeDescription_Func parameter. >> > Also, it > >> is a bit confusing that this method is named GetCapacity. Did you >> > mean > >> GetCapability? "capacity" refers to the size of something. >> > > This seems similar to what we need for the DeviceContext3D API. Al >> > Patrick had > >> devised a nice way to query device capabilities that was IIRC based on >> > how EGL > >> works. Can we use that method here? >> > > http://codereview.chromium.org/2827052/diff/18001/4003#newcode53 > c/ppb_video_decoder.h:53: // When event == > PP_VIDEODECODEPLUGINEVENT_DECODE, > This is a question about extendability. So far I know we need these 3 > functions. What if we need add more API functions in the future since > video is a developing field? Using a single API function (or at least > there is one such function in API) with command ID's gives us much > flexibilities. > > We can add functions to the interface while it is still in development / unfrozen. Once we wish to claim that the interface is stable, we would also not want to add additional event values as that would cause potential compatibility problems. We would need to bump the version number of the interface whenever the interface contract changes. This includes enum values since there is no other way to detect support for enum values other than to select the version of the interface you want. At that point, you might as well just have another function. -Darin > On 2010/07/21 18:22:55, darin wrote: > >> how about just defining three methods here instead of trying to >> > compact them > >> down into a single uber function? >> > > http://codereview.chromium.org/2827052/show >
I've got more comments!!! Need to finish formulating them :) http://codereview.chromium.org/2827052/diff/18001/4002 File c/pp_video.h (right): http://codereview.chromium.org/2827052/diff/18001/4002#newcode211 c/pp_video.h:211: // Function to free memory for descriptions. descriptions -> descriptors
http://codereview.chromium.org/2827052/diff/18001/4002 File c/pp_video.h (right): http://codereview.chromium.org/2827052/diff/18001/4002#newcode251 c/pp_video.h:251: typedef struct _pp_VideoFrameInfo { Can we merge some of the fields in VideoFrameInto into data buffer? The compressed video data buffer almost always has timestamp and flags like eof, keyframe, anchor, etc. Why splitting it up? http://codereview.chromium.org/2827052/diff/18001/4002#newcode258 c/pp_video.h:258: // Output from decoder, How is this used in combination with PP_VIDEODECODEREVENT_ERROR? It looks like duplicated. http://codereview.chromium.org/2827052/diff/18001/4003 File c/ppb_video_decoder.h (right): http://codereview.chromium.org/2827052/diff/18001/4003#newcode42 c/ppb_video_decoder.h:42: const PP_VideoDecoderConfig* decoder_config, The implication that you pass in a decoder config with detailed profiles and levels is that the plugin using this API has to know the codec specific details about the video file it's try to decode. Suppose now the system has both a hardware decoder and we provide a software H264 decoder. The hardware decoder supports only main profile but software decoder supports upto high profile. The plugin not knowing what the content is would probably choose the hardware decoder, and then when it will decode with failure, it will need to destroy the hardware decoder and creates a software decoder again. The whole initialization process seems to be pretty complicated for the plugin that uses this API. It is nice to query the capability of the decoder backing this API, but letting the plugin to setting a decoder with specific profile / level seems to be a bit excessive, and complicates the initialization procedure. If we really want to provide a fallback software decoder, we can do this internal to this API, so there will only be one generic H264 decoder and when we have got enough header data this API gets to choose SW or HW decoder to use. This is similar to FFmpeg's avcodec_open() that it reads the file internally. We can do this by feeding the decoder data until it is initialized. The other question is that this create API is synchronous, how does this work if a decoder is created in the gpu process? http://codereview.chromium.org/2827052/diff/18001/4003#newcode53 c/ppb_video_decoder.h:53: // When event == PP_VIDEODECODEPLUGINEVENT_DECODE, I can understand extendability for future codecs, but having flags to define behavior of an API seems strange to me. The paradigm of video decoder doesn't really change, it's always like init / decode / flush. Just like now the parameters are changing meaning / validity as event changes, it'll get more complicated as we extend behavior through more event and parameters combinations.
http://codereview.chromium.org/2827052/diff/18001/4002 File c/pp_video.h (right): http://codereview.chromium.org/2827052/diff/18001/4002#newcode68 c/pp_video.h:68: enum PP_VideoCodecId { echoing alpha's concerns here about including codecs we don't plan on supporting in the near future we can always add but taking away is much more difficult http://codereview.chromium.org/2827052/diff/18001/4002#newcode82 c/pp_video.h:82: enum PP_VideoCodecProfile { I'm wondering about forcing all codecs into a profile+level format. This could work for H264 but might not carry over as well to other codecs. Let's assume we're trying to use this API for H264 in an MP4 container: we'd most likely have some code that finds the avcC atom, parse out this information and then pass it through to this interface. Not too bad but a little annoying. Now let's think about it implementing this API: on Windows I believe the DXVAv2 interface doesn't use this at all. On Mac we'd have to reconstruct the avcC struct and pass it to their API. Similar story with OpenMAX and OMX_VIDEO_PARAM_AVCTYPE. Problem here is that we actually have less information to determine hardware acceleration support than what we started with. I'm thinking we could either go very lax and let API users provide a buffer containing codec-specific information (for H264 this would be AVCDecoderConfigurationRecord as described in ISO-14495 Part 15), or be stricter and have more detailed configuration (like OpenMAX's OMX_VIDEO_PARAM_AVCTYPE or DXVAv2's DXVA2_ConfigPictureDecode structs). In any case the current form feels strange where Profile/Level is required but isn't applicable in most cases. What do you think? http://codereview.chromium.org/2827052/diff/18001/4002#newcode157 c/pp_video.h:157: PP_VIDEOFRAMESURFACETYPE_GLTEXTURE, just as a sanity check there'll be no way to ever have a D3D surface or similar? what about an EGLImage? will we assume the underlying codecs will fall into these categories? http://codereview.chromium.org/2827052/diff/18001/4002#newcode271 c/pp_video.h:271: PP_Resource buffer[PP_VIDEOFRAMEBUFFER_MAXNUMBERPLANES]; what about extending PP_Resource to include width/height/stride (PP_VideoFrame) and then having an array of those? http://codereview.chromium.org/2827052/diff/18001/4002#newcode284 c/pp_video.h:284: PP_VideoConfig format; does every uncompressed frame need this information? would the API user already have this stored?
Thank you all for the review. Patch Set 4 addresses some comments from review: 1. Function call from plugin to decoder is explicit now, instead of a master dispatcher. 2. decoder configuration querying is pretty much same as EGL, assuming browser owns the memory for configs all the way. There are other concerns in the review. Here are my thoughts on them: 1. When decoder tries to send something to plugin, could decoder just send a signal so as to let plugin query the real data? This approach introduces some complexity in PPB code. It has to store all results coming from low level decoder and has to find the correct one when plugin queries it. Performance wise, it's also a hit. 2.Can we merge some of the fields in VideoFrameInto into data buffer? The frame info is used by both input and output buffers. It's ok to put frame info in both input and output buffer data structures if extra memory copying is fine. 3. Why are there both PP_VIDEODECODEREVENT_ERROR and |has_error| in decoded frame info? There are at least 2 major types of error that decoder needs convey to plugin. One is that something really goes wrong in decoding process, e.g., H/W error, resource is lost. The other one is the frame is decoded, but input bit stream is corrupted. The decoder has applied error concealment (or similar techniques) to create a decoded frame. These 2 types of errors result in different actions on plugin side. 4. How does capability querying work? Is it needed? The upstream component of decoder is demuxer. In many cases, demuxer is able to obtain profile/level information about the stream. Querying PPB's decoding capability allows plugin to choose correct decoding path when PPB doesn't support that profile/level. As far as I know, one of our key customers has that. In case plugin doesn't know profile/level, it can always skip profile/level keys in |input_format|, or just pass *_NONE as value when |Create| is called. Then it's up to PPB to choose the most appropriate decoder. In this case, PPB has to wait for the bit stream data to come so that it knows which decoder to open internally. This means that plugin doesn't have to query PPB's capability. Furthermore, PP_VideoConfig is expandable. So is PP_VideoKey. Any key in PP_VideoKey can be included in PP_VideoConfig, and PP_VideoConfig can contains no key from PP_VideoKey. PP_VideoKey is also expandable to include more keys. Profile/level is the basic language for video codec. AVCDecoderConfigurationRecord (as described in ISO-14495 Part 15) has explicitly AVCProfileIndication and AVCLevelIndication, while everything else is in SPS and PPS. When client queries OMX with OMX_VIDEO_PARAM_AVCTYPE, it also inludes profile and level with many other features. The only exception is DXVA. DXVA client has to parse lots of stuffs and pass them to DXVA. In order to be prepared to fall back to software decoding, DXVA client has to store bit stream data till it's sure that the stream can be decoded by DXVA. That's bad, especially for streaming with limited cache. DXVA2_ConfigPictureDecode describes how low level decoder works, not much related to which streams can be decoded. It's always possible to include more detailed codec features in PP_VideoKey. That will be a long list though. I have included some important ones in the new patch. 5. |Create| API would be synchronous or asynchronous? Right now, it's synchronous. I expect plugin will take different actions based on |Create| result. If plugin wants to make use of that waiting time, it would do multi-threading. 6. Why use a master dispatch function? The original intention was to fix functions in the API for extendability. This could allow forward/backward compatibility without looking to versioning. I am not sure if create/decode/flush covers all use cases. What if plugin wants to query some runtime info of the decoder? 7. How about taking away those codec id's which are not supported in the near future? If some codec is not supported, plugin should be able to know about it by querying PPB's capability, or |Create| will fail. On the contrary, I think we should add as many codec id's as possible now to avoid another new version of PPB_VideoDecoder API just for adding more codec id's. 8. In PP_VideoFrameSurfaceType, there is no D3D_surface or EGLImage. Thanks to Angle project, we should be able to use GLES/EGL interface for D3D. For EGLImage, we have to pass GL_texture back to plugin because we have experienced that deriving gl texture from EGLImage somehow doesn't work. That's why I put a private_handle in PP_VideoFrameBuffer. If you know of a way to create gl texture from EGLImage, please do let me know. 9. does every uncompressed frame need |format| info? Extra caution (maybe too much?) was exercised here because some profile allows more than YUV420 (e.g., monochrome) according to spec. Meanwhile, in case of buffer recycling, putting |format| in PP_VideoUncompressedDataBuffer doesn't hurt performance because it will be set only once.
It would be great if all the reviewers could look at Wei's questions soon. At some point, we need to decide this is good enough and check it in so we can make progress. We can always iterate. I would like to get some agreement this week if possible. 1. When decoder tries to send something to plugin, could decoder just send a signal so as to let plugin query the real data? I would like to try this first. If it ends up being very complicated or too slow we can see what we need to do to fix it. But since this matches the rest of the system, I'd like to at least try this way first. 5. |Create| API would be synchronous or asynchronous? Does create need to do something in the GPU process, or can it just set up some object and return? If it's the "allocate and return" approach, we should do synchronous. If it's complicated or requires cross-process stuff, we should make it asynchronous since we have to implement it as asynchronous in the renderer anyway If we need the asynchronous case, it's probably best to have a simple "Create" function that allocates the resource, and then an initialize function that does the slow initialization. The initialize function would take a completion callback. We already have a way to specify "I want to make this completion callback just block when I'm running on a background thread" so this will be convenient for the plugin authors as well. 6. Why use a master dispatch function? We should deal with API compatibility using the PPAPI interface system since it is designed for this. Individual components should just be as clear as possible so they're easy to use, and then we have our uniform compat system. 7. How about taking away those codec id's which are not supported in the near future? It's OK with me if we have enums for some codecs we don't support. http://codereview.chromium.org/2827052/diff/26001/15002 File c/pp_video.h (right): http://codereview.chromium.org/2827052/diff/26001/15002#newcode250 c/pp_video.h:250: typedef void (*PP_VideoDecodeOutputCallback_Func)(PP_Instance instance, Google style generally prefers each argument to be aligned. So if you have to indent args to 4 spaces like you do here, you should also move the first argument with them. Same for the other functions here. http://codereview.chromium.org/2827052/diff/26001/15003 File c/ppb_video_decoder.h (right): http://codereview.chromium.org/2827052/diff/26001/15003#newcode29 c/ppb_video_decoder.h:29: // NOTE: browser owns the memory of all PP_VideoConfig's. I'm not sure how we can proxy this API out of process. It seems like we would be forced to leak the configs in the proxy layer in the plugin. What I would do is specify that the plugin must free the memory. We do this with PPB_Core.MemFree(). Darin said to try to avoid this, but I don't see a way around it in this case. You can also make the wrapping array work this way to avoid the two-level calls. So it would be GetConfig(codec, PP_VideoConfig** configs, int32_t* num_config); And the plugin would free configs[0], configs[1], ... and also configs when those are done. We'll write a C++ wrapper for this so it will be hard to mess up from the plugin side. http://codereview.chromium.org/2827052/diff/26001/15003#newcode38 c/ppb_video_decoder.h:38: // Plugin has the option to specify codec profile and level, even more info, Grammar: I'm not sure what "even more info" means here. Maybe you can just delete it?
Thanks for answering my questions wei! I think we're at a point where we'll have to start iterating based on implementation experience. Anyone else still want to review this? http://codereview.chromium.org/2827052/diff/26001/15002 File c/pp_video.h (right): http://codereview.chromium.org/2827052/diff/26001/15002#newcode85 c/pp_video.h:85: PP_VIDEOCODECPROFILE_VC1_SIMPLE = 64, any reason to use decimal here versus hex above to separate enum ranges? http://codereview.chromium.org/2827052/diff/26001/15002#newcode141 c/pp_video.h:141: PP_VIDEOFRAMECOLORTYPE_Mono, Mono -> MONOCHROME http://codereview.chromium.org/2827052/diff/26001/15003 File c/ppb_video_decoder.h (right): http://codereview.chromium.org/2827052/diff/26001/15003#newcode33 c/ppb_video_decoder.h:33: int32_t *num_config); pointer goes with type http://codereview.chromium.org/2827052/diff/26001/15003#newcode83 c/ppb_video_decoder.h:83: bool (*ReturnUncompressedDataBuffer)(PP_Resource decoder, ditto on indentation style here
On Wed, Jul 28, 2010 at 11:19 AM, <scherkus@chromium.org> wrote: > Thanks for answering my questions wei! > > I think we're at a point where we'll have to start iterating based on > implementation experience. > > Anyone else still want to review this? Not it!
Sorry for the delay. This looks good to me. Yup we can always iterate later. |
