Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
210373003:
Pepper VideoDecoder API definition. (Closed)
|
Created:
6 years, 9 months ago by bbudge Modified:
6 years, 7 months ago CC:
chromium-reviews, yusukes+watch_chromium.org, tzik, raymes+watch_chromium.org, teravest+watch_chromium.org, yzshen+watch_chromium.org, nfullagar1, piman+watch_chromium.org, noelallen1, binji, ihf+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdds IDL for enums and structs for encoding/decoding.
Adds IDL for PPB_VideoDecoder.
Adds C++ wrappers and generated files.
BUG=281689
R=binji@chromium.org, dmichael@chromium.org, igorc@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=268618
Patch Set 1 : #
Total comments: 41
Patch Set 2 : Eliminate GetBitstreamBuffer function. Add 'picture_id' parameter. #
Total comments: 20
Patch Set 3 : #Patch Set 4 : Rename Decode() param 'picture_id' to 'user_id'. #Patch Set 5 : Plugin should call RecyclePicture during Flush. #
Total comments: 18
Patch Set 6 : David's comments, run clang format. #
Total comments: 2
Patch Set 7 : Rename 'picture_id' to 'decode_id'. Address comments. #
Total comments: 2
Patch Set 8 : Tweak comment.x #
Total comments: 10
Patch Set 9 : Add 'allow_software_fallback' param to Initialize. #
Total comments: 1
Patch Set 10 : Clarify Flush / GetPicture behavior. #Patch Set 11 : Remove 'MediaCodec' from names, fix enum naming. #Patch Set 12 : Add CreateVideoDecoder to resource creation API. #Patch Set 13 : Exclude thunk from build for now. #
Messages
Total messages: 39 (0 generated)
I have the example working so I think this API will work. The implementation patch is here: https://codereview.chromium.org/213313003/
binji@chromium.org: Please review changes in native client sdk
https://codereview.chromium.org/210373003/diff/210001/ppapi/api/pp_media_code... File ppapi/api/pp_media_codec.idl (right): https://codereview.chromium.org/210373003/diff/210001/ppapi/api/pp_media_code... ppapi/api/pp_media_codec.idl:28: * should not be modified by the plugin. So this comment only refers to the struct, but the plugin can write to the data addr references? Not sure if there's a better way to word that. https://codereview.chromium.org/210373003/diff/210001/ppapi/api/ppb_media_cod... File ppapi/api/ppb_media_codec_video_decoder.idl (right): https://codereview.chromium.org/210373003/diff/210001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:76: [in] PP_Resource context, might be good to name graphics3d_context https://codereview.chromium.org/210373003/diff/210001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:77: [in] PP_MediaCodec_Profile profile, Do you have a function yet for getting supported profiles? Do we want to consider a "RecommendProfile" kind of function, where you can pass some information about the video you have and have the decoder tell you what profile you should use? Or: 1) Treat this profile parameter as a suggestion, and the decoder automatically figures out the best applicable profile the hardware can support or... 2) Detect the appropriate profile automatically from the stream (I have to admit I know little enough about decoding to know if 1 and 2 are insane) https://codereview.chromium.org/210373003/diff/210001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:85: * to GetBitstreamBuffer(). I wonder if it might be possible for Chrome/PPAPI to handle the throttling? One possible way is to only return a BitstreamBuffer to the plugin *only* when it's appropriate to call Decode again. Then the plugin doesn't have to think at all about keeping the pipe full; they keep pushing data so long as they have a place to write it. https://codereview.chromium.org/210373003/diff/210001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:161: * flushing, the plugin should not attempt to decode more video. You could maybe flesh this comment out a bit with why you might want to use flush and what happens when it's done. If I understand correctly, something like: " After Flush() completes, the plugin may begin decoding frames again. This can be useful for seeking in the video stream." Also, what happens to BitStreamBuffers the plugin already has? Are they still valid to use after a Flush? And... maybe all of what I just said really belongs on "Reset" instead... https://codereview.chromium.org/210373003/diff/210001/ppapi/cpp/media_codec_v... File ppapi/cpp/media_codec_video_decoder.h (right): https://codereview.chromium.org/210373003/diff/210001/ppapi/cpp/media_codec_v... ppapi/cpp/media_codec_video_decoder.h:43: int32_t Initialize(Graphics3D* context, Shouldn't this be "const Graphics3D&"?
https://codereview.chromium.org/210373003/diff/210001/ppapi/api/pp_media_code... File ppapi/api/pp_media_codec.idl (right): https://codereview.chromium.org/210373003/diff/210001/ppapi/api/pp_media_code... ppapi/api/pp_media_codec.idl:9: enum PP_MediaCodec_Profile { "MediaCodec" implies to me these values would match Android's MediaCodec, not chromium's profile num. https://codereview.chromium.org/210373003/diff/210001/ppapi/api/pp_media_code... ppapi/api/pp_media_codec.idl:76: * Dimensions of the texture holding the decoded picture. Elsewhere (cf. media::VideoFrame) it's been important to keep other data such as visible rect (for cropping), coded rect (for padding/alignment), and natural size (for aspect ratio). I don't know if you want to track any of that here. https://codereview.chromium.org/210373003/diff/210001/ppapi/api/ppb_media_cod... File ppapi/api/ppb_media_codec_video_decoder.idl (right): https://codereview.chromium.org/210373003/diff/210001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:58: * after Create() and before any other functions. Hardware video decoding is I thought the point of this take on the API was to hide the unavailability of HW capabilities behind SW fallback, no? https://codereview.chromium.org/210373003/diff/210001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:71: * Returns PP_ERROR_NOTSUPPORTED on platforms where hardware video decoding is I'm surprised status is not returned in the callback instead. (comment applies elsewhere below as well) https://codereview.chromium.org/210373003/diff/210001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:85: * to GetBitstreamBuffer(). why GBB() and not Decode() in this sentence? https://codereview.chromium.org/210373003/diff/210001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:89: * @param[in] size The size of the buffer in bytes. s/size/capacity/ ? https://codereview.chromium.org/210373003/diff/210001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:115: * completion. what does "completion" mean in this context? - decoder accepting the buffer - buffer is on the IPC wire to the decoder - decoder has finished with the buffer - something else? https://codereview.chromium.org/210373003/diff/210001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:127: * system. The plugin can call GetPictureBuffer() again in its callback to what callback? https://codereview.chromium.org/210373003/diff/210001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:161: * flushing, the plugin should not attempt to decode more video. what about calling reset during flush? https://codereview.chromium.org/210373003/diff/210001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:177: * calls. Once all decodes are aborted and reset is complete, |callback| will Forbid decode & flush from being called during reset? https://codereview.chromium.org/210373003/diff/210001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:178: * be called. doco impact on ownership of picture and bitstream buffers. https://codereview.chromium.org/210373003/diff/210001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:190: }; How does teardown work? (plugin needs to be able to indicate that its pending callbacks should not fire)
https://codereview.chromium.org/210373003/diff/210001/ppapi/api/ppb_media_cod... File ppapi/api/ppb_media_codec_video_decoder.idl (right): https://codereview.chromium.org/210373003/diff/210001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:77: [in] PP_MediaCodec_Profile profile, On 2014/04/09 21:54:07, dmichael wrote: > Do you have a function yet for getting supported profiles? > > Do we want to consider a "RecommendProfile" kind of function, where you can pass > some information about the video you have and have the decoder tell you what > profile you should use? Or: > 1) Treat this profile parameter as a suggestion, and the decoder automatically > figures out the best applicable profile the hardware can support > or... > 2) Detect the appropriate profile automatically from the stream > > (I have to admit I know little enough about decoding to know if 1 and 2 are > insane) profile is a property of the video to be decoded. It does you no good to init the decoder at a higher profile, and it is likely to fail during decode if you initialize with a lower profile. There is active harm in init'ing at a higher-than-needed profile because HW resources will be consumed (reducing the available decoding power for other videos concurrently decoding), so I don't think we want to do #1. The profile is usually stored in the muxing container (e.g. mp4), not in the media bitstream (e.g. h.264) so I don't think #2 works in general. Also, you don't want to find out that the decoder can't handle your stream part-way through (because mid-stream fallback is a hard problem), so Initialize() is the right time to specify the profile.
https://codereview.chromium.org/210373003/diff/210001/ppapi/api/pp_media_code... File ppapi/api/pp_media_codec.idl (right): https://codereview.chromium.org/210373003/diff/210001/ppapi/api/pp_media_code... ppapi/api/pp_media_codec.idl:9: enum PP_MediaCodec_Profile { On 2014/04/09 22:12:41, Ami Fischman wrote: > "MediaCodec" implies to me these values would match Android's MediaCodec, not > chromium's profile num. I'm open to naming suggestions :) I'm using MediaCodec as a prefix in this API with the idea that it will help group things better if we add an encoding API, or Audio APIs later. https://codereview.chromium.org/210373003/diff/210001/ppapi/api/pp_media_code... ppapi/api/pp_media_codec.idl:28: * should not be modified by the plugin. On 2014/04/09 21:54:07, dmichael wrote: > So this comment only refers to the struct, but the plugin can write to the data > addr references? > > Not sure if there's a better way to word that. Eliminated this struct. https://codereview.chromium.org/210373003/diff/210001/ppapi/api/pp_media_code... ppapi/api/pp_media_codec.idl:76: * Dimensions of the texture holding the decoded picture. On 2014/04/09 22:12:41, Ami Fischman wrote: > Elsewhere (cf. media::VideoFrame) it's been important to keep other data such as > visible rect (for cropping), coded rect (for padding/alignment), and natural > size (for aspect ratio). I don't know if you want to track any of that here. I'm not sure where that information would come from. The decoder only gives us a media::Picture. https://codereview.chromium.org/210373003/diff/210001/ppapi/api/ppb_media_cod... File ppapi/api/ppb_media_codec_video_decoder.idl (right): https://codereview.chromium.org/210373003/diff/210001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:58: * after Create() and before any other functions. Hardware video decoding is On 2014/04/09 22:12:41, Ami Fischman wrote: > I thought the point of this take on the API was to hide the unavailability of HW > capabilities behind SW fallback, no? We aren't attempting to provide software fallback because it's hard (the API returns pictures as GL textures). If it's not hard, we could consider it. https://codereview.chromium.org/210373003/diff/210001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:71: * Returns PP_ERROR_NOTSUPPORTED on platforms where hardware video decoding is On 2014/04/09 22:12:41, Ami Fischman wrote: > I'm surprised status is not returned in the callback instead. > (comment applies elsewhere below as well) Comment is a bit misleading since calling this with good params will return PP_OK_COMPLETIONPENDING. The user's callback will receive PP_ERROR_NOTSUPPORTED. Fixed the comment. https://codereview.chromium.org/210373003/diff/210001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:76: [in] PP_Resource context, On 2014/04/09 21:54:07, dmichael wrote: > might be good to name graphics3d_context Done. https://codereview.chromium.org/210373003/diff/210001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:85: * to GetBitstreamBuffer(). On 2014/04/09 21:54:07, dmichael wrote: > I wonder if it might be possible for Chrome/PPAPI to handle the throttling? > > One possible way is to only return a BitstreamBuffer to the plugin *only* when > it's appropriate to call Decode again. Then the plugin doesn't have to think at > all about keeping the pipe full; they keep pushing data so long as they have a > place to write it. I tried to think of a way to do this automatically but there are difficulties. 1) Since we want to allow multiple pending Decode calls, we need multiple pending GBB calls, and so we have to be prepared to queue up requests if the plugin calls GBB really fast. I think failing in this case would make writing plugin callbacks a pain. 2) Need a magic number of buffers to allow before we start throttling. I don't know what a good value would be and likely the plugin knows better. 3) We could add this number to the parameters for Initialize(), and have some hard limit in case the plugin sets it too large. It adds complexity to the proxy though. https://codereview.chromium.org/210373003/diff/210001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:85: * to GetBitstreamBuffer(). On 2014/04/09 22:12:41, Ami Fischman wrote: > why GBB() and not Decode() in this sentence? Throttling GBB would limit the number of pending decodes. I've eliminated the GetBitstreamBuffer function, so it is now Decode that needs throttling. https://codereview.chromium.org/210373003/diff/210001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:89: * @param[in] size The size of the buffer in bytes. On 2014/04/09 22:12:41, Ami Fischman wrote: > s/size/capacity/ ? Eliminated GBB. https://codereview.chromium.org/210373003/diff/210001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:115: * completion. On 2014/04/09 22:12:41, Ami Fischman wrote: > what does "completion" mean in this context? > - decoder accepting the buffer > - buffer is on the IPC wire to the decoder > - decoder has finished with the buffer > - something else? Yes, completion means the buffer has been decoded. (proxy received NotifyEndOfBitstreamBuffer). Made the comment say this. https://codereview.chromium.org/210373003/diff/210001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:127: * system. The plugin can call GetPictureBuffer() again in its callback to On 2014/04/09 22:12:41, Ami Fischman wrote: > what callback? The user-provided callback. Made the comment clearer. https://codereview.chromium.org/210373003/diff/210001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:161: * flushing, the plugin should not attempt to decode more video. On 2014/04/09 22:12:41, Ami Fischman wrote: > what about calling reset during flush? I'm going to say the plugin can't call anything during Flush or Reset (until its callback is run) https://codereview.chromium.org/210373003/diff/210001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:177: * calls. Once all decodes are aborted and reset is complete, |callback| will On 2014/04/09 22:12:41, Ami Fischman wrote: > Forbid decode & flush from being called during reset? Done. https://codereview.chromium.org/210373003/diff/210001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:178: * be called. On 2014/04/09 22:12:41, Ami Fischman wrote: > doco impact on ownership of picture and bitstream buffers. Done. https://codereview.chromium.org/210373003/diff/210001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:190: }; On 2014/04/09 22:12:41, Ami Fischman wrote: > How does teardown work? > (plugin needs to be able to indicate that its pending callbacks should not fire) The plugin should just release its references on the video decoder. Pepper proxy code aborts all callbacks at that point. I removed the old Destroy method, since it leaves the resource in a zombie state. https://codereview.chromium.org/210373003/diff/210001/ppapi/cpp/media_codec_v... File ppapi/cpp/media_codec_video_decoder.h (right): https://codereview.chromium.org/210373003/diff/210001/ppapi/cpp/media_codec_v... ppapi/cpp/media_codec_video_decoder.h:43: int32_t Initialize(Graphics3D* context, On 2014/04/09 21:54:07, dmichael wrote: > Shouldn't this be "const Graphics3D&"? I suppose it could be even though we use it in a non-const way (compiles because we only access its pp_resource() member officially). Done.
https://codereview.chromium.org/210373003/diff/230001/ppapi/api/pp_media_code... File ppapi/api/pp_media_codec.idl (right): https://codereview.chromium.org/210373003/diff/230001/ppapi/api/pp_media_code... ppapi/api/pp_media_codec.idl:39: * Texture ID in the plugin's GL context. The plugin can use this to render Could you elaborate on what plugin's GL context is? Is it the one used for decoder init? https://codereview.chromium.org/210373003/diff/230001/ppapi/api/pp_media_code... ppapi/api/pp_media_codec.idl:50: uint32_t texture_target; Could you also document or expose pixel format? Similarly to internal format in http://www.opengl.org/sdk/docs/man2/xhtml/glCopyTexImage2D.xml https://codereview.chromium.org/210373003/diff/230001/ppapi/api/ppb_media_cod... File ppapi/api/ppb_media_codec_video_decoder.idl (right): https://codereview.chromium.org/210373003/diff/230001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:75: [in] PP_MediaCodec_Profile profile, Could we have a function that returns the list of supported profiles? That would save us from trying to init-then-close a bunch of decoders. https://codereview.chromium.org/210373003/diff/230001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:85: * the callback to throttle calls to Decode to some constant number to avoid Could you clarify (or provide an API) to help determine that number? https://codereview.chromium.org/210373003/diff/230001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:95: * the decoder has finished reading the buffer. Does it mean I cannot free picture_id->timestamp mapping at the time of callback, because GetPictureBuffer() may happen even later? If so, given that GetPictureBuffer() may not happen at all for a particular buffer - how do I know when to free picture_id->timestamp mapping? If callback can only happen after both GetPictureBuffer() and RecyclePictureBuffer() have completed, then we could use Decode's callback to free the mapping. https://codereview.chromium.org/210373003/diff/230001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:97: * @return An int32_t containing an error code from <code>pp_errors.h</code>. Could you add clarifications - in which cases I should Reset() vs. re-create the decoder vs. forget about the video? My goal is to be able to skip some frames and continue when it makes sense. https://codereview.chromium.org/210373003/diff/230001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:107: * Gets the next picture buffer from the decoder. When the plugin is finished Would be nice to clarify that the picture is available upon the invocation of the callback. https://codereview.chromium.org/210373003/diff/230001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:109: * system. The plugin can call GetPictureBuffer() again at any time after I'm not sure I understand the significance of this statement... Is it OK to have multiple concurrent GetPictureBuffer() calls outstanding? What happens if several of them are outstanding? Will callbacks be somehow invoked in the order of Decode calls? Or the callbacks can be called in a random order (which implies that I have to watch out and order them myself)? Should we discourage this pattern? Would be great to document. https://codereview.chromium.org/210373003/diff/230001/ppapi/cpp/media_codec_v... File ppapi/cpp/media_codec_video_decoder.h (right): https://codereview.chromium.org/210373003/diff/230001/ppapi/cpp/media_codec_v... ppapi/cpp/media_codec_video_decoder.h:47: int32_t GetBitstreamBuffer( Is this documented somewhere? I also don't see it in the .cc file
https://codereview.chromium.org/210373003/diff/210001/ppapi/api/ppb_media_cod... File ppapi/api/ppb_media_codec_video_decoder.idl (right): https://codereview.chromium.org/210373003/diff/210001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:190: }; On 2014/04/11 17:15:16, bbudge wrote: > On 2014/04/09 22:12:41, Ami Fischman wrote: > > How does teardown work? > > (plugin needs to be able to indicate that its pending callbacks should not > fire) > > The plugin should just release its references on the video decoder. Pepper proxy > code aborts all callbacks at that point. I removed the old Destroy method, since > it leaves the resource in a zombie state. Would be great to document this.
Lots of changes to the API. In pp_media_codec.idl: PP_MEDIACODEC_PROFILE -> PP_MEDIACODEC_VIDEOPROFILE. In ppb_media_codec_video_decoder.idl: Decode now throttles itself. The plugin can have only 1 call pending at any given time. Multiple decodes can be inflight internally though. GetPictureBuffer -> GetPicture RecyclePictureBuffer -> RecyclePicture. PTAL https://codereview.chromium.org/210373003/diff/210001/ppapi/api/ppb_media_cod... File ppapi/api/ppb_media_codec_video_decoder.idl (right): https://codereview.chromium.org/210373003/diff/210001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:77: [in] PP_MediaCodec_Profile profile, On 2014/04/09 22:18:38, Ami Fischman wrote: > On 2014/04/09 21:54:07, dmichael wrote: > > Do you have a function yet for getting supported profiles? > > > > Do we want to consider a "RecommendProfile" kind of function, where you can > pass > > some information about the video you have and have the decoder tell you what > > profile you should use? Or: > > 1) Treat this profile parameter as a suggestion, and the decoder automatically > > figures out the best applicable profile the hardware can support > > or... > > 2) Detect the appropriate profile automatically from the stream > > > > (I have to admit I know little enough about decoding to know if 1 and 2 are > > insane) > > profile is a property of the video to be decoded. It does you no good to init > the decoder at a higher profile, and it is likely to fail during decode if you > initialize with a lower profile. > There is active harm in init'ing at a higher-than-needed profile because HW > resources will be consumed (reducing the available decoding power for other > videos concurrently decoding), so I don't think we want to do #1. > > The profile is usually stored in the muxing container (e.g. mp4), not in the > media bitstream (e.g. h.264) so I don't think #2 works in general. Also, you > don't want to find out that the decoder can't handle your stream part-way > through (because mid-stream fallback is a hard problem), so Initialize() is the > right time to specify the profile. So based on this discussion, I don't think we can provide assistance to the plugin here. The plugin processes its stream to determine the codec profile and then attempts to Initialize at that level. https://codereview.chromium.org/210373003/diff/210001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:85: * to GetBitstreamBuffer(). On 2014/04/11 17:15:16, bbudge wrote: > On 2014/04/09 22:12:41, Ami Fischman wrote: > > why GBB() and not Decode() in this sentence? > > Throttling GBB would limit the number of pending decodes. I've eliminated the > GetBitstreamBuffer function, so it is now Decode that needs throttling. I've changed the API to eliminate GBB. I've also changed the semantics of Decode so only one call can be pending. Internally, the proxy now throttles requests for new shm buffers to solve the problem. It's better to do this in the proxy, where we can better determine appropriate limits, rather than leaving this to the plugin developer to guess. https://codereview.chromium.org/210373003/diff/210001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:115: * completion. On 2014/04/11 17:15:16, bbudge wrote: > On 2014/04/09 22:12:41, Ami Fischman wrote: > > what does "completion" mean in this context? > > - decoder accepting the buffer > > - buffer is on the IPC wire to the decoder > > - decoder has finished with the buffer > > - something else? > > Yes, completion means the buffer has been decoded. (proxy received > NotifyEndOfBitstreamBuffer). > Made the comment say this. I've changed the semantics slightly. Now this means the plugin can release its buffer and make another call to Decode. https://codereview.chromium.org/210373003/diff/210001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:161: * flushing, the plugin should not attempt to decode more video. On 2014/04/11 17:15:16, bbudge wrote: > On 2014/04/09 22:12:41, Ami Fischman wrote: > > what about calling reset during flush? > > I'm going to say the plugin can't call anything during Flush or Reset (until its > callback is run) Also, Flush is to signal end-of-stream, so the plugin can shut down a video gracefully. https://codereview.chromium.org/210373003/diff/210001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:190: }; On 2014/04/11 22:16:28, igorc wrote: > On 2014/04/11 17:15:16, bbudge wrote: > > On 2014/04/09 22:12:41, Ami Fischman wrote: > > > How does teardown work? > > > (plugin needs to be able to indicate that its pending callbacks should not > > fire) > > > > The plugin should just release its references on the video decoder. Pepper > proxy > > code aborts all callbacks at that point. I removed the old Destroy method, > since > > it leaves the resource in a zombie state. > > Would be great to document this. It's a Pepper convention, but I'll add a note at the top usage summary. https://codereview.chromium.org/210373003/diff/230001/ppapi/api/pp_media_code... File ppapi/api/pp_media_codec.idl (right): https://codereview.chromium.org/210373003/diff/230001/ppapi/api/pp_media_code... ppapi/api/pp_media_codec.idl:39: * Texture ID in the plugin's GL context. The plugin can use this to render On 2014/04/11 22:13:18, igorc wrote: > Could you elaborate on what plugin's GL context is? Is it the one used for > decoder init? Done. https://codereview.chromium.org/210373003/diff/230001/ppapi/api/pp_media_code... ppapi/api/pp_media_codec.idl:50: uint32_t texture_target; On 2014/04/11 22:13:18, igorc wrote: > Could you also document or expose pixel format? Similarly to internal format in > http://www.opengl.org/sdk/docs/man2/xhtml/glCopyTexImage2D.xml Done. https://codereview.chromium.org/210373003/diff/230001/ppapi/api/ppb_media_cod... File ppapi/api/ppb_media_codec_video_decoder.idl (right): https://codereview.chromium.org/210373003/diff/230001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:75: [in] PP_MediaCodec_Profile profile, On 2014/04/11 22:13:18, igorc wrote: > Could we have a function that returns the list of supported profiles? That would > save us from trying to init-then-close a bunch of decoders. See the comments on patchset #1. In short, the plugin analyzes its video to determine the codec profile, then tries to Initialize. I don't know of an easy way to determine the supported profiles on the current platform, or even if it's possible to do so. https://codereview.chromium.org/210373003/diff/230001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:85: * the callback to throttle calls to Decode to some constant number to avoid On 2014/04/11 22:13:18, igorc wrote: > Could you clarify (or provide an API) to help determine that number? I've changed the behavior of Decode. Now only a single pending call is allowed. The plugin has wait until the decoder signals completion (either by returning PP_OK or running the callback) before it can release its buffer and make another call. The proxy is not doing the throttling (necessary to limit the number of shm memory buffers that are created. https://codereview.chromium.org/210373003/diff/230001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:95: * the decoder has finished reading the buffer. On 2014/04/11 22:13:18, igorc wrote: > Does it mean I cannot free picture_id->timestamp mapping at the time of > callback, because GetPictureBuffer() may happen even later? > > If so, given that GetPictureBuffer() may not happen at all for a particular > buffer - how do I know when to free picture_id->timestamp mapping? > > If callback can only happen after both GetPictureBuffer() and > RecyclePictureBuffer() have completed, then we could use Decode's callback to > free the mapping. True, not every Decode results in a picture. Since pictures are returned in order, the plugin would need a way to determine which id's are older and therefore never going to be received in a picture. I'll try to add this to the example to get a feel for difficulty. https://codereview.chromium.org/210373003/diff/230001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:97: * @return An int32_t containing an error code from <code>pp_errors.h</code>. On 2014/04/11 22:13:18, igorc wrote: > Could you add clarifications - in which cases I should Reset() vs. re-create the > decoder vs. forget about the video? My goal is to be able to skip some frames > and continue when it makes sense. In that case, it's better to call Reset, since all the textures and shm buffers can be reused. But either approach works. See the improved comments on Flush and Reset. https://codereview.chromium.org/210373003/diff/230001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:107: * Gets the next picture buffer from the decoder. When the plugin is finished On 2014/04/11 22:13:18, igorc wrote: > Would be nice to clarify that the picture is available upon the invocation of > the callback. Good you bring this up. The plugin may fall behind so we buffer pictures we receive in the proxy. So it's possible for this to return immediately with PP_OK. Updated the comment. https://codereview.chromium.org/210373003/diff/230001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:109: * system. The plugin can call GetPictureBuffer() again at any time after On 2014/04/11 22:13:18, igorc wrote: > I'm not sure I understand the significance of this statement... Is it OK to have > multiple concurrent GetPictureBuffer() calls outstanding? > > What happens if several of them are outstanding? Will callbacks be somehow > invoked in the order of Decode calls? Or the callbacks can be called in a random > order (which implies that I have to watch out and order them myself)? > > Should we discourage this pattern? > > Would be great to document. Only 1 GetPictureBuffer call may be outstanding. Calling again will return PP_ERROR_INPROGRESS. I'll try to make this clearer. https://codereview.chromium.org/210373003/diff/230001/ppapi/cpp/media_codec_v... File ppapi/cpp/media_codec_video_decoder.h (right): https://codereview.chromium.org/210373003/diff/230001/ppapi/cpp/media_codec_v... ppapi/cpp/media_codec_video_decoder.h:47: int32_t GetBitstreamBuffer( On 2014/04/11 22:13:18, igorc wrote: > Is this documented somewhere? I also don't see it in the .cc file Whoops, should be deleted.
I think this is a nice simplification, thank you https://codereview.chromium.org/210373003/diff/330001/ppapi/api/ppb_media_cod... File ppapi/api/ppb_media_codec_video_decoder.idl (right): https://codereview.chromium.org/210373003/diff/330001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:29: * - After Flush or Reset, the plugin can release any references on the decoder any->all ? https://codereview.chromium.org/210373003/diff/330001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:104: [in] uint32_t user_id, Is the expectation that this id will be different for each call to Decode? The name "user_id" makes it sound more static than that. Should it be "buffer_id" or something, instead? Or would it maybe suffice to eliminate this parameter and use |buffer| for this purpose? https://codereview.chromium.org/210373003/diff/330001/ppapi/cpp/media_codec_v... File ppapi/cpp/media_codec_video_decoder.cc (right): https://codereview.chromium.org/210373003/diff/330001/ppapi/cpp/media_codec_v... ppapi/cpp/media_codec_video_decoder.cc:66: return PP_ERROR_NOINTERFACE; cc.MayForce? https://codereview.chromium.org/210373003/diff/330001/ppapi/cpp/media_codec_v... File ppapi/cpp/media_codec_video_decoder.h (right): https://codereview.chromium.org/210373003/diff/330001/ppapi/cpp/media_codec_v... ppapi/cpp/media_codec_video_decoder.h:63: const CompletionCallback& cc); nit: I'm not sure why we seem to put parameters this way in IDL, but I think in the cpp we usually follow more conventional chromium style. This might be a good time to try out clang-format :-). You can run: clang-format -i -style=Chromium ppapi/cpp/media_codec_video_decoder.* to get just these files. https://codereview.chromium.org/210373003/diff/330001/ppapi/ppapi_sources.gypi File ppapi/ppapi_sources.gypi (right): https://codereview.chromium.org/210373003/diff/330001/ppapi/ppapi_sources.gyp... ppapi/ppapi_sources.gypi:67: 'c/ppb_media_codec_video_decoder.h', This and others seem out of order in this file. https://codereview.chromium.org/210373003/diff/330001/ppapi/thunk/ppb_media_c... File ppapi/thunk/ppb_media_codec_video_decoder_api.h (right): https://codereview.chromium.org/210373003/diff/330001/ppapi/thunk/ppb_media_c... ppapi/thunk/ppb_media_codec_video_decoder_api.h:39: scoped_refptr<TrackedCallback> callback) = 0; nit: Don't some of these fit on 1 line? Might be another good job for clang-format.
https://codereview.chromium.org/210373003/diff/330001/ppapi/api/ppb_media_cod... File ppapi/api/ppb_media_codec_video_decoder.idl (right): https://codereview.chromium.org/210373003/diff/330001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:29: * - After Flush or Reset, the plugin can release any references on the decoder On 2014/04/17 20:23:26, dmichael wrote: > any->all ? Done. https://codereview.chromium.org/210373003/diff/330001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:104: [in] uint32_t user_id, On 2014/04/17 20:23:26, dmichael wrote: > Is the expectation that this id will be different for each call to Decode? The > name "user_id" makes it sound more static than that. Should it be "buffer_id" or > something, instead? Or would it maybe suffice to eliminate this parameter and > use |buffer| for this purpose? It was 'picture_id' and from your comment I think that's a better name. I think it's needed since |buffer| might not be unique. https://codereview.chromium.org/210373003/diff/330001/ppapi/cpp/media_codec_v... File ppapi/cpp/media_codec_video_decoder.h (right): https://codereview.chromium.org/210373003/diff/330001/ppapi/cpp/media_codec_v... ppapi/cpp/media_codec_video_decoder.h:63: const CompletionCallback& cc); On 2014/04/17 20:23:26, dmichael wrote: > nit: I'm not sure why we seem to put parameters this way in IDL, but I think in > the cpp we usually follow more conventional chromium style. > > This might be a good time to try out clang-format :-). You can run: > clang-format -i -style=Chromium ppapi/cpp/media_codec_video_decoder.* > to get just these files. Done. https://codereview.chromium.org/210373003/diff/330001/ppapi/ppapi_sources.gypi File ppapi/ppapi_sources.gypi (right): https://codereview.chromium.org/210373003/diff/330001/ppapi/ppapi_sources.gyp... ppapi/ppapi_sources.gypi:67: 'c/ppb_media_codec_video_decoder.h', On 2014/04/17 20:23:26, dmichael wrote: > This and others seem out of order in this file. The names changed during the course of this. Done. https://codereview.chromium.org/210373003/diff/330001/ppapi/thunk/ppb_media_c... File ppapi/thunk/ppb_media_codec_video_decoder_api.h (right): https://codereview.chromium.org/210373003/diff/330001/ppapi/thunk/ppb_media_c... ppapi/thunk/ppb_media_codec_video_decoder_api.h:39: scoped_refptr<TrackedCallback> callback) = 0; On 2014/04/17 20:23:26, dmichael wrote: > nit: Don't some of these fit on 1 line? > > Might be another good job for clang-format. Done.
https://codereview.chromium.org/210373003/diff/330001/ppapi/api/ppb_media_cod... File ppapi/api/ppb_media_codec_video_decoder.idl (right): https://codereview.chromium.org/210373003/diff/330001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:104: [in] uint32_t user_id, On 2014/04/18 17:03:17, bbudge wrote: > On 2014/04/17 20:23:26, dmichael wrote: > > Is the expectation that this id will be different for each call to Decode? The > > name "user_id" makes it sound more static than that. Should it be "buffer_id" > or > > something, instead? Or would it maybe suffice to eliminate this parameter and > > use |buffer| for this purpose? > > It was 'picture_id' and from your comment I think that's a better name. I think > it's needed since |buffer| might not be unique. But will you ever have more than 1 Decode call in flight with the same buffer? I thought you wouldn't. I thought it was probably "unique enough" I'm just worried this will be confusing; that plugin authors might not know what they should pass here. If I understand right, they're supposed to make up their own unique ID? Is it legal for them to pass 0 all the time, if they somehow don't actually need to associate Decode calls with GetPicture results? https://codereview.chromium.org/210373003/diff/350001/ppapi/cpp/media_codec_v... File ppapi/cpp/media_codec_video_decoder.h (right): https://codereview.chromium.org/210373003/diff/350001/ppapi/cpp/media_codec_v... ppapi/cpp/media_codec_video_decoder.h:46: /// @param[in] graphics3d_context A <code>Graphics3D</code> resource to use nit: comment doesn't match param name
https://codereview.chromium.org/210373003/diff/330001/ppapi/api/ppb_media_cod... File ppapi/api/ppb_media_codec_video_decoder.idl (right): https://codereview.chromium.org/210373003/diff/330001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:104: [in] uint32_t user_id, On 2014/04/21 17:25:33, dmichael wrote: > On 2014/04/18 17:03:17, bbudge wrote: > > On 2014/04/17 20:23:26, dmichael wrote: > > > Is the expectation that this id will be different for each call to Decode? > The > > > name "user_id" makes it sound more static than that. Should it be > "buffer_id" > > or > > > something, instead? Or would it maybe suffice to eliminate this parameter > and > > > use |buffer| for this purpose? > > > > It was 'picture_id' and from your comment I think that's a better name. I > think > > it's needed since |buffer| might not be unique. > But will you ever have more than 1 Decode call in flight with the same buffer? I > thought you wouldn't. I thought it was probably "unique enough" > > I'm just worried this will be confusing; that plugin authors might not know what > they should pass here. If I understand right, they're supposed to make up their > own unique ID? Is it legal for them to pass 0 all the time, if they somehow > don't actually need to associate Decode calls with GetPicture results? On further reflection, I think a better name than user_id or picture_id is 'decode_id'. This parameter is optional. I've changed the docs to make this clear. As far as the plugin is concerned, it only has a single Decode pending at any time. Internally, however, there can be several Decodes in flight in the steady state, most of them causing a new picture to be generated. The plugin could call Decode with a given 'buffer', receive completion, and send off another Decode from the same 'buffer' before getting the picture from the first one. It would be unclear which Decode caused the picture. That's why this is needed. https://codereview.chromium.org/210373003/diff/330001/ppapi/cpp/media_codec_v... File ppapi/cpp/media_codec_video_decoder.cc (right): https://codereview.chromium.org/210373003/diff/330001/ppapi/cpp/media_codec_v... ppapi/cpp/media_codec_video_decoder.cc:66: return PP_ERROR_NOINTERFACE; On 2014/04/17 20:23:26, dmichael wrote: > cc.MayForce? Missed this one. Done. https://codereview.chromium.org/210373003/diff/350001/ppapi/cpp/media_codec_v... File ppapi/cpp/media_codec_video_decoder.h (right): https://codereview.chromium.org/210373003/diff/350001/ppapi/cpp/media_codec_v... ppapi/cpp/media_codec_video_decoder.h:46: /// @param[in] graphics3d_context A <code>Graphics3D</code> resource to use On 2014/04/21 17:25:34, dmichael wrote: > nit: comment doesn't match param name Done.
lgtm As discussed offline, it might be a good idea to look in to MediaSourceExtensions to see if it might influence this. But I think we can go ahead with implementing as a dev channel API to start getting some experience with this API. https://codereview.chromium.org/210373003/diff/370001/ppapi/api/ppb_media_cod... File ppapi/api/ppb_media_codec_video_decoder.idl (right): https://codereview.chromium.org/210373003/diff/370001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:93: * @param[in] decode_id An optional value that can be used to associate calls optional suggestion: "An optional value, chosen by the plugin, ..."
The CQ bit was checked by bbudge@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bbudge@chromium.org/210373003/390001
https://codereview.chromium.org/210373003/diff/370001/ppapi/api/ppb_media_cod... File ppapi/api/ppb_media_codec_video_decoder.idl (right): https://codereview.chromium.org/210373003/diff/370001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:93: * @param[in] decode_id An optional value that can be used to associate calls On 2014/04/21 20:53:54, dmichael wrote: > optional suggestion: > "An optional value, chosen by the plugin, ..." Done.
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
native_client_sdk lgtm
The CQ bit was checked by bbudge@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bbudge@chromium.org/210373003/390001
lgtm, see a few requests for clarification https://codereview.chromium.org/210373003/diff/230001/ppapi/api/ppb_media_cod... File ppapi/api/ppb_media_codec_video_decoder.idl (right): https://codereview.chromium.org/210373003/diff/230001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:97: * @return An int32_t containing an error code from <code>pp_errors.h</code>. On 2014/04/16 00:51:05, bbudge wrote: > On 2014/04/11 22:13:18, igorc wrote: > > Could you add clarifications - in which cases I should Reset() vs. re-create > the > > decoder vs. forget about the video? My goal is to be able to skip some frames > > and continue when it makes sense. > > In that case, it's better to call Reset, since all the textures and shm buffers > can be reused. But either approach works. See the improved comments on Flush and > Reset. Could you add it to the comments? Basically that a failed Decode() call should be followed by a Reset() rather than by more Decode() calls. In other words - that there's no recovery after even one failed Decode() call. https://codereview.chromium.org/210373003/diff/330001/ppapi/api/pp_media_code... File ppapi/api/pp_media_codec.idl (right): https://codereview.chromium.org/210373003/diff/330001/ppapi/api/pp_media_code... ppapi/api/pp_media_codec.idl:9: enum PP_MediaCodec_VideoProfile { The enum here is video-specific, however the file name is generic. On the other hand, ppb_media_codec_video_decoder.idl is video-specific... Just wanted to make sure this inconsistency is intentional. https://codereview.chromium.org/210373003/diff/330001/ppapi/api/ppb_media_cod... File ppapi/api/ppb_media_codec_video_decoder.idl (right): https://codereview.chromium.org/210373003/diff/330001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:127: [out] PP_MediaCodec_Picture picture, What is the order of resolved pictures? Does it somehow correspond to the order of Decode() calls?
Igor, I have an implementation patch which you can start playing with. It would be great to see if it works for you or what else you need. https://codereview.chromium.org/213313003/ https://codereview.chromium.org/210373003/diff/230001/ppapi/api/ppb_media_cod... File ppapi/api/ppb_media_codec_video_decoder.idl (right): https://codereview.chromium.org/210373003/diff/230001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:97: * @return An int32_t containing an error code from <code>pp_errors.h</code>. On 2014/04/21 23:12:48, igorc wrote: > On 2014/04/16 00:51:05, bbudge wrote: > > On 2014/04/11 22:13:18, igorc wrote: > > > Could you add clarifications - in which cases I should Reset() vs. re-create > > the > > > decoder vs. forget about the video? My goal is to be able to skip some > frames > > > and continue when it makes sense. > > > > In that case, it's better to call Reset, since all the textures and shm > buffers > > can be reused. But either approach works. See the improved comments on Flush > and > > Reset. > > Could you add it to the comments? Basically that a failed Decode() call should > be followed by a Reset() rather than by more Decode() calls. In other words - > that there's no recovery after even one failed Decode() call. Good idea. I'll add them in a later CL, when it becomes clearer what the errors can be. I think it will require some new PP_Error codes. Also, I think it makes sense for them to be returned by the GetPicture function, since from the plugin's point of view, Decodes succeed before the gpu gets the bitstream buffer. https://codereview.chromium.org/210373003/diff/330001/ppapi/api/pp_media_code... File ppapi/api/pp_media_codec.idl (right): https://codereview.chromium.org/210373003/diff/330001/ppapi/api/pp_media_code... ppapi/api/pp_media_codec.idl:9: enum PP_MediaCodec_VideoProfile { On 2014/04/21 23:12:48, igorc wrote: > The enum here is video-specific, however the file name is generic. On the other > hand, ppb_media_codec_video_decoder.idl is video-specific... Just wanted to make > sure this inconsistency is intentional. It's a little speculative, but the file is intended to contain supporting structs for video encoding and even audio codecs. That's why I call this 'VideoProfile' here - so we could have 'AudioProfile' as well. https://codereview.chromium.org/210373003/diff/330001/ppapi/api/ppb_media_cod... File ppapi/api/ppb_media_codec_video_decoder.idl (right): https://codereview.chromium.org/210373003/diff/330001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:127: [out] PP_MediaCodec_Picture picture, On 2014/04/21 23:12:48, igorc wrote: > What is the order of resolved pictures? Does it somehow correspond to the order > of Decode() calls? According to Ami, pictures are always returned in presentation order. I don't know enough about how video is encoded to say if that implies that decode order is the same as presentation order. I'll make sure this is the case and add a comment.
The CQ bit was unchecked by bbudge@chromium.org
On Mon, Apr 21, 2014 at 4:29 PM, <bbudge@chromium.org> wrote: > https://codereview.chromium.org/210373003/diff/330001/ > ppapi/api/ppb_media_codec_video_decoder.idl > File ppapi/api/ppb_media_codec_video_decoder.idl (right): > > https://codereview.chromium.org/210373003/diff/330001/ > ppapi/api/ppb_media_codec_video_decoder.idl#newcode127 > ppapi/api/ppb_media_codec_video_decoder.idl:127: [out] > PP_MediaCodec_Picture picture, > On 2014/04/21 23:12:48, igorc wrote: > >> What is the order of resolved pictures? Does it somehow correspond to >> > the order > >> of Decode() calls? >> > > According to Ami, pictures are always returned in presentation order. I > don't know enough about how video is encoded to say if that implies that > decode order is the same as presentation order. It explicitly means these orders are _not_ the same. Decode order is the order you feed bitstream to the decoder. Presentation order is the order the decoder feeds you pictures, and the order in which you need to feed those pictures to the renderer/compositor/screen. As an example consider the diagram at http://en.wikipedia.org/wiki/Inter_frame#Typical_Group_Of_Pictures_.28GOP.29_... the frames are ordered from left to right in presentation order in which the first four frames are IBBP. The decode order of those same 4 frames is IPBB. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/210373003/diff/390001/ppapi/api/pp_media_code... File ppapi/api/pp_media_codec.idl (right): https://codereview.chromium.org/210373003/diff/390001/ppapi/api/pp_media_code... ppapi/api/pp_media_codec.idl:48: * The pixel format of the texture is GL_RGBA. Is that really true? E.g. https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu... https://codereview.chromium.org/210373003/diff/390001/ppapi/api/ppb_media_cod... File ppapi/api/ppb_media_codec_video_decoder.idl (right): https://codereview.chromium.org/210373003/diff/390001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:19: * Typical usage: IIUC platform errors from the HW get delivered to an arbitrary callback (e.g. Initialize()'s callback, or Decode()'s callback, or Flush()'s callback, etc), based on whatever is currently outstanding from the plugin (if anything). That seems unfortunate, but maybe this is forced by pepper style? https://codereview.chromium.org/210373003/diff/390001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:30: * it. To avoid receiving aborted callbacks, call Flush() and wait for It seems unfortunate to have to wait for the decode pipeline to drain to be able to exit? https://codereview.chromium.org/210373003/diff/390001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:89: * or frame numbers to pictures.) It's OK to pass 0 if this isn't needed. I would just say that this value is opaque to the API (since "0" isn't special).
https://codereview.chromium.org/210373003/diff/390001/ppapi/api/pp_media_code... File ppapi/api/pp_media_codec.idl (right): https://codereview.chromium.org/210373003/diff/390001/ppapi/api/pp_media_code... ppapi/api/pp_media_codec.idl:48: * The pixel format of the texture is GL_RGBA. On 2014/04/22 00:27:40, Ami Fischman wrote: > Is that really true? > > E.g. > https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu... Going back through some emails, I see you mentioned it's "guaranteed" to be GL_BGRA. Is that true? https://codereview.chromium.org/210373003/diff/390001/ppapi/api/ppb_media_cod... File ppapi/api/ppb_media_codec_video_decoder.idl (right): https://codereview.chromium.org/210373003/diff/390001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:19: * Typical usage: On 2014/04/22 00:27:40, Ami Fischman wrote: > IIUC platform errors from the HW get delivered to an arbitrary callback (e.g. > Initialize()'s callback, or Decode()'s callback, or Flush()'s callback, etc), > based on whatever is currently outstanding from the plugin (if anything). That > seems unfortunate, but maybe this is forced by pepper style? I can choose how to return the error, but yes, there's no way to have a separate channel for these errors with this API, short of adding a new GetError function with a callback. I think the best thing to do is to return the error code to all outstanding callbacks when it's received on the plugin side. https://codereview.chromium.org/210373003/diff/390001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:30: * it. To avoid receiving aborted callbacks, call Flush() and wait for On 2014/04/22 00:27:40, Ami Fischman wrote: > It seems unfortunate to have to wait for the decode pipeline to drain to be able > to exit? I think we can avoid this in the resource destructor. https://codereview.chromium.org/210373003/diff/390001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:89: * or frame numbers to pictures.) It's OK to pass 0 if this isn't needed. On 2014/04/22 00:27:40, Ami Fischman wrote: > I would just say that this value is opaque to the API (since "0" isn't special). Done.
https://codereview.chromium.org/210373003/diff/390001/ppapi/api/ppb_media_cod... File ppapi/api/ppb_media_codec_video_decoder.idl (right): https://codereview.chromium.org/210373003/diff/390001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:19: * Typical usage: On 2014/04/23 18:35:22, bbudge wrote: > On 2014/04/22 00:27:40, Ami Fischman wrote: > > IIUC platform errors from the HW get delivered to an arbitrary callback (e.g. > > Initialize()'s callback, or Decode()'s callback, or Flush()'s callback, etc), > > based on whatever is currently outstanding from the plugin (if anything). > That > > seems unfortunate, but maybe this is forced by pepper style? > > I can choose how to return the error, but yes, there's no way to have a separate > channel for these errors with this API, short of adding a new GetError function > with a callback. I think the best thing to do is to return the error code to all > outstanding callbacks when it's received on the plugin side. My worry is what happens if there are no outstanding callbacks. (e.g. the error happens because the GPU process crashed after a Flush was done & called-back, but before the next Decode or GetPicture call is made). I guess you'd eventually get the error when the next API call is made but sad for that delay. https://codereview.chromium.org/210373003/diff/390001/ppapi/api/ppb_media_cod... ppapi/api/ppb_media_codec_video_decoder.idl:30: * it. To avoid receiving aborted callbacks, call Flush() and wait for On 2014/04/23 18:35:22, bbudge wrote: > On 2014/04/22 00:27:40, Ami Fischman wrote: > > It seems unfortunate to have to wait for the decode pipeline to drain to be > able > > to exit? > > I think we can avoid this in the resource destructor. Can it be hidden from the plugin? IOW make it so that when the last reference is released it guarantees no more callbacks? https://codereview.chromium.org/210373003/diff/430001/ppapi/api/pp_media_code... File ppapi/api/pp_media_codec.idl (right): https://codereview.chromium.org/210373003/diff/430001/ppapi/api/pp_media_code... ppapi/api/pp_media_codec.idl:48: * The pixel format of the texture is GL_BGRA. On 2014/04/23 18:35:22, bbudge wrote: > On 2014/04/22 00:27:40, Ami Fischman wrote: > > Is that really true? > > > > E.g. > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu... > > Going back through some emails, I see you mentioned it's "guaranteed" to be > GL_BGRA. Is that true? Per IM convo, I don't believe this is guaranteed anywhere in writing, but I suspect it is true in practice. Definitely worth adding a test for it in your impl CL if you want to promise it here. (related: r230229)
After implementing the software fallback path, I've made a few tweaks to the API. 1) Changed Initialize() behavior. Now the plugin may make multiple calls in case of failure, in order to make probing capabilities less tedious (before, the plugin would have to create a new one on failure.) 2) Changed Flush behavior. Since the plugin must pump the decoder for pictures, it's hard for it to avoid having a pending GetPicture call when a call to Flush completes. I abort any pending GetPicture call at this point to signal that no pictures are available. The alternative would be to just leave it pending, but that seems to make plugin coding more difficult. 3) Added more docs on bitstream formats at the top.
still lgtm
The CQ bit was checked by bbudge@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bbudge@chromium.org/210373003/470001
The CQ bit was unchecked by bbudge@chromium.org
A lot of renaming going on. 1) Fix PP_VideoProfile enum. e.g. PP_H264PROFILE_BASELINE -> PP_VIDEOPROFILE_H264BASELINE 2) PP_MediaCodecPicture -> PP_VideoPicture 3) PPB_MediaCodecVideoDecoder -> PPB_VideoDecoder Supporting types and file names changed accordingly.
The CQ bit was checked by bbudge@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bbudge@chromium.org/210373003/560001
The CQ bit was unchecked by bbudge@chromium.org
Message was sent while issue was closed.
Committed patchset #13 manually as r268618 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
