Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
270213004:
Implement Pepper PPB_VideoDecoder interface. (Closed)
|
Created:
6 years, 7 months ago by bbudge Modified:
4 years, 3 months ago Reviewers:
jar (doing other things), Ami GONE FROM CHROMIUM, dielui94, piman, dmichael (off chromium), yzshen1, Tom Sepez, thangah002 CC:
chromium-reviews, binji+watch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yusukes+watch_chromium.org, noelallen1, tzik, jam, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, teravest+watch_chromium.org, darin-cc_chromium.org, raymes+watch_chromium.org, nfullagar1, piman+watch_chromium.org, darin (slow to review), ben+mojo_chromium.org, ihf+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImplement Pepper PPB_VideoDecoder interface.
Adds resource and host, unit test for the resource, and an example plugin.
Implements only the hardware accelerated case. Software fallback will be
in a follow-on CL.
Adds two new PP_Error codes:
PP_ERROR_UNREADABLE_INPUT
PP_ERROR_RESOURCE_FAILED
BUG=281689
R=dmichael@chromium.org, fischman@chromium.org, jar@chromium.org, piman@chromium.org, tsepez@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=273920
Patch Set 1 : #
Total comments: 59
Patch Set 2 : Rebase, update to PPAPI message map macros. #
Total comments: 43
Patch Set 3 : Address remaining comments, including Yuzhu's. #Patch Set 4 : Fix compile (missing return in new fn). #
Total comments: 4
Patch Set 5 : Address Yuzhu's follow up comments. #
Total comments: 3
Patch Set 6 : Changed docs for PP_ERROR_PLATFORM_FAILED. #
Total comments: 17
Patch Set 7 : Address David's and Antoine's lesser concerns. #Patch Set 8 : Make new error codes and docs a little clearer. #Patch Set 9 : Bound the number of shared memory buffers. #Patch Set 10 : Easier buffer passing, bounded # shm buffers, fix decode_id handling. #Patch Set 11 : Rebase. #
Total comments: 39
Patch Set 12 : Address David's comments. Reduce latency when creating or resizing a shm buffer. #Patch Set 13 : Fix constants in header. #Patch Set 14 : Rebase. #
Total comments: 6
Patch Set 15 : Make shared memory creation synchronous. #
Total comments: 18
Patch Set 16 : Clean up example plugin cc usage. #Patch Set 17 : Address Antoine's comments. #Patch Set 18 : DCHECK that shm bufffers are free after Flush/Reset. #
Total comments: 45
Patch Set 19 : Address Ami's comments. #
Total comments: 12
Patch Set 20 : Fix uid wrapping, int/uint issues. #Patch Set 21 : Fix shm problem in Win ppapi_unittests. #
Total comments: 1
Patch Set 22 : Resource generates uid's, passed in Decode msg to Host. #Patch Set 23 : Disable some unit tests on Win 64 bit builds. #Messages
Total messages: 64 (2 generated)
dmichael, yzshen for pepper proxy tsepez for ppapi_messages.h fischman for ppapi/example/video_decde, pepper_video_decoder_host.* and overall wisdom. piman for OpenGL usage in video_decoder_resource.*
I think your CL is missing new files: idl and generated headers for the new interface.
On 2014/05/08 01:49:42, piman wrote: > I think your CL is missing new files: idl and generated headers for the new > interface. The tries needed to be against a newer revision. The IDL landed yesterday.
https://codereview.chromium.org/270213004/diff/80001/ppapi/proxy/video_decode... File ppapi/proxy/video_decoder_resource.cc (right): https://codereview.chromium.org/270213004/diff/80001/ppapi/proxy/video_decode... ppapi/proxy/video_decoder_resource.cc:51: shm_->Unmap(); nit: that's implicit with the destruction. https://codereview.chromium.org/270213004/diff/80001/ppapi/proxy/video_decode... ppapi/proxy/video_decoder_resource.cc:251: gles2_impl_->GenTextures(1, &texture_id); It's really better to GenTextures(num_textures, array). https://codereview.chromium.org/270213004/diff/80001/ppapi/proxy/video_decode... ppapi/proxy/video_decoder_resource.cc:254: gles2_impl_->BindTexture(texture_target, texture_id); So, these 2 calls modify GL state. Since this comes from a renderer message, it will really happen behind the scenes from the plugin point-of-view, so its state will be surprisingly changed. At the very least we should restore the state, using GetIntegerv with GL_ACTIVE_TEXTURE and GL_TEXTURE_BINDING_* https://codereview.chromium.org/270213004/diff/80001/ppapi/proxy/video_decode... ppapi/proxy/video_decoder_resource.cc:276: if (!mailboxes.empty()) We should check somewhere that mailbox.size() == num_textures https://codereview.chromium.org/270213004/diff/80001/ppapi/proxy/video_decode... ppapi/proxy/video_decoder_resource.cc:362: uint32_t shm_id) { It makes me a bit uncomfortable to have the shm_id round-trip through the renderer. I don't think it's needed since you have the decode_id (so you could keep a queue of pending decodes, where you'd stash the shm id). A compromised renderer could cause the shm_id to be out-of-sync with the decode_id, or be out-of-bounds. It could try to use this to gain elevated privilege to the plugin process, which may have different access (think Flash, that connects to renderers from other origins too). Maybe far-fetched, but I think it's better not to make assumptions about trusting the renderer. https://codereview.chromium.org/270213004/diff/80001/ppapi/proxy/video_decode... ppapi/proxy/video_decoder_resource.cc:367: decode_ids_[shm_id] = decode_id; Is this ok, to use the shm_id (which is just an index in a small range) to tag the decode calls? 2 things I'm worried about: 1- are we ensured OnPluginMsgDecodeComplete will always be called before OnPluginMsgPictureReady? 2- since this shm buffer will be reused at the next Decode, are we ensured we'll get *all* OnPluginMsgPictureReady for this Decode, before the OnPluginMsgDecodeComplete for the next one? Wouldn't it be safer to use decode_id all along the path, since they're monotonically increasing and there's no chance of reuse? I think it'd even make some of the logic simpler. https://codereview.chromium.org/270213004/diff/80001/ppapi/proxy/video_decode... ppapi/proxy/video_decoder_resource.cc:414: RENDERER, Why not allocate the shm by going to the browser? It would avoid round trips (since the renderer has to go to the browser), and not involve the renderer's main thread which can be busy. It could also then be a sync IPC, which would remove the restriction that the user can't touch their buffer until the callback is called. Which probably forces a copy on most patterns (which is a shame, since we copy too), though more likely users will forget and send flaky garbage to the video decoder. https://codereview.chromium.org/270213004/diff/80001/ppapi/proxy/video_decode... File ppapi/proxy/video_decoder_resource.h (right): https://codereview.chromium.org/270213004/diff/80001/ppapi/proxy/video_decode... ppapi/proxy/video_decoder_resource.h:138: typedef std::map<uint32_t, Texture> TextureMap; nit: hash_map https://codereview.chromium.org/270213004/diff/80001/ppapi/proxy/video_decode... ppapi/proxy/video_decoder_resource.h:142: typedef std::map<uint32_t, uint32_t> DecodeIdMap; nit: hash_map
Haven't looked at the example or the plugin side yet... https://codereview.chromium.org/270213004/diff/80001/content/renderer/pepper/... File content/renderer/pepper/pepper_video_decoder_host.cc (right): https://codereview.chromium.org/270213004/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_video_decoder_host.cc:108: if (profile < 0 || profile > PP_VIDEOPROFILE_MAX) Should that be >= PP_VIDEOPROFILE_MAX? It looks like PP_VIDEOPROFILE_MAX will actually map to VP9PROFILE_*, which it appears you are not trying to support yet? https://codereview.chromium.org/270213004/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_video_decoder_host.cc:185: if (decoder_) { nit: What's inside the if block is actually the common case, and you currently return PP_OK even in the unlikely event that decoder_ is NULL. I would instead break early in the NULL case, like: if (!decoder_) { NOTREACHED(); return PP_ERROR_FAILED; } // normal case goes here https://codereview.chromium.org/270213004/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_video_decoder_host.cc:272: const std::vector<gpu::Mailbox>& mailboxes) { I don't see any case where mailboxes will be anything but an empty vector (called from ProvidePictureBuffers above). Maybe you can remove that parameter from the function and message, or am I missing something? https://codereview.chromium.org/270213004/diff/80001/content/renderer/pepper/... File content/renderer/pepper/pepper_video_decoder_host.h (right): https://codereview.chromium.org/270213004/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_video_decoder_host.h:51: public base::SupportsWeakPtr<PepperVideoDecoderHost> { You don't seem to use this base class? https://codereview.chromium.org/270213004/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_video_decoder_host.h:58: virtual int32_t OnResourceMessageReceived( I think this and everything below should be private https://codereview.chromium.org/270213004/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_video_decoder_host.h:103: typedef std::map<uint32_t, ppapi::host::ReplyMessageContext> ReplyMap; Some indication of what the uint32_t is would be nice; a comment or different name for the map? https://codereview.chromium.org/270213004/diff/80001/ppapi/api/pp_errors.idl File ppapi/api/pp_errors.idl (right): https://codereview.chromium.org/270213004/diff/80001/ppapi/api/pp_errors.idl#... ppapi/api/pp_errors.idl:185: PP_ERROR_UNREADABLE_INPUT = -111, Hmm, I'm a little unsure about this error code. It does seem to be more info than BAD_ARGUMENT, but "unreadable input" doesn't sound right to me, either. It's "readable", it just (if I understand right) doesn't conform to the expected format. What do you think of: PP_ERROR_DATA_INVALID or PP_ERROR_CONTENT_INVALID https://codereview.chromium.org/270213004/diff/80001/ppapi/api/pp_errors.idl#... ppapi/api/pp_errors.idl:190: PP_ERROR_PLATFORM_FAILED = -112 This one makes me a little sad. We're exposing more platform differences. But I think it's probably a good idea to add the new error, rather than hide it behing PP_ERROR_FAILED.
Thanks, that certainly is a hefty block of code! https://codereview.chromium.org/270213004/diff/80001/content/renderer/pepper/... File content/renderer/pepper/pepper_video_decoder_host.cc (right): https://codereview.chromium.org/270213004/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_video_decoder_host.cc:83: PPAPI_DISPATCH_HOST_RESOURCE_CALL(PpapiHostMsg_VideoDecoder_Initialize, nit: it's common to indent these two spaces between the IPC_BEGIN_MESSAGE_MAP and the IPC_END_MESSAGE_MAP https://codereview.chromium.org/270213004/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_video_decoder_host.cc:331: DCHECK(it != pending_decodes_.end()); Can this happen for real? Does bitstream_buffer_id come from a trusted source? May want to handle the error case in Release builds. https://codereview.chromium.org/270213004/diff/80001/ppapi/examples/video_dec... File ppapi/examples/video_decode/video_decode.cc (right): https://codereview.chromium.org/270213004/diff/80001/ppapi/examples/video_dec... ppapi/examples/video_decode/video_decode.cc:171: static bool LookingAtNAL(const unsigned char* encoded, size_t pos) { I'd be happier if you passed in the max length as a parameter, rather than just assuming that |encoded| is always kDataLen bytes long. https://codereview.chromium.org/270213004/diff/80001/ppapi/examples/video_dec... ppapi/examples/video_decode/video_decode.cc:173: encoded[pos + 2] == 0 && encoded[pos + 3] == 1; Theoretically, |pos + 3 < kDataLen| could overflow; but we control pos such that this can't happen, right? https://codereview.chromium.org/270213004/diff/80001/ppapi/examples/video_dec... ppapi/examples/video_decode/video_decode.cc:177: static void GetNextFrame(size_t *start_pos, size_t* end_pos) { why pass start_pos by pointer? I didn't see it modified below? https://codereview.chromium.org/270213004/diff/80001/ppapi/examples/video_dec... ppapi/examples/video_decode/video_decode.cc:178: // H264 frames start with 0, 0, 0, 1. nit: this comment might move up to LookingAtNAL(). https://codereview.chromium.org/270213004/diff/80001/ppapi/examples/video_dec... ppapi/examples/video_decode/video_decode.cc:182: while (*end_pos + 3 < kDataLen && !LookingAtNAL(kData, *end_pos)) { I suppose if LookingAtNAL is bullet-proof against overlflow, you can avoid all the tests with +3's here, just let it increment until kDataLen, and avoid the if below. https://codereview.chromium.org/270213004/diff/80001/ppapi/examples/video_dec... ppapi/examples/video_decode/video_decode.cc:265: uint32_t size = static_cast<uint32_t>(end_pos - start_pos); We know this always fits? https://codereview.chromium.org/270213004/diff/80001/ppapi/proxy/ppapi_messag... File ppapi/proxy/ppapi_messages.h (right): https://codereview.chromium.org/270213004/diff/80001/ppapi/proxy/ppapi_messag... ppapi/proxy/ppapi_messages.h:125: IPC_ENUM_TRAITS(PP_VideoProfile) Can we use IPC_ENUM_TRATIS_MAX_VALUE or the like here to get free bounds checking on incoming messages? https://codereview.chromium.org/270213004/diff/80001/ppapi/proxy/video_decode... File ppapi/proxy/video_decoder_resource.cc (right): https://codereview.chromium.org/270213004/diff/80001/ppapi/proxy/video_decode... ppapi/proxy/video_decoder_resource.cc:47: DCHECK(addr_); might want to handle error cases here. I'd suspect that being unable to map memory could occur at any time based on system load, etc. https://codereview.chromium.org/270213004/diff/80001/ppapi/proxy/video_decode... ppapi/proxy/video_decoder_resource.cc:247: const std::vector<gpu::Mailbox>& mailboxes) { Are the mailboxes are supplied by a trusted process?
+mpearson for tools/metrics/histograms https://codereview.chromium.org/270213004/diff/80001/content/renderer/pepper/... File content/renderer/pepper/pepper_video_decoder_host.cc (right): https://codereview.chromium.org/270213004/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_video_decoder_host.cc:83: PPAPI_DISPATCH_HOST_RESOURCE_CALL(PpapiHostMsg_VideoDecoder_Initialize, On 2014/05/08 20:35:07, Tom Sepez wrote: > nit: it's common to indent these two spaces between the IPC_BEGIN_MESSAGE_MAP > and the IPC_END_MESSAGE_MAP We're trying to use clang-format on Pepper code. I think this file is in a directory where everything else matches this style now. https://codereview.chromium.org/270213004/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_video_decoder_host.cc:108: if (profile < 0 || profile > PP_VIDEOPROFILE_MAX) On 2014/05/08 20:27:00, dmichael wrote: > Should that be >= PP_VIDEOPROFILE_MAX? > > It looks like PP_VIDEOPROFILE_MAX will actually map to VP9PROFILE_*, which it > appears you are not trying to support yet? There should be no more VP9 in the API. It isn't supported yet in Chrome. I think this is correct. Additionally, we now bounds check in the IPC. https://codereview.chromium.org/270213004/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_video_decoder_host.cc:185: if (decoder_) { On 2014/05/08 20:27:00, dmichael wrote: > nit: What's inside the if block is actually the common case, and you currently > return PP_OK even in the unlikely event that decoder_ is NULL. I would instead > break early in the NULL case, like: > if (!decoder_) { > NOTREACHED(); > return PP_ERROR_FAILED; > } > // normal case goes here Much better. Done. https://codereview.chromium.org/270213004/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_video_decoder_host.cc:272: const std::vector<gpu::Mailbox>& mailboxes) { On 2014/05/08 20:27:00, dmichael wrote: > I don't see any case where mailboxes will be anything but an empty vector > (called from ProvidePictureBuffers above). Maybe you can remove that parameter > from the function and message, or am I missing something? Removed these for now. https://codereview.chromium.org/270213004/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_video_decoder_host.cc:331: DCHECK(it != pending_decodes_.end()); On 2014/05/08 20:35:07, Tom Sepez wrote: > Can this happen for real? Does bitstream_buffer_id come from a trusted source? > May want to handle the error case in Release builds. This is from the decoder which is trusted. A future CL will connect a software decoder through this code path. Added protection for the release build. https://codereview.chromium.org/270213004/diff/80001/content/renderer/pepper/... File content/renderer/pepper/pepper_video_decoder_host.h (right): https://codereview.chromium.org/270213004/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_video_decoder_host.h:51: public base::SupportsWeakPtr<PepperVideoDecoderHost> { On 2014/05/08 20:27:00, dmichael wrote: > You don't seem to use this base class? I removed all vestiges of the software decoder stuff for this issue. Done. https://codereview.chromium.org/270213004/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_video_decoder_host.h:58: virtual int32_t OnResourceMessageReceived( On 2014/05/08 20:27:00, dmichael wrote: > I think this and everything below should be private Done. https://codereview.chromium.org/270213004/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_video_decoder_host.h:103: typedef std::map<uint32_t, ppapi::host::ReplyMessageContext> ReplyMap; On 2014/05/08 20:27:00, dmichael wrote: > Some indication of what the uint32_t is would be nice; a comment or different > name for the map? Definitely, done. https://codereview.chromium.org/270213004/diff/80001/ppapi/api/pp_errors.idl File ppapi/api/pp_errors.idl (right): https://codereview.chromium.org/270213004/diff/80001/ppapi/api/pp_errors.idl#... ppapi/api/pp_errors.idl:185: PP_ERROR_UNREADABLE_INPUT = -111, On 2014/05/08 20:27:00, dmichael wrote: > Hmm, I'm a little unsure about this error code. It does seem to be more info > than BAD_ARGUMENT, but "unreadable input" doesn't sound right to me, either. > It's "readable", it just (if I understand right) doesn't conform to the expected > format. > > What do you think of: > PP_ERROR_DATA_INVALID > or > PP_ERROR_CONTENT_INVALID I struggled with this one. My first thought was PP_ERROR_MALFORMED_INPUT but I decided to just add what was needed for this API. Your preference? https://codereview.chromium.org/270213004/diff/80001/ppapi/api/pp_errors.idl#... ppapi/api/pp_errors.idl:190: PP_ERROR_PLATFORM_FAILED = -112 On 2014/05/08 20:27:00, dmichael wrote: > This one makes me a little sad. We're exposing more platform differences. But I > think it's probably a good idea to add the new error, rather than hide it behing > PP_ERROR_FAILED. Yes. I'm not adding these lightly. The existing error codes just don't seem appropriate for these. https://codereview.chromium.org/270213004/diff/80001/ppapi/proxy/ppapi_messag... File ppapi/proxy/ppapi_messages.h (right): https://codereview.chromium.org/270213004/diff/80001/ppapi/proxy/ppapi_messag... ppapi/proxy/ppapi_messages.h:125: IPC_ENUM_TRAITS(PP_VideoProfile) On 2014/05/08 20:35:07, Tom Sepez wrote: > Can we use IPC_ENUM_TRATIS_MAX_VALUE or the like here to get free bounds > checking on incoming messages? Done. https://codereview.chromium.org/270213004/diff/80001/ppapi/proxy/video_decode... File ppapi/proxy/video_decoder_resource.cc (right): https://codereview.chromium.org/270213004/diff/80001/ppapi/proxy/video_decode... ppapi/proxy/video_decoder_resource.cc:51: shm_->Unmap(); On 2014/05/08 04:26:04, piman wrote: > nit: that's implicit with the destruction. Done. https://codereview.chromium.org/270213004/diff/80001/ppapi/proxy/video_decode... ppapi/proxy/video_decoder_resource.cc:247: const std::vector<gpu::Mailbox>& mailboxes) { On 2014/05/08 20:35:07, Tom Sepez wrote: > Are the mailboxes are supplied by a trusted process? They are generated by the renderer process. I am removing this parameter for now though, as it's only needed by a future CL. https://codereview.chromium.org/270213004/diff/80001/ppapi/proxy/video_decode... ppapi/proxy/video_decoder_resource.cc:251: gles2_impl_->GenTextures(1, &texture_id); On 2014/05/08 04:26:04, piman wrote: > It's really better to GenTextures(num_textures, array). Done. https://codereview.chromium.org/270213004/diff/80001/ppapi/proxy/video_decode... ppapi/proxy/video_decoder_resource.cc:254: gles2_impl_->BindTexture(texture_target, texture_id); On 2014/05/08 04:26:04, piman wrote: > So, these 2 calls modify GL state. > Since this comes from a renderer message, it will really happen behind the > scenes from the plugin point-of-view, so its state will be surprisingly changed. > > At the very least we should restore the state, using GetIntegerv with > GL_ACTIVE_TEXTURE and GL_TEXTURE_BINDING_* I've changed the resource to create its own Graphics3D resource to make textures. It uses the plugin's Graphics3D as a share_context. https://codereview.chromium.org/270213004/diff/80001/ppapi/proxy/video_decode... ppapi/proxy/video_decoder_resource.cc:276: if (!mailboxes.empty()) On 2014/05/08 04:26:04, piman wrote: > We should check somewhere that mailbox.size() == num_textures Will do in future CL (mailboxes removed for now) https://codereview.chromium.org/270213004/diff/80001/ppapi/proxy/video_decode... ppapi/proxy/video_decoder_resource.cc:362: uint32_t shm_id) { On 2014/05/08 04:26:04, piman wrote: > It makes me a bit uncomfortable to have the shm_id round-trip through the > renderer. I don't think it's needed since you have the decode_id (so you could > keep a queue of pending decodes, where you'd stash the shm id). > > A compromised renderer could cause the shm_id to be out-of-sync with the > decode_id, or be out-of-bounds. It could try to use this to gain elevated > privilege to the plugin process, which may have different access (think Flash, > that connects to renderers from other origins too). Maybe far-fetched, but I > think it's better not to make assumptions about trusting the renderer. I must pass the shm_id round trip to identify busy and available buffers. I bounds check the shm_id in the host and plugin. https://codereview.chromium.org/270213004/diff/80001/ppapi/proxy/video_decode... ppapi/proxy/video_decoder_resource.cc:367: decode_ids_[shm_id] = decode_id; On 2014/05/08 04:26:04, piman wrote: > Is this ok, to use the shm_id (which is just an index in a small range) to tag > the decode calls? > > 2 things I'm worried about: > 1- are we ensured OnPluginMsgDecodeComplete will always be called before > OnPluginMsgPictureReady? > 2- since this shm buffer will be reused at the next Decode, are we ensured we'll > get *all* OnPluginMsgPictureReady for this Decode, before the > OnPluginMsgDecodeComplete for the next one? > > > Wouldn't it be safer to use decode_id all along the path, since they're > monotonically increasing and there's no chance of reuse? I think it'd even make > some of the logic simpler. The decode_id is provided by the plugin, which may pass any value if it doesn't care about identifying pictures with decode calls. I've changed the implementation so that the 'decode_id' is passed through instead of the shm_id, for all the reasons you give above. https://codereview.chromium.org/270213004/diff/80001/ppapi/proxy/video_decode... File ppapi/proxy/video_decoder_resource.h (right): https://codereview.chromium.org/270213004/diff/80001/ppapi/proxy/video_decode... ppapi/proxy/video_decoder_resource.h:138: typedef std::map<uint32_t, Texture> TextureMap; On 2014/05/08 04:26:04, piman wrote: > nit: hash_map Done. https://codereview.chromium.org/270213004/diff/80001/ppapi/proxy/video_decode... ppapi/proxy/video_decoder_resource.h:142: typedef std::map<uint32_t, uint32_t> DecodeIdMap; On 2014/05/08 04:26:04, piman wrote: > nit: hash_map Done.
I addressed a bunch of comment from the original round of reviews, but there are a few more. I will address those soon. Hold off on re-reviewing if you're already published comments on the first patchset.
Sorry for late reply, Bill. https://codereview.chromium.org/270213004/diff/190001/content/renderer/pepper... File content/renderer/pepper/pepper_video_decoder_host.cc (right): https://codereview.chromium.org/270213004/diff/190001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.cc:119: return PP_ERROR_FAILED; Maybe we could consider PP_ERROR_BADRESOURCE? https://codereview.chromium.org/270213004/diff/190001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.cc:127: static_cast<media::VideoCodecProfile>(profile); If we add new values to PP_VideoProfile but forget to update the compiler asserts at the top of the file, it may cause problem. Maybe we could consider a conversion function (and intentionally omit 'default' for 'switch') instead. https://codereview.chromium.org/270213004/diff/190001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.cc:165: #if defined(OS_WIN) I think it is recommended to use #XXX around complete statements. https://codereview.chromium.org/270213004/diff/190001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.cc:188: if (!decoder_) { I think as long as |initialized_| is true, |decoder_| is never NULL. Maybe we could simply use DCHECK(decoder_). (here and elsewhere) https://codereview.chromium.org/270213004/diff/190001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.cc:216: pending_decodes_.insert(std::make_pair( It seems this code allows multiple in-flight Decode requests. The IDL says "The plugin should maintain the buffer and not call Decode() again until the decoder signals completion by returning PP_OK or by running |callback|." https://codereview.chromium.org/270213004/diff/190001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.cc:244: flush_reply_context_ = context->MakeReplyMessageContext(); If there is a pending Flush, this will rewrite the existing context. Besides, the IDL says "The plugin should make no further calls to the decoder other than GetPicture() and RecyclePicture() until the decoder signals completion by running the callback". Shall we directly fail calls to the decoder other than GetPicture() and RecyclePicture() while Flush is pending? (I understand that the proxy side has relevant checks. But we might receive malicious messages. If we don't do such checks at the renderer side, we need to be sure that |decoder_| won't be confused by unexpected Flush calls.) https://codereview.chromium.org/270213004/diff/190001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.cc:257: reset_reply_context_ = context->MakeReplyMessageContext(); If there is a pending Reset, this will rewrite the existing context. Besides, the IDL says "The plugin should not make any further calls to the decoder until the decoder signals completion by running |callback|". Shall we directly fail calls while Reset is pending? https://codereview.chromium.org/270213004/diff/190001/content/renderer/pepper... File content/renderer/pepper/pepper_video_decoder_host.h (right): https://codereview.chromium.org/270213004/diff/190001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.h:8: #include <list> This is not needed (and line 10) https://codereview.chromium.org/270213004/diff/190001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.h:51: uint32_t shm_id_; I think public struct members don't need the '_' suffix. https://codereview.chromium.org/270213004/diff/190001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.h:71: ppapi::HostResource graphics_context, maybe const&? https://codereview.chromium.org/270213004/diff/190001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.h:77: PP_Size size, const &? https://codereview.chromium.org/270213004/diff/190001/ppapi/api/pp_errors.idl File ppapi/api/pp_errors.idl (right): https://codereview.chromium.org/270213004/diff/190001/ppapi/api/pp_errors.idl... ppapi/api/pp_errors.idl:183: * This value indicates that plugin passed bad input data to the browser. - Does it make sense to consider reusing existing errors? * PP_ERROR_UNREADABLE_INPUT -> PP_ERROR_BADARGUMENT * PP_ERROR_PLATFORM_FAILED -> PP_ERROR_NOTSUPPORTED or simply PP_ERROR_FAILED. (But I am not sure what "platform failure" actually means.) - If we decide to add these two, maybe we could consider changing the values, say, putting them in the range of -14 ~ -19. -100~-110 are networking errors and I think we probably want to reserve some more values starting from -111 for future networking errors. https://codereview.chromium.org/270213004/diff/190001/ppapi/examples/video_de... File ppapi/examples/video_decode/video_decode.cc (right): https://codereview.chromium.org/270213004/diff/190001/ppapi/examples/video_de... ppapi/examples/video_decode/video_decode.cc:192: decoder_(new pp::VideoDecoder(instance)), It seems this is leaked. https://codereview.chromium.org/270213004/diff/190001/ppapi/examples/video_de... ppapi/examples/video_decode/video_decode.cc:204: callback_factory_.NewCallback(&Decoder::Start)); InitializeDone? https://codereview.chromium.org/270213004/diff/190001/ppapi/examples/video_de... ppapi/examples/video_decode/video_decode.cc:210: void Decoder::InitializeDone(int32_t result) { Currently this method is not used. https://codereview.chromium.org/270213004/diff/190001/ppapi/examples/video_de... ppapi/examples/video_decode/video_decode.cc:309: first_frame_delivered_ticks_(-1), please also init last_swap_request_ticks_ https://codereview.chromium.org/270213004/diff/190001/ppapi/proxy/video_decod... File ppapi/proxy/video_decoder_resource.cc (right): https://codereview.chromium.org/270213004/diff/190001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.cc:134: graphics3d.Release(); Is this leak intentional? https://codereview.chromium.org/270213004/diff/190001/ppapi/proxy/video_decod... File ppapi/proxy/video_decoder_resource.h (right): https://codereview.chromium.org/270213004/diff/190001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.h:72: scoped_ptr<base::SharedMemory> shm_; I think public struct members don't need '_' suffix. (here and below) https://codereview.chromium.org/270213004/diff/190001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.h:107: PP_Size size, const &?
I think I addressed everything. Big changes: 1) Create another Graphics3D resource rather than use plugin's. Textures are shared. 2) Fix 'decode_id' handling as piman pointed out. 3) Make unit tests work by adding special SetForTest() method to VideoDecoderResource, which disables use of graphics / GL. Thanks for the reviews, and please have another look. https://codereview.chromium.org/270213004/diff/80001/ppapi/examples/video_dec... File ppapi/examples/video_decode/video_decode.cc (right): https://codereview.chromium.org/270213004/diff/80001/ppapi/examples/video_dec... ppapi/examples/video_decode/video_decode.cc:171: static bool LookingAtNAL(const unsigned char* encoded, size_t pos) { On 2014/05/08 20:35:07, Tom Sepez wrote: > I'd be happier if you passed in the max length as a parameter, rather than just > assuming that |encoded| is always kDataLen bytes long. The test data is actually compiled into the example plugin (testdata.h) and in addition to the encoded video, that file defines kDataLen to be the actual length of the data. Funky, but this is just some example code. https://codereview.chromium.org/270213004/diff/80001/ppapi/examples/video_dec... ppapi/examples/video_decode/video_decode.cc:173: encoded[pos + 2] == 0 && encoded[pos + 3] == 1; On 2014/05/08 20:35:07, Tom Sepez wrote: > Theoretically, |pos + 3 < kDataLen| could overflow; but we control pos such that > this can't happen, right? pos should always be in the range [0, kDataLen + 1] and kDataLen is only 150148 currently. https://codereview.chromium.org/270213004/diff/80001/ppapi/examples/video_dec... ppapi/examples/video_decode/video_decode.cc:177: static void GetNextFrame(size_t *start_pos, size_t* end_pos) { On 2014/05/08 20:35:07, Tom Sepez wrote: > why pass start_pos by pointer? I didn't see it modified below? True. It's here because it will be needed in the following CL which adds software decoding for VP8 data, where the frame parsing is a little more complicated, and needs this. https://codereview.chromium.org/270213004/diff/80001/ppapi/examples/video_dec... ppapi/examples/video_decode/video_decode.cc:178: // H264 frames start with 0, 0, 0, 1. On 2014/05/08 20:35:07, Tom Sepez wrote: > nit: this comment might move up to LookingAtNAL(). Done. https://codereview.chromium.org/270213004/diff/80001/ppapi/examples/video_dec... ppapi/examples/video_decode/video_decode.cc:182: while (*end_pos + 3 < kDataLen && !LookingAtNAL(kData, *end_pos)) { On 2014/05/08 20:35:07, Tom Sepez wrote: > I suppose if LookingAtNAL is bullet-proof against overlflow, you can avoid all > the tests with +3's here, just let it increment until kDataLen, and avoid the if > below. Indeed. Done. https://codereview.chromium.org/270213004/diff/80001/ppapi/examples/video_dec... ppapi/examples/video_decode/video_decode.cc:265: uint32_t size = static_cast<uint32_t>(end_pos - start_pos); On 2014/05/08 20:35:07, Tom Sepez wrote: > We know this always fits? The current test data (ppapi/examples/video_decode/test_data.h) is only about 150K bytes. I expect it to always be about that size, so this should always be positive and < 150K. https://codereview.chromium.org/270213004/diff/80001/ppapi/proxy/video_decode... File ppapi/proxy/video_decoder_resource.cc (right): https://codereview.chromium.org/270213004/diff/80001/ppapi/proxy/video_decode... ppapi/proxy/video_decoder_resource.cc:47: DCHECK(addr_); On 2014/05/08 20:35:07, Tom Sepez wrote: > might want to handle error cases here. I'd suspect that being unable to map > memory could occur at any time based on system load, etc. Added a check that Map succeeded below, when we create the shm buffer. https://codereview.chromium.org/270213004/diff/80001/ppapi/proxy/video_decode... ppapi/proxy/video_decoder_resource.cc:253: gles2_impl_->ActiveTexture(GL_TEXTURE0); piman: Can I move this out of the loop, to after GenTextures? https://codereview.chromium.org/270213004/diff/80001/ppapi/proxy/video_decode... ppapi/proxy/video_decoder_resource.cc:362: uint32_t shm_id) { Just to reiterate, shm_id is the only Id I can count on here. I'll make sure it's validated (range checked) whenever I receive it on either side of the proxy. https://codereview.chromium.org/270213004/diff/80001/ppapi/proxy/video_decode... ppapi/proxy/video_decoder_resource.cc:414: RENDERER, On 2014/05/08 04:26:04, piman wrote: > Why not allocate the shm by going to the browser? It would avoid round trips > (since the renderer has to go to the browser), and not involve the renderer's > main thread which can be busy. > > It could also then be a sync IPC, which would remove the restriction that the > user can't touch their buffer until the callback is called. Which probably > forces a copy on most patterns (which is a shame, since we copy too), though > more likely users will forget and send flaky garbage to the video decoder. This was simpler, and the patch is already large. In general shm buffers are only created as decoding is starting up, and ideally in the steady state no new ones are created. If you like, I can add a TODO to improve this later. https://codereview.chromium.org/270213004/diff/190001/content/renderer/pepper... File content/renderer/pepper/pepper_video_decoder_host.cc (right): https://codereview.chromium.org/270213004/diff/190001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.cc:119: return PP_ERROR_FAILED; On 2014/05/14 18:08:23, yzshen1 (OOO until may 19) wrote: > Maybe we could consider PP_ERROR_BADRESOURCE? It's actually our own Graphics3D, created by the resource, that we use to create textures to share with the plugin. So it might be misleading. https://codereview.chromium.org/270213004/diff/190001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.cc:127: static_cast<media::VideoCodecProfile>(profile); On 2014/05/14 18:08:23, yzshen1 (OOO until may 19) wrote: > If we add new values to PP_VideoProfile but forget to update the compiler > asserts at the top of the file, it may cause problem. Maybe we could consider a > conversion function (and intentionally omit 'default' for 'switch') instead. Done. https://codereview.chromium.org/270213004/diff/190001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.cc:165: #if defined(OS_WIN) On 2014/05/14 18:08:23, yzshen1 (OOO until may 19) wrote: > I think it is recommended to use #XXX around complete statements. Done. https://codereview.chromium.org/270213004/diff/190001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.cc:188: if (!decoder_) { On 2014/05/14 18:08:23, yzshen1 (OOO until may 19) wrote: > I think as long as |initialized_| is true, |decoder_| is never NULL. Maybe we > could simply use DCHECK(decoder_). (here and elsewhere) Done. https://codereview.chromium.org/270213004/diff/190001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.cc:216: pending_decodes_.insert(std::make_pair( On 2014/05/14 18:08:23, yzshen1 (OOO until may 19) wrote: > It seems this code allows multiple in-flight Decode requests. The IDL says "The > plugin should maintain the buffer and not call Decode() again until the decoder > signals completion by returning PP_OK or by running |callback|." It's true that multiple decodes can be in-flight simultaneously. That's because we have several shm buffers that can be filled and awaiting decode. From the plugin's point of view, it can only call Decode once until completion. We signal completion after we have copied the plugin's buffer to the shm buffer. If there is a shm buffer available, we immediately copy the data and return PP_OK, otherwise, we return PP_OK_COMPLETIONPENDING and signal completion when a buffer becomes available. https://codereview.chromium.org/270213004/diff/190001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.cc:244: flush_reply_context_ = context->MakeReplyMessageContext(); On 2014/05/14 18:08:23, yzshen1 (OOO until may 19) wrote: > If there is a pending Flush, this will rewrite the existing context. > Besides, the IDL says "The plugin should make no further calls to the decoder > other than GetPicture() and RecyclePicture() until the decoder signals > completion by running the callback". Shall we directly fail calls to the decoder > other than GetPicture() and RecyclePicture() while Flush is pending? > > (I understand that the proxy side has relevant checks. But we might receive > malicious messages. If we don't do such checks at the renderer side, we need to > be sure that |decoder_| won't be confused by unexpected Flush calls.) The IDL is correct. I'll add checks here as well, return PP_ERROR_FAILED (not PP_ERROR_INPROGRESS). Done. https://codereview.chromium.org/270213004/diff/190001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.cc:257: reset_reply_context_ = context->MakeReplyMessageContext(); On 2014/05/14 18:08:23, yzshen1 (OOO until may 19) wrote: > If there is a pending Reset, this will rewrite the existing context. > > Besides, the IDL says "The plugin should not make any further calls to the > decoder until the decoder signals completion by running |callback|". Shall we > directly fail calls while Reset is pending? Done. https://codereview.chromium.org/270213004/diff/190001/content/renderer/pepper... File content/renderer/pepper/pepper_video_decoder_host.h (right): https://codereview.chromium.org/270213004/diff/190001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.h:8: #include <list> On 2014/05/14 18:08:23, yzshen1 (OOO until may 19) wrote: > This is not needed (and line 10) Done. https://codereview.chromium.org/270213004/diff/190001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.h:51: uint32_t shm_id_; On 2014/05/14 18:08:23, yzshen1 (OOO until may 19) wrote: > I think public struct members don't need the '_' suffix. I did this to avoid name collisions in ctors. But I think it's better to add '_' to the params instead. Done. https://codereview.chromium.org/270213004/diff/190001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.h:71: ppapi::HostResource graphics_context, On 2014/05/14 18:08:23, yzshen1 (OOO until may 19) wrote: > maybe const&? Done. https://codereview.chromium.org/270213004/diff/190001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.h:77: PP_Size size, On 2014/05/14 18:08:23, yzshen1 (OOO until may 19) wrote: > const &? Done. https://codereview.chromium.org/270213004/diff/190001/ppapi/api/pp_errors.idl File ppapi/api/pp_errors.idl (right): https://codereview.chromium.org/270213004/diff/190001/ppapi/api/pp_errors.idl... ppapi/api/pp_errors.idl:183: * This value indicates that plugin passed bad input data to the browser. On 2014/05/14 18:08:23, yzshen1 (OOO until may 19) wrote: > - Does it make sense to consider reusing existing errors? > * PP_ERROR_UNREADABLE_INPUT -> PP_ERROR_BADARGUMENT > * PP_ERROR_PLATFORM_FAILED -> PP_ERROR_NOTSUPPORTED or simply PP_ERROR_FAILED. > (But I am not sure what "platform failure" actually means.) > > - If we decide to add these two, maybe we could consider changing the values, > say, putting them in the range of -14 ~ -19. -100~-110 are networking errors and > I think we probably want to reserve some more values starting from -111 for > future networking errors. I thought about both of those possibilities. PP_ERROR_BADARGUMENT to me means something was NULL or out of range and doesn't express that a buffer of data was malformed. (I'm changing this name to PP_ERROR_MALFORMED_INPUT). PP_ERROR_FAILED is a little too generic to capture that there was a failure in Chrome's implementation, and doesn't express that the failure is unrecoverable. However I feel less strongly about this one. In the next CL I'll move these into the empty range you suggest. https://codereview.chromium.org/270213004/diff/190001/ppapi/examples/video_de... File ppapi/examples/video_decode/video_decode.cc (right): https://codereview.chromium.org/270213004/diff/190001/ppapi/examples/video_de... ppapi/examples/video_decode/video_decode.cc:192: decoder_(new pp::VideoDecoder(instance)), On 2014/05/14 18:08:23, yzshen1 (OOO until may 19) wrote: > It seems this is leaked. Good catch. Deleted in dtor. https://codereview.chromium.org/270213004/diff/190001/ppapi/examples/video_de... ppapi/examples/video_decode/video_decode.cc:204: callback_factory_.NewCallback(&Decoder::Start)); On 2014/05/14 18:08:23, yzshen1 (OOO until may 19) wrote: > InitializeDone? Done. https://codereview.chromium.org/270213004/diff/190001/ppapi/examples/video_de... ppapi/examples/video_decode/video_decode.cc:210: void Decoder::InitializeDone(int32_t result) { On 2014/05/14 18:08:23, yzshen1 (OOO until may 19) wrote: > Currently this method is not used. Changed callback to call this. https://codereview.chromium.org/270213004/diff/190001/ppapi/examples/video_de... ppapi/examples/video_decode/video_decode.cc:309: first_frame_delivered_ticks_(-1), On 2014/05/14 18:08:23, yzshen1 (OOO until may 19) wrote: > please also init last_swap_request_ticks_ Done. https://codereview.chromium.org/270213004/diff/190001/ppapi/proxy/video_decod... File ppapi/proxy/video_decoder_resource.cc (right): https://codereview.chromium.org/270213004/diff/190001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.cc:134: graphics3d.Release(); On 2014/05/14 18:08:23, yzshen1 (OOO until may 19) wrote: > Is this leak intentional? Won't graphics3d_ (scoped_refptr) hold onto it? https://codereview.chromium.org/270213004/diff/190001/ppapi/proxy/video_decod... File ppapi/proxy/video_decoder_resource.h (right): https://codereview.chromium.org/270213004/diff/190001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.h:72: scoped_ptr<base::SharedMemory> shm_; On 2014/05/14 18:08:23, yzshen1 (OOO until may 19) wrote: > I think public struct members don't need '_' suffix. (here and below) Done. https://codereview.chromium.org/270213004/diff/190001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.h:107: PP_Size size, On 2014/05/14 18:08:23, yzshen1 (OOO until may 19) wrote: > const &? Done.
+jar for histograms.xml
+jar for histograms.xml This time do it manually.
https://codereview.chromium.org/270213004/diff/190001/content/renderer/pepper... File content/renderer/pepper/pepper_video_decoder_host.cc (right): https://codereview.chromium.org/270213004/diff/190001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.cc:119: return PP_ERROR_FAILED; Okay. I see. On 2014/05/14 19:35:04, bbudge wrote: > On 2014/05/14 18:08:23, yzshen1 (OOO until may 19) wrote: > > Maybe we could consider PP_ERROR_BADRESOURCE? > > It's actually our own Graphics3D, created by the resource, that we use to create > textures to share with the plugin. So it might be misleading. https://codereview.chromium.org/270213004/diff/190001/content/renderer/pepper... File content/renderer/pepper/pepper_video_decoder_host.h (right): https://codereview.chromium.org/270213004/diff/190001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.h:51: uint32_t shm_id_; I think '_' suffix has pretty specific meaning in our codebase. Maybe we could use in_foo as ctor name in this case? On 2014/05/14 19:35:04, bbudge wrote: > On 2014/05/14 18:08:23, yzshen1 (OOO until may 19) wrote: > > I think public struct members don't need the '_' suffix. > > I did this to avoid name collisions in ctors. But I think it's better to add '_' > to the params instead. > Done. https://codereview.chromium.org/270213004/diff/190001/ppapi/api/pp_errors.idl File ppapi/api/pp_errors.idl (right): https://codereview.chromium.org/270213004/diff/190001/ppapi/api/pp_errors.idl... ppapi/api/pp_errors.idl:183: * This value indicates that plugin passed bad input data to the browser. On 2014/05/14 19:35:04, bbudge wrote: > On 2014/05/14 18:08:23, yzshen1 (OOO until may 19) wrote: > > - Does it make sense to consider reusing existing errors? > > * PP_ERROR_UNREADABLE_INPUT -> PP_ERROR_BADARGUMENT > > * PP_ERROR_PLATFORM_FAILED -> PP_ERROR_NOTSUPPORTED or simply > PP_ERROR_FAILED. > > (But I am not sure what "platform failure" actually means.) > > > > - If we decide to add these two, maybe we could consider changing the values, > > say, putting them in the range of -14 ~ -19. -100~-110 are networking errors > and > > I think we probably want to reserve some more values starting from -111 for > > future networking errors. > > I thought about both of those possibilities. PP_ERROR_BADARGUMENT to me means > something was NULL or out of range and doesn't express that a buffer of data was > malformed. (I'm changing this name to PP_ERROR_MALFORMED_INPUT). > > PP_ERROR_FAILED is a little too generic to capture that there was a failure in > Chrome's implementation, and doesn't express that the failure is unrecoverable. > However I feel less strongly about this one. In the next CL I'll move these into > the empty range you suggest. Okay. One thing I don't quite understand is "platform failure". Would you please tell me more about the meaning? (And maybe add that to the comment of the error code) https://codereview.chromium.org/270213004/diff/190001/ppapi/proxy/video_decod... File ppapi/proxy/video_decoder_resource.cc (right): https://codereview.chromium.org/270213004/diff/190001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.cc:134: graphics3d.Release(); On 2014/05/14 19:35:04, bbudge wrote: > On 2014/05/14 18:08:23, yzshen1 (OOO until may 19) wrote: > > Is this leak intentional? > > Won't graphics3d_ (scoped_refptr) hold onto it? - When assigning a value to scoped_refptr, it adds ref automatically. - PP_Resource ref and scoped_refptr's ref are two different kind of ref. Please see https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/shared_impl/... https://codereview.chromium.org/270213004/diff/230001/ppapi/proxy/video_decod... File ppapi/proxy/video_decoder_resource.cc (right): https://codereview.chromium.org/270213004/diff/230001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.cc:41: VideoDecoderResource::ShmBuffer::ShmBuffer(base::SharedMemory* shm_, I think '_' suffix has quite specific meaning in our codebase. Maybe we could consider 'in_shm'? https://codereview.chromium.org/270213004/diff/230001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.cc:238: PPAPI_DISPATCH_PLUGIN_RESOURCE_CALL( I think it is good to add indent in this case.
Messages LGTM.
Fixed up VideoDecoderResource management of its Graphics3D resource, using a ScopedPPResource instead of raw scoped_refptr. Added detail to comment on PP_ERROR_PLATFORM_FAILED error code. https://codereview.chromium.org/270213004/diff/230001/ppapi/proxy/video_decod... File ppapi/proxy/video_decoder_resource.cc (right): https://codereview.chromium.org/270213004/diff/230001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.cc:41: VideoDecoderResource::ShmBuffer::ShmBuffer(base::SharedMemory* shm_, On 2014/05/14 22:58:16, yzshen1 (OOO until may 19) wrote: > I think '_' suffix has quite specific meaning in our codebase. Maybe we could > consider 'in_shm'? That's a good convention. Done. https://codereview.chromium.org/270213004/diff/230001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.cc:238: PPAPI_DISPATCH_PLUGIN_RESOURCE_CALL( On 2014/05/14 22:58:16, yzshen1 (OOO until may 19) wrote: > I think it is good to add indent in this case. You're the second reviewer to point this out so I'll change it. We're going to be fighting with clang format, but I agree it does look better. Other message maps in 'content/renderer/pepper' and 'content/browser/renderer_host/pepper' are clang formatted without indentation already though. Done.
https://codereview.chromium.org/270213004/diff/250001/ppapi/api/pp_errors.idl File ppapi/api/pp_errors.idl (right): https://codereview.chromium.org/270213004/diff/250001/ppapi/api/pp_errors.idl... ppapi/api/pp_errors.idl:94: * plugin's request was valid but the browser could not complete it. Some Hmm.. I think we have a lot of cases in which plugin makes legitimate requests but the browser is not able to complete. For example, the plugin requests to make a connection to a server, but the computer is not connected to the internet; or the plugin makes a reasonable memory request but the system is reaching its resource limit. I feel that in such cases we either want to give a more specific error code, for example, PP_ERROR_ADDRESS_UNREACHABLE and PP_ERROR_NOMEMORY; or we use the most generic error code PP_ERROR_FAILED. With regard to "bugs in the browser", it seems a little weird to make an error code for it. We probably need to fix the bugs instead. Maybe we could make an error code specific to "hardware/driver limitations"? What do you think?
https://codereview.chromium.org/270213004/diff/250001/ppapi/api/pp_errors.idl File ppapi/api/pp_errors.idl (right): https://codereview.chromium.org/270213004/diff/250001/ppapi/api/pp_errors.idl... ppapi/api/pp_errors.idl:94: * plugin's request was valid but the browser could not complete it. Some On 2014/05/15 00:55:30, yzshen1 (OOO until may 19) wrote: > Hmm.. I think we have a lot of cases in which plugin makes legitimate requests > but the browser is not able to complete. For example, the plugin requests to > make a connection to a server, but the computer is not connected to the > internet; or the plugin makes a reasonable memory request but the system is > reaching its resource limit. > > I feel that in such cases we either want to give a more specific error code, for > example, PP_ERROR_ADDRESS_UNREACHABLE and PP_ERROR_NOMEMORY; or we use the most > generic error code PP_ERROR_FAILED. > > With regard to "bugs in the browser", it seems a little weird to make an error > code for it. We probably need to fix the bugs instead. > > Maybe we could make an error code specific to "hardware/driver limitations"? > > What do you think? I've changed the docs. I think the unique thing about this error is that when a resource returns it, it has become unusable. We could change the name to PP_ERROR_UNRECOVERABLE_ERROR if you like. I think for VideoDecoder, returning PP_ERROR_FAILED will be confusing since the plugin can't determine whether it made a bad call, or whether the resource has died.
https://codereview.chromium.org/270213004/diff/80001/ppapi/proxy/video_decode... File ppapi/proxy/video_decoder_resource.cc (right): https://codereview.chromium.org/270213004/diff/80001/ppapi/proxy/video_decode... ppapi/proxy/video_decoder_resource.cc:414: RENDERER, On 2014/05/14 19:35:04, bbudge wrote: > On 2014/05/08 04:26:04, piman wrote: > > Why not allocate the shm by going to the browser? It would avoid round trips > > (since the renderer has to go to the browser), and not involve the renderer's > > main thread which can be busy. > > > > It could also then be a sync IPC, which would remove the restriction that the > > user can't touch their buffer until the callback is called. Which probably > > forces a copy on most patterns (which is a shame, since we copy too), though > > more likely users will forget and send flaky garbage to the video decoder. > > This was simpler, and the patch is already large. In general shm buffers are > only created as decoding is starting up, and ideally in the steady state no new > ones are created. If you like, I can add a TODO to improve this later. I disagree... the API is too subtle. See the comment in the test. https://codereview.chromium.org/270213004/diff/270001/ppapi/proxy/video_decod... File ppapi/proxy/video_decoder_resource.cc (right): https://codereview.chromium.org/270213004/diff/270001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.cc:43: : shm(in_shm), size(in_size), addr(NULL), shm_id(in_shm_id) { nit: no need for in_ prefixes. For an initializer, shm(shm) works. https://codereview.chromium.org/270213004/diff/270001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.cc:157: return PP_ERROR_INPROGRESS; This is actually very unfortunate. It means you can't pipeline decode calls and keep the decoder busy, since you need a round-trip before being able to decode more. Since you will send at least one Decode per video frame, you can't afford a full round trip every time if the renderer or GPU process is busy. https://codereview.chromium.org/270213004/diff/270001/ppapi/proxy/video_decod... File ppapi/proxy/video_decoder_resource_unittest.cc (right): https://codereview.chromium.org/270213004/diff/270001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource_unittest.cc:152: &MockCompletionCallback::Callback, cb)); This is an illegal call. decode_buffer is on the stack (hence is about to go out of scope), but the semantic of Decode is that you have to maintain your buffer valid until the callback is called. This is why I think allocating the shm synchronously is way preferable. This is subtle, and will fail in subtle ways.
https://codereview.chromium.org/270213004/diff/270001/ppapi/proxy/video_decod... File ppapi/proxy/video_decoder_resource.cc (right): https://codereview.chromium.org/270213004/diff/270001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.cc:43: : shm(in_shm), size(in_size), addr(NULL), shm_id(in_shm_id) { On 2014/05/15 04:02:53, piman wrote: > nit: no need for in_ prefixes. For an initializer, shm(shm) works. Awesome. I didn't know that! I'll fix this in the next upload. https://codereview.chromium.org/270213004/diff/270001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.cc:157: return PP_ERROR_INPROGRESS; On 2014/05/15 04:02:53, piman wrote: > This is actually very unfortunate. It means you can't pipeline decode calls and > keep the decoder busy, since you need a round-trip before being able to decode > more. > > Since you will send at least one Decode per video frame, you can't afford a full > round trip every time if the renderer or GPU process is busy. It's not as bad as it looks. If a shared memory buffer is available, TryDecode will return with PP_OK and the plugin can immediately call Decode again. In the steady state, when there are > 8 shm buffers, the plugin will thus have 8 Decodes in flight if it is fast enough. The only exception is at start up, or if a buffer is too large to fit in the last available shm buffer, in which case we have to wait for a new buffer. There is some complexity with the current code because we have to wait for the asynchronous return of a shm buffer. Is there a way for plugin code to synchronously request one from the browser without writing a browser host for this resource? I'd like to avoid having two hosts for this class. https://codereview.chromium.org/270213004/diff/270001/ppapi/proxy/video_decod... File ppapi/proxy/video_decoder_resource_unittest.cc (right): https://codereview.chromium.org/270213004/diff/270001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource_unittest.cc:152: &MockCompletionCallback::Callback, cb)); On 2014/05/15 04:02:53, piman wrote: > This is an illegal call. > decode_buffer is on the stack (hence is about to go out of scope), but the > semantic of Decode is that you have to maintain your buffer valid until the > callback is called. > > > This is why I think allocating the shm synchronously is way preferable. This is > subtle, and will fail in subtle ways. Agreed, this is illegal and only works because we don't check the buffer contents. I will fix the test code. It is unavoidable that the plugin has to wait for completion when calling Decode. If not, we would potentially have to copy many buffers and queue them up, or alternatively allocate many shared memory buffers. This implementation puts back pressure on the plugin by allowing a maximum of 8 concurrent decodes. Although more shm buffers than that may be needed if some very large buffers are required mid stream, this keeps the number of buffers to a minimum.
https://codereview.chromium.org/270213004/diff/250001/ppapi/api/pp_errors.idl File ppapi/api/pp_errors.idl (right): https://codereview.chromium.org/270213004/diff/250001/ppapi/api/pp_errors.idl... ppapi/api/pp_errors.idl:94: * plugin's request was valid but the browser could not complete it. Some On 2014/05/15 01:19:18, bbudge wrote: > On 2014/05/15 00:55:30, yzshen1 (OOO until may 19) wrote: > > Hmm.. I think we have a lot of cases in which plugin makes legitimate requests > > but the browser is not able to complete. For example, the plugin requests to > > make a connection to a server, but the computer is not connected to the > > internet; or the plugin makes a reasonable memory request but the system is > > reaching its resource limit. > > > > I feel that in such cases we either want to give a more specific error code, > for > > example, PP_ERROR_ADDRESS_UNREACHABLE and PP_ERROR_NOMEMORY; or we use the > most > > generic error code PP_ERROR_FAILED. > > > > With regard to "bugs in the browser", it seems a little weird to make an error > > code for it. We probably need to fix the bugs instead. > > > > Maybe we could make an error code specific to "hardware/driver limitations"? > > > > What do you think? > > I've changed the docs. I think the unique thing about this error is that when a > resource returns it, it has become unusable. We could change the name to > PP_ERROR_UNRECOVERABLE_ERROR if you like. Yes, PP_ERROR_UNRECOVERABLE_ERROR sounds easier to understand to me. > > I think for VideoDecoder, returning PP_ERROR_FAILED will be confusing since the > plugin can't determine whether it made a bad call, or whether the resource has > died.
https://codereview.chromium.org/270213004/diff/190001/content/renderer/pepper... File content/renderer/pepper/pepper_video_decoder_host.cc (right): https://codereview.chromium.org/270213004/diff/190001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.cc:216: pending_decodes_.insert(std::make_pair( This confused me at first as well. It seems piman and yzshen were also expecting Decode to mean something different. In particular, the completion callback doesn't indicate that the _Decode_ is complete, it just means that the system is finished writing it to shmem. Maybe there's a less confusing way to name Decode? Like... "EnqueueDecode" or something? https://codereview.chromium.org/270213004/diff/270001/content/renderer/pepper... File content/renderer/pepper/pepper_video_decoder_host.cc (right): https://codereview.chromium.org/270213004/diff/270001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.cc:166: shm_buffers_.push_back(shm.release()); You don't seem to limit the number of buffers possible here... it seems like we ought to do that? https://codereview.chromium.org/270213004/diff/270001/ppapi/proxy/video_decod... File ppapi/proxy/video_decoder_resource.cc (right): https://codereview.chromium.org/270213004/diff/270001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.cc:40: VideoDecoderResource::ShmBuffer::ShmBuffer(base::SharedMemory* in_shm, nit: scoped_ptr<> would make it clearer that this is taking ownership. https://codereview.chromium.org/270213004/diff/270001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.cc:62: : decode_id( in_decode_id), texture_id(in_texture_id) { nit: extra space before in_decode_id https://codereview.chromium.org/270213004/diff/270001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.cc:428: available_shm_buffers_.back()->size < size) { The way I'm reading this, it seems like you may or may not have a big enough buffer inside available_shm_buffers_, but you only check the last one. And their order is essentially arbitrary, I think. You're on the edge of basically making an allocator here, and I think as written, it's going to be a wasteful one. Can we consider a different way to do this? I was going to suggest a circular buffer again, but then I saw that the media backend works in terms of shmem ids :-/. (It seems like it would be nice if maybe those could deal with shmem id + offset + size, so you could use share a shmem buffer across decode calls). In the absence of that, you might want to make the code a little smarter. E.g., use a container that's ordered by size (probably doesn't matter what kind, since it's fairly small). You can then find the smallest available buffer that's big enough and use it. (You can use lower_bound or upper_bound or jsut linear search, since it should be a small list). Only if no buffers are big enough do you request a new one. You're also not bounding how many shmem buffers you have or expiring them. As a first cut, would it make sense to set a limit on the number of buffers? You might even reasonably cap it at kMaximumPendingDecodes. Any time you have available buffers but none are large enough, take the smallest one and recycle it and request a new, bigger one. As a completely different idea, you could use 1 big circular buffer between the plugin and renderer that is just for pushing "Decode" data to the renderer. I'm guessing there's some reason the decode stuff in media can't use s circular buffer, but you could conform to their API by keeping the pool of shmem blocks in the renderer. That would just save you all this cross-process dancing with the multiple shmem buffers. But it would cost you a copy in the renderer, to go from the circular buffer in to the shmem block to pass to media decode API. So perhaps it's not worth it. https://codereview.chromium.org/270213004/diff/270001/ppapi/proxy/video_decod... File ppapi/proxy/video_decoder_resource_unittest.cc (right): https://codereview.chromium.org/270213004/diff/270001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource_unittest.cc:152: &MockCompletionCallback::Callback, cb)); It is a bit confusing, though... it's not an out-param, but still has to outlive the scope of the call-site. I was curious; it appears that FileIO::Write has the same restriction :-(. I'm also wondering if that's a *new* restriction with the in-process file writing... I think maybe it is. I wonder if you could fix it with two methods, a sort of TryDecode that's always synchronous, and a WaitForDecodeCapacity? A blocking C++ caller might look like this: decoder.WaitForDecodeCapacity(buffer_size, pp::BlockUntilComplete()); int32_t result = decoder.TryDecode(decode_id, buffer_size, buffer); (this assumes they're not doing something weird like calling TryDecode from another thread). That might be worse, I'm not sure. It does make the lifetime requirements of the buffer easier to understand.
Addressing some of the minor issues here. https://codereview.chromium.org/270213004/diff/270001/content/renderer/pepper... File content/renderer/pepper/pepper_video_decoder_host.cc (right): https://codereview.chromium.org/270213004/diff/270001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.cc:166: shm_buffers_.push_back(shm.release()); On 2014/05/15 21:18:22, dmichael wrote: > You don't seem to limit the number of buffers possible here... it seems like we > ought to do that? Are we concerned about DoS attacks by plugins? It seems like they can already do this by creating other types of resources that allocate shmem. https://codereview.chromium.org/270213004/diff/270001/ppapi/proxy/video_decod... File ppapi/proxy/video_decoder_resource.cc (right): https://codereview.chromium.org/270213004/diff/270001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.cc:40: VideoDecoderResource::ShmBuffer::ShmBuffer(base::SharedMemory* in_shm, On 2014/05/15 21:18:22, dmichael wrote: > nit: scoped_ptr<> would make it clearer that this is taking ownership. Done. https://codereview.chromium.org/270213004/diff/270001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.cc:43: : shm(in_shm), size(in_size), addr(NULL), shm_id(in_shm_id) { On 2014/05/15 16:31:57, bbudge wrote: > On 2014/05/15 04:02:53, piman wrote: > > nit: no need for in_ prefixes. For an initializer, shm(shm) works. > > Awesome. I didn't know that! > I'll fix this in the next upload. Done. https://codereview.chromium.org/270213004/diff/270001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.cc:62: : decode_id( in_decode_id), texture_id(in_texture_id) { On 2014/05/15 21:18:22, dmichael wrote: > nit: extra space before in_decode_id Done. https://codereview.chromium.org/270213004/diff/270001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.cc:428: available_shm_buffers_.back()->size < size) { On 2014/05/15 21:18:22, dmichael wrote: > The way I'm reading this, it seems like you may or may not have a big enough > buffer inside available_shm_buffers_, but you only check the last one. And their > order is essentially arbitrary, I think. > > You're on the edge of basically making an allocator here, and I think as > written, it's going to be a wasteful one. Can we consider a different way to do > this? I was going to suggest a circular buffer again, but then I saw that the > media backend works in terms of shmem ids :-/. (It seems like it would be nice > if maybe those could deal with shmem id + offset + size, so you could use share > a shmem buffer across decode calls). > > In the absence of that, you might want to make the code a little smarter. E.g., > use a container that's ordered by size (probably doesn't matter what kind, since > it's fairly small). You can then find the smallest available buffer that's big > enough and use it. (You can use lower_bound or upper_bound or jsut linear > search, since it should be a small list). Only if no buffers are big enough do > you request a new one. > > You're also not bounding how many shmem buffers you have or expiring them. As a > first cut, would it make sense to set a limit on the number of buffers? You > might even reasonably cap it at kMaximumPendingDecodes. Any time you have > available buffers but none are large enough, take the smallest one and recycle > it and request a new, bigger one. > > As a completely different idea, you could use 1 big circular buffer between the > plugin and renderer that is just for pushing "Decode" data to the renderer. I'm > guessing there's some reason the decode stuff in media can't use s circular > buffer, but you could conform to their API by keeping the pool of shmem blocks > in the renderer. That would just save you all this cross-process dancing with > the multiple shmem buffers. But it would cost you a copy in the renderer, to go > from the circular buffer in to the shmem block to pass to media decode API. So > perhaps it's not worth it. I've been worried about this too. This technique is actually cribbed from media code. I think it works because it makes sure the buffers are pretty big (100K bytes). I was hoping Ami can provide some guidance here when he does his review. Here's an example of this: https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/gpu_... I've considered being a little smarter about sizing and bounding the number of the buffers, but trying to keep it simple until I know I need more. https://codereview.chromium.org/270213004/diff/270001/ppapi/proxy/video_decod... File ppapi/proxy/video_decoder_resource_unittest.cc (right): https://codereview.chromium.org/270213004/diff/270001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource_unittest.cc:152: &MockCompletionCallback::Callback, cb)); On 2014/05/15 21:18:22, dmichael wrote: > It is a bit confusing, though... it's not an out-param, but still has to > outlive the scope of the call-site. I was curious; it appears that FileIO::Write > has the same restriction :-(. I'm also wondering if that's a *new* restriction > with the in-process file writing... I think maybe it is. > > I wonder if you could fix it with two methods, a sort of TryDecode that's always > synchronous, and a WaitForDecodeCapacity? > > A blocking C++ caller might look like this: > > decoder.WaitForDecodeCapacity(buffer_size, pp::BlockUntilComplete()); > int32_t result = decoder.TryDecode(decode_id, buffer_size, buffer); > (this assumes they're not doing something weird like calling TryDecode from > another thread). > > That might be worse, I'm not sure. It does make the lifetime requirements of > the buffer easier to understand. I'd like to keep the API simple. I think FileIO Read and Write establish this precedent (that buffer must be preserved until completion). Do you think it's too confusing that a method may complete both synchronously and asynchronously?
On Thu, May 15, 2014 at 3:27 PM, <bbudge@chromium.org> wrote: > This technique is actually cribbed from media code. I think it works > because it makes sure the buffers are pretty big (100K bytes). I was > hoping Ami can provide some guidance here when he does his review. > Probably both of these make sense to do (also in the media code): - drop buffers that are found to be too small (effectively caps the # of SHM buffers to the # of outstanding decodes, ish) - cap the total amount of memory allocated to these buffers (e.g. even 4k content shouldn't exceed 4MB per buffer at reasonable compression levels) In practice neither has proven to be a problem so far in media. (note that while a plugin can exhaust SHM, the worry here would be that a reasonable plugin shouldn't exhaust SHM as the result of malicious content) To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
tsepez: I added a parameter to the GetShmBuffer message) dmichael: Shared memory buffers are now bounded (size and quantity). PTAL
On 2014/05/16 18:16:05, bbudge wrote: > tsepez: I added a parameter to the GetShmBuffer message) Who generates the shm_id? What if they're lying about it?
On 2014/05/16 19:05:59, Tom Sepez wrote: > On 2014/05/16 18:16:05, bbudge wrote: > > tsepez: I added a parameter to the GetShmBuffer message) > Who generates the shm_id? What if they're lying about it? An excellent question. It's from untrusted code. I range check it, but it's possible that it's in use by the gpu and I don't want to delete it if it's in use. I'll add a check for that.
tsepez: I sanity check shm_id's as they are passed between plugin and renderer. piman: The plugin no longer has to maintain its buffer when calling Decode. dmichael: The number of shm buffers is now bounded. When a buffer won't fit, and we're at the limit of # of buffers, we reallocate a free buffer. PTAL
https://chromiumcodereview.appspot.com/270213004/diff/450001/content/renderer... File content/renderer/pepper/pepper_video_decoder_host.cc (right): https://chromiumcodereview.appspot.com/270213004/diff/450001/content/renderer... content/renderer/pepper/pepper_video_decoder_host.cc:59: // No default case, to catch unhandled PP_VideoProfile values. I think the previous indent was right. Did clang-format do this? That might be considered a bug... You can probably force it to the right indent by adding a carriage return before. https://chromiumcodereview.appspot.com/270213004/diff/450001/content/renderer... content/renderer/pepper/pepper_video_decoder_host.cc:65: // These constants should be kept in sync with VideoDecoderResource. Your header currently includes video_decoder_resource.h with a comment about constants. Maybe a header in ppapi/proxy called media_codec_constants.h would make sense? (ppapi/shared_impl would also be OK) https://chromiumcodereview.appspot.com/270213004/diff/450001/content/renderer... content/renderer/pepper/pepper_video_decoder_host.cc:96: ignore_result(decoder_.release()); Yuck. Seems like the destructor for that class ought to be protected or private (not your problem). Is it worth using scoped_ptr in this case? I'm not sure. It does make it more self-documenting in a way, but it would be a bug for scoped_ptr to actually delete it via the destructor. You could make scoped_ptr use Destroy() by using a custom deleter. It would look something like: struct VideoDecodeAcceleratorDeleter { void operator()(VideoDecodeAccelerator* decoder) { decoder->Destroy(); } }; The header would have: scoped_ptr<VideoDecodeAccelerator, VideoDecodeAcceleratorDeleter> decoder_; } Arguably the Deleter could live with VideoDecodeAccelerator, kind of like this: https://code.google.com/p/chromium/codesearch#chromium/src/media/base/android... ...but it's up to you and Ami if you want to go there. https://chromiumcodereview.appspot.com/270213004/diff/450001/content/renderer... content/renderer/pepper/pepper_video_decoder_host.cc:177: if (shm_buffer_busy_[pending_shm_id]) Could you instead just say: if (pending_decodes_.count(pending_shm_id)) and get rid of shm_buffer_busy_? ...or maybe more idiomatic in Chromium: if (ContainsKey(pending_decodes_, pending_shm_id)) (https://code.google.com/p/chromium/codesearch#chromium/src/base/stl_util.h&l=195) https://chromiumcodereview.appspot.com/270213004/diff/450001/content/renderer... content/renderer/pepper/pepper_video_decoder_host.cc:181: // doesn't do the right thing. nit: maybe better to say "won't delete the existing element if we just assign it" or something for clarity. https://chromiumcodereview.appspot.com/270213004/diff/450001/content/renderer... content/renderer/pepper/pepper_video_decoder_host.cc:251: if (shm_buffer_busy_[shm_id]) same as above, I think you could just look in the PendingDecode map? https://chromiumcodereview.appspot.com/270213004/diff/450001/content/renderer... content/renderer/pepper/pepper_video_decoder_host.cc:375: uint32_t uid = static_cast<uint32_t>(bitstream_buffer_id); If the decode API forces you to use int32, you might just want to bite the bullet and use int32 also. I think the conversion from unsigned to signed on line 263 is implementation defined if the value is too large to fit in int32. Probably does the right thing, but it's hard to say with optimizers these days. OTOH, at 60Hz, I think it will take over 2 years to reach that case, so maybe who cares. https://chromiumcodereview.appspot.com/270213004/diff/450001/content/renderer... File content/renderer/pepper/pepper_video_decoder_host.h (right): https://chromiumcodereview.appspot.com/270213004/diff/450001/content/renderer... content/renderer/pepper/pepper_video_decoder_host.h:24: #include "ppapi/proxy/video_decoder_resource.h" Should this go in the body? https://chromiumcodereview.appspot.com/270213004/diff/450001/content/renderer... content/renderer/pepper/pepper_video_decoder_host.h:101: std::vector<uint8_t> shm_buffer_busy_; Why not std::vector<bool>? Better yet, can't you just check to see if the id exists in pending_decodes_? https://chromiumcodereview.appspot.com/270213004/diff/450001/ppapi/proxy/vide... File ppapi/proxy/video_decoder_resource.cc (right): https://chromiumcodereview.appspot.com/270213004/diff/450001/ppapi/proxy/vide... ppapi/proxy/video_decoder_resource.cc:48: if (shm->Map(size)) Given this is what size means, do you need the size field at all? SharedMemory knows the size, I think https://chromiumcodereview.appspot.com/270213004/diff/450001/ppapi/proxy/vide... ppapi/proxy/video_decoder_resource.cc:167: // decode_id for the maximim picture delay. maximim->maximum https://chromiumcodereview.appspot.com/270213004/diff/450001/ppapi/proxy/vide... ppapi/proxy/video_decoder_resource.cc:168: decode_ids_[next_decode_id_] = decode_id; So, decode_id is from the plugin and can be anything, and next_decode_id_ is... something else for internal bookkeeping. I don't quite understand next_decode_id_ yet. Would it make more sense to just use a deque to store the plugin's decode_id? https://chromiumcodereview.appspot.com/270213004/diff/450001/ppapi/proxy/vide... ppapi/proxy/video_decoder_resource.cc:369: uint32_t shm_id = pending_shm_id_; So you only allow one request for a shared memory region at a time? When we were discussing over IM I was assuming you could have more than one in flight. This is likely to add to latency at the beginning of decoding. Would it be hard to pipeline the calls and allow more pending requests for shmem? https://chromiumcodereview.appspot.com/270213004/diff/450001/ppapi/proxy/vide... ppapi/proxy/video_decoder_resource.cc:402: SendDecodeMessage(shm_id); So this adds to latency at the start of decode also. Maybe it's not important. But a different way to do this that would cut down latency would be to have two Decode messages, one for shmem, and one that sends over the frame as a vector of bytes. When the plugin calls decode: If you are already up to the max number of decodes, then return PP_ERROR_INPROGRESS (as you do currently). Else If you have a big enough shmem buffer: Use the free shmem buffer and send the Decode message that's like the one you have now. Else Send a Decode message that contains the bytes as a buffer. This message also implies that the renderer will allocate a shared memory buffer in order to pass it to the media layer. When that Decode is done, the buffer can be recycled back to the plugin as usual. So I think it would replace the GetShm message. If you think this is too complicated, maybe ignore for now. But it will probably reduce latency and might even simplify some things, since you wouldn't have to store the last Decode buffer in local memory. It would also mean you wouldn't have to throttle the plugin until you reached your max number of decodes. WDYT? https://chromiumcodereview.appspot.com/270213004/diff/450001/ppapi/proxy/vide... ppapi/proxy/video_decoder_resource.cc:418: // to call Decode again. I think it would be clearer with this comment inside the: if (!decode_buffer_) ...conditional instead https://chromiumcodereview.appspot.com/270213004/diff/450001/ppapi/proxy/vide... ppapi/proxy/video_decoder_resource.cc:558: pp_picture->decode_id = decode_ids_[decode_id]; How do you know that the picture.decode_id will map to the right index in to decode_ids_? I must have missed something. I don't see where your index in to the array goes that the Picture can know about it. https://chromiumcodereview.appspot.com/270213004/diff/450001/ppapi/proxy/vide... File ppapi/proxy/video_decoder_resource.h (right): https://chromiumcodereview.appspot.com/270213004/diff/450001/ppapi/proxy/vide... ppapi/proxy/video_decoder_resource.h:76: uint32_t size; is this different that SharedMemory::actual_size()? If so, maybe you could comment what it means (size of the decode buffer, while the shared memory region might be larger?) https://chromiumcodereview.appspot.com/270213004/diff/450001/ppapi/proxy/vide... ppapi/proxy/video_decoder_resource.h:166: static const int kMaximumPictureLatency = 128; nit: I'm used to Latency meaning time. Maybe there's a better name, like... kMaximumPictureBacklog? And do you actually need this? I would have assumed that the delay would be effectively bounded by... const uint32_t kMaximumPendingDecodes = 8; https://chromiumcodereview.appspot.com/270213004/diff/450001/ppapi/proxy/vide... ppapi/proxy/video_decoder_resource.h:172: scoped_ptr<char[]> temp_buffer_; scoped_ptr<uint8_t[]>?
piman: are you going to have another look? tsepez: yet another change to messages. We combined Decode / GetShm into s single message to simplify the code and reduce latency in the rare case where we must create/resize a shm buffer. https://codereview.chromium.org/270213004/diff/450001/content/renderer/pepper... File content/renderer/pepper/pepper_video_decoder_host.cc (right): https://codereview.chromium.org/270213004/diff/450001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.cc:59: // No default case, to catch unhandled PP_VideoProfile values. On 2014/05/20 22:40:38, dmichael wrote: > I think the previous indent was right. Did clang-format do this? That might be > considered a bug... > > You can probably force it to the right indent by adding a carriage return > before. I think I "fixed" this when I fixed the indent in OnResourceMessageReceived. Fixed it back. Done. https://codereview.chromium.org/270213004/diff/450001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.cc:65: // These constants should be kept in sync with VideoDecoderResource. On 2014/05/20 22:40:38, dmichael wrote: > Your header currently includes video_decoder_resource.h with a comment about > constants. Maybe a header in ppapi/proxy called media_codec_constants.h would > make sense? (ppapi/shared_impl would also be OK) Added a .h file, ppapi/proxy/video_decoder_constants.h. https://codereview.chromium.org/270213004/diff/450001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.cc:96: ignore_result(decoder_.release()); On 2014/05/20 22:40:38, dmichael wrote: > Yuck. Seems like the destructor for that class ought to be protected or private > (not your problem). > > Is it worth using scoped_ptr in this case? I'm not sure. It does make it more > self-documenting in a way, but it would be a bug for scoped_ptr to actually > delete it via the destructor. > > You could make scoped_ptr use Destroy() by using a custom deleter. It would look > something like: > struct VideoDecodeAcceleratorDeleter { > void operator()(VideoDecodeAccelerator* decoder) { > decoder->Destroy(); > } > }; > The header would have: > scoped_ptr<VideoDecodeAccelerator, VideoDecodeAcceleratorDeleter> decoder_; > } > > Arguably the Deleter could live with VideoDecodeAccelerator, kind of like this: > https://code.google.com/p/chromium/codesearch#chromium/src/media/base/android... > ...but it's up to you and Ami if you want to go there. I looked for existing usages of Destroy. Seems like the idiom is a bit nicer: if (decoder_) decoder_.release()->Destroy(); https://code.google.com/p/chromium/codesearch#chromium/src/media/video/video_... Is that better? https://codereview.chromium.org/270213004/diff/450001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.cc:177: if (shm_buffer_busy_[pending_shm_id]) On 2014/05/20 22:40:38, dmichael wrote: > Could you instead just say: > if (pending_decodes_.count(pending_shm_id)) > and get rid of shm_buffer_busy_? > > ...or maybe more idiomatic in Chromium: > if (ContainsKey(pending_decodes_, pending_shm_id)) > > (https://code.google.com/p/chromium/codesearch#chromium/src/base/stl_util.h&l=195) It requires iterating through the map. The key here is decode number, not shm_id. https://codereview.chromium.org/270213004/diff/450001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.cc:181: // doesn't do the right thing. On 2014/05/20 22:40:38, dmichael wrote: > nit: maybe better to say "won't delete the existing element if we just assign > it" or something for clarity. Done. https://codereview.chromium.org/270213004/diff/450001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.cc:251: if (shm_buffer_busy_[shm_id]) On 2014/05/20 22:40:38, dmichael wrote: > same as above, I think you could just look in the PendingDecode map? See comments above. https://codereview.chromium.org/270213004/diff/450001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.cc:375: uint32_t uid = static_cast<uint32_t>(bitstream_buffer_id); On 2014/05/20 22:40:38, dmichael wrote: > If the decode API forces you to use int32, you might just want to bite the > bullet and use int32 also. I think the conversion from unsigned to signed on > line 263 is implementation defined if the value is too large to fit in int32. > Probably does the right thing, but it's hard to say with optimizers these days. > > OTOH, at 60Hz, I think it will take over 2 years to reach that case, so maybe > who cares. Changed uid type to int32. https://codereview.chromium.org/270213004/diff/450001/content/renderer/pepper... File content/renderer/pepper/pepper_video_decoder_host.h (right): https://codereview.chromium.org/270213004/diff/450001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.h:24: #include "ppapi/proxy/video_decoder_resource.h" On 2014/05/20 22:40:38, dmichael wrote: > Should this go in the body? Removed this. https://codereview.chromium.org/270213004/diff/450001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.h:101: std::vector<uint8_t> shm_buffer_busy_; On 2014/05/20 22:40:38, dmichael wrote: > Why not std::vector<bool>? Better yet, can't you just check to see if the id > exists in pending_decodes_? I was avoiding vector<bool> because it is slightly hazardous: http://isocpp.org/blog/2012/11/on-vectorbool http://stackoverflow.com/questions/670308/alternative-to-vectorbool In this case I think it would be OK though. The keys in pending_decodes_ are decode numbers, not shm_ids, so I would have to iterate through the map values. I was trying to avoid that, even though the map is small now. https://codereview.chromium.org/270213004/diff/450001/ppapi/proxy/video_decod... File ppapi/proxy/video_decoder_resource.cc (right): https://codereview.chromium.org/270213004/diff/450001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.cc:48: if (shm->Map(size)) On 2014/05/20 22:40:38, dmichael wrote: > Given this is what size means, do you need the size field at all? SharedMemory > knows the size, I think Done. https://codereview.chromium.org/270213004/diff/450001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.cc:167: // decode_id for the maximim picture delay. On 2014/05/20 22:40:38, dmichael wrote: > maximim->maximum Done. https://codereview.chromium.org/270213004/diff/450001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.cc:168: decode_ids_[next_decode_id_] = decode_id; On 2014/05/20 22:40:38, dmichael wrote: > So, decode_id is from the plugin and can be anything, and next_decode_id_ is... > something else for internal bookkeeping. I don't quite understand > next_decode_id_ yet. Would it make more sense to just use a deque to store the > plugin's decode_id? The resource and host both maintain a count of decode calls to use as a decode uid. I've changed those member names and added comments to clarify that. The API paints us into a corner by allowing the plugin the flexibility to pass any value for its ids. The actual host implementation requires a uid. Therefore this mapping scheme. What allows it to work without an unbounded amount of storage for remembering the plugin's values is the kMaximumPictureDelay value. The ring buffer seems like the right thing here. Not every decode_id value we store there is returned to the plugin since not every decode call results in a picture. https://codereview.chromium.org/270213004/diff/450001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.cc:369: uint32_t shm_id = pending_shm_id_; On 2014/05/20 22:40:38, dmichael wrote: > So you only allow one request for a shared memory region at a time? When we were > discussing over IM I was assuming you could have more than one in flight. This > is likely to add to latency at the beginning of decoding. Would it be hard to > pipeline the calls and allow more pending requests for shmem? It does add latency, but as discussed offline, it can make error recovery impossible if a Decode can fail after other Decodes have entered the pipeline. https://codereview.chromium.org/270213004/diff/450001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.cc:402: SendDecodeMessage(shm_id); On 2014/05/20 22:40:38, dmichael wrote: > So this adds to latency at the start of decode also. Maybe it's not important. > > But a different way to do this that would cut down latency would be to have two > Decode messages, one for shmem, and one that sends over the frame as a vector of > bytes. > > When the plugin calls decode: > If you are already up to the max number of decodes, then return > PP_ERROR_INPROGRESS (as you do currently). > Else If you have a big enough shmem buffer: > Use the free shmem buffer and send the Decode message that's like the one > you have now. > Else > Send a Decode message that contains the bytes as a buffer. This message also > implies that the renderer will allocate a shared memory buffer in order to pass > it to the media layer. When that Decode is done, the buffer can be recycled back > to the plugin as usual. So I think it would replace the GetShm message. > > If you think this is too complicated, maybe ignore for now. But it will probably > reduce latency and might even simplify some things, since you wouldn't have to > store the last Decode buffer in local memory. It would also mean you wouldn't > have to throttle the plugin until you reached your max number of decodes. > > WDYT? Not always. In the steady state, Decode calls TryDecode, and if there is a free buffer that's big enough, does the copy and returns synchronously. I took your suggestion to add a new DecodeBuffer message that copies the buffer and reduces the latency of creating / resizing a shm buffer. It worked out well, simplifying the code and making the example plugin noticeably smoother at startup, with both streams closely in sync. Marking as Done. https://codereview.chromium.org/270213004/diff/450001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.cc:418: // to call Decode again. On 2014/05/20 22:40:38, dmichael wrote: > I think it would be clearer with this comment inside the: > if (!decode_buffer_) > ...conditional instead Done. https://codereview.chromium.org/270213004/diff/450001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.cc:558: pp_picture->decode_id = decode_ids_[decode_id]; On 2014/05/20 22:40:38, dmichael wrote: > How do you know that the picture.decode_id will map to the right index in to > decode_ids_? I must have missed something. I don't see where your index in to > the array goes that the Picture can know about it. The resource and host both count decodes to give each a uid. Those should match given IPC ordering guarantees. I documented this where I declare the |num_decodes_| fields. https://codereview.chromium.org/270213004/diff/450001/ppapi/proxy/video_decod... File ppapi/proxy/video_decoder_resource.h (right): https://codereview.chromium.org/270213004/diff/450001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.h:76: uint32_t size; On 2014/05/20 22:40:38, dmichael wrote: > is this different that SharedMemory::actual_size()? If so, maybe you could > comment what it means (size of the decode buffer, while the shared memory region > might be larger?) This is just shm->mapped_size(). Removed the extra field. https://codereview.chromium.org/270213004/diff/450001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.h:166: static const int kMaximumPictureLatency = 128; On 2014/05/20 22:40:38, dmichael wrote: > nit: I'm used to Latency meaning time. Maybe there's a better name, like... > kMaximumPictureBacklog? > > And do you actually need this? I would have assumed that the delay would be > effectively bounded by... > const uint32_t kMaximumPendingDecodes = 8; Renamed to kMaximumPictureDelay. Unfortunately for us, this is a property of the stream, not of number of simultaneous decodes. The software decoder (FFmpeg) only supports 16 buffers of delay. This constant matches what is done elsewhere in Chrome, picking a large value because it's not expensive and should work, even with pathological cases. https://codereview.chromium.org/270213004/diff/450001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.h:172: scoped_ptr<char[]> temp_buffer_; On 2014/05/20 22:40:38, dmichael wrote: > scoped_ptr<uint8_t[]>? Done.
Just in case you are waiting for my response: The CL looks good to me when the other reviewers are happy with it. Thanks! On Thu, May 22, 2014 at 11:34 AM, <bbudge@chromium.org> wrote: > piman: are you going to have another look? > tsepez: yet another change to messages. We combined Decode / GetShm into s > single message to simplify the code and reduce latency in the rare case > where we > must create/resize a shm buffer. > > > https://codereview.chromium.org/270213004/diff/450001/ > content/renderer/pepper/pepper_video_decoder_host.cc > File content/renderer/pepper/pepper_video_decoder_host.cc (right): > > https://codereview.chromium.org/270213004/diff/450001/ > content/renderer/pepper/pepper_video_decoder_host.cc#newcode59 > > content/renderer/pepper/pepper_video_decoder_host.cc:59: // No default > case, to catch unhandled PP_VideoProfile values. > On 2014/05/20 22:40:38, dmichael wrote: > >> I think the previous indent was right. Did clang-format do this? That >> > might be > >> considered a bug... >> > > You can probably force it to the right indent by adding a carriage >> > return > >> before. >> > > I think I "fixed" this when I fixed the indent in > OnResourceMessageReceived. Fixed it back. Done. > > https://codereview.chromium.org/270213004/diff/450001/ > content/renderer/pepper/pepper_video_decoder_host.cc#newcode65 > > content/renderer/pepper/pepper_video_decoder_host.cc:65: // These > constants should be kept in sync with VideoDecoderResource. > On 2014/05/20 22:40:38, dmichael wrote: > >> Your header currently includes video_decoder_resource.h with a comment >> > about > >> constants. Maybe a header in ppapi/proxy called >> > media_codec_constants.h would > >> make sense? (ppapi/shared_impl would also be OK) >> > > Added a .h file, ppapi/proxy/video_decoder_constants.h. > > https://codereview.chromium.org/270213004/diff/450001/ > content/renderer/pepper/pepper_video_decoder_host.cc#newcode96 > content/renderer/pepper/pepper_video_decoder_host.cc:96: > ignore_result(decoder_.release()); > > On 2014/05/20 22:40:38, dmichael wrote: > >> Yuck. Seems like the destructor for that class ought to be protected >> > or private > >> (not your problem). >> > > Is it worth using scoped_ptr in this case? I'm not sure. It does make >> > it more > >> self-documenting in a way, but it would be a bug for scoped_ptr to >> > actually > >> delete it via the destructor. >> > > You could make scoped_ptr use Destroy() by using a custom deleter. It >> > would look > >> something like: >> struct VideoDecodeAcceleratorDeleter { >> void operator()(VideoDecodeAccelerator* decoder) { >> decoder->Destroy(); >> } >> }; >> The header would have: >> scoped_ptr<VideoDecodeAccelerator, VideoDecodeAcceleratorDeleter> >> > decoder_; > >> } >> > > Arguably the Deleter could live with VideoDecodeAccelerator, kind of >> > like this: > > https://code.google.com/p/chromium/codesearch#chromium/ > src/media/base/android/media_decoder_job.h&l=27 > >> ...but it's up to you and Ami if you want to go there. >> > > I looked for existing usages of Destroy. Seems like the idiom is a bit > nicer: > > if (decoder_) > decoder_.release()->Destroy(); > > https://code.google.com/p/chromium/codesearch#chromium/ > src/media/video/video_decode_accelerator.h&q=videodecodeaccelerator&sq= > package:chromium&l=21 > > Is that better? > > https://codereview.chromium.org/270213004/diff/450001/ > content/renderer/pepper/pepper_video_decoder_host.cc#newcode177 > > content/renderer/pepper/pepper_video_decoder_host.cc:177: if > (shm_buffer_busy_[pending_shm_id]) > On 2014/05/20 22:40:38, dmichael wrote: > >> Could you instead just say: >> if (pending_decodes_.count(pending_shm_id)) >> and get rid of shm_buffer_busy_? >> > > ...or maybe more idiomatic in Chromium: >> if (ContainsKey(pending_decodes_, pending_shm_id)) >> > > > (https://code.google.com/p/chromium/codesearch#chromium/ > src/base/stl_util.h&l=195) > > It requires iterating through the map. The key here is decode number, > not shm_id. > > https://codereview.chromium.org/270213004/diff/450001/ > content/renderer/pepper/pepper_video_decoder_host.cc#newcode181 > > content/renderer/pepper/pepper_video_decoder_host.cc:181: // doesn't do > the right thing. > On 2014/05/20 22:40:38, dmichael wrote: > >> nit: maybe better to say "won't delete the existing element if we just >> > assign > >> it" or something for clarity. >> > > Done. > > https://codereview.chromium.org/270213004/diff/450001/ > content/renderer/pepper/pepper_video_decoder_host.cc#newcode251 > > content/renderer/pepper/pepper_video_decoder_host.cc:251: if > (shm_buffer_busy_[shm_id]) > On 2014/05/20 22:40:38, dmichael wrote: > >> same as above, I think you could just look in the PendingDecode map? >> > > See comments above. > > https://codereview.chromium.org/270213004/diff/450001/ > content/renderer/pepper/pepper_video_decoder_host.cc#newcode375 > > content/renderer/pepper/pepper_video_decoder_host.cc:375: uint32_t uid = > static_cast<uint32_t>(bitstream_buffer_id); > On 2014/05/20 22:40:38, dmichael wrote: > >> If the decode API forces you to use int32, you might just want to bite >> > the > >> bullet and use int32 also. I think the conversion from unsigned to >> > signed on > >> line 263 is implementation defined if the value is too large to fit in >> > int32. > >> Probably does the right thing, but it's hard to say with optimizers >> > these days. > > OTOH, at 60Hz, I think it will take over 2 years to reach that case, >> > so maybe > >> who cares. >> > > Changed uid type to int32. > > https://codereview.chromium.org/270213004/diff/450001/ > content/renderer/pepper/pepper_video_decoder_host.h > File content/renderer/pepper/pepper_video_decoder_host.h (right): > > https://codereview.chromium.org/270213004/diff/450001/ > content/renderer/pepper/pepper_video_decoder_host.h#newcode24 > content/renderer/pepper/pepper_video_decoder_host.h:24: #include > "ppapi/proxy/video_decoder_resource.h" > > On 2014/05/20 22:40:38, dmichael wrote: > >> Should this go in the body? >> > > Removed this. > > https://codereview.chromium.org/270213004/diff/450001/ > content/renderer/pepper/pepper_video_decoder_host.h#newcode101 > > content/renderer/pepper/pepper_video_decoder_host.h:101: > std::vector<uint8_t> shm_buffer_busy_; > On 2014/05/20 22:40:38, dmichael wrote: > >> Why not std::vector<bool>? Better yet, can't you just check to see if >> > the id > >> exists in pending_decodes_? >> > > I was avoiding vector<bool> because it is slightly hazardous: > http://isocpp.org/blog/2012/11/on-vectorbool > http://stackoverflow.com/questions/670308/alternative-to-vectorbool > > In this case I think it would be OK though. > > The keys in pending_decodes_ are decode numbers, not shm_ids, so I would > have to iterate through the map values. I was trying to avoid that, even > though the map is small now. > > https://codereview.chromium.org/270213004/diff/450001/ > ppapi/proxy/video_decoder_resource.cc > File ppapi/proxy/video_decoder_resource.cc (right): > > https://codereview.chromium.org/270213004/diff/450001/ > ppapi/proxy/video_decoder_resource.cc#newcode48 > > ppapi/proxy/video_decoder_resource.cc:48: if (shm->Map(size)) > On 2014/05/20 22:40:38, dmichael wrote: > >> Given this is what size means, do you need the size field at all? >> > SharedMemory > >> knows the size, I think >> > > Done. > > https://codereview.chromium.org/270213004/diff/450001/ > ppapi/proxy/video_decoder_resource.cc#newcode167 > > ppapi/proxy/video_decoder_resource.cc:167: // decode_id for the maximim > picture delay. > On 2014/05/20 22:40:38, dmichael wrote: > >> maximim->maximum >> > > Done. > > https://codereview.chromium.org/270213004/diff/450001/ > ppapi/proxy/video_decoder_resource.cc#newcode168 > > ppapi/proxy/video_decoder_resource.cc:168: decode_ids_[next_decode_id_] > = decode_id; > On 2014/05/20 22:40:38, dmichael wrote: > >> So, decode_id is from the plugin and can be anything, and >> > next_decode_id_ is... > >> something else for internal bookkeeping. I don't quite understand >> next_decode_id_ yet. Would it make more sense to just use a deque to >> > store the > >> plugin's decode_id? >> > > The resource and host both maintain a count of decode calls to use as a > decode uid. I've changed those member names and added comments to > clarify that. > > The API paints us into a corner by allowing the plugin the flexibility > to pass any value for its ids. The actual host implementation requires a > uid. Therefore this mapping scheme. What allows it to work without an > unbounded amount of storage for remembering the plugin's values is the > kMaximumPictureDelay value. > > The ring buffer seems like the right thing here. Not every decode_id > value we store there is returned to the plugin since not every decode > call results in a picture. > > https://codereview.chromium.org/270213004/diff/450001/ > ppapi/proxy/video_decoder_resource.cc#newcode369 > > ppapi/proxy/video_decoder_resource.cc:369: uint32_t shm_id = > pending_shm_id_; > On 2014/05/20 22:40:38, dmichael wrote: > >> So you only allow one request for a shared memory region at a time? >> > When we were > >> discussing over IM I was assuming you could have more than one in >> > flight. This > >> is likely to add to latency at the beginning of decoding. Would it be >> > hard to > >> pipeline the calls and allow more pending requests for shmem? >> > > It does add latency, but as discussed offline, it can make error > recovery impossible if a Decode can fail after other Decodes have > entered the pipeline. > > https://codereview.chromium.org/270213004/diff/450001/ > ppapi/proxy/video_decoder_resource.cc#newcode402 > ppapi/proxy/video_decoder_resource.cc:402: SendDecodeMessage(shm_id); > > On 2014/05/20 22:40:38, dmichael wrote: > >> So this adds to latency at the start of decode also. Maybe it's not >> > important. > > But a different way to do this that would cut down latency would be to >> > have two > >> Decode messages, one for shmem, and one that sends over the frame as a >> > vector of > >> bytes. >> > > When the plugin calls decode: >> If you are already up to the max number of decodes, then return >> PP_ERROR_INPROGRESS (as you do currently). >> Else If you have a big enough shmem buffer: >> Use the free shmem buffer and send the Decode message that's like >> > the one > >> you have now. >> Else >> Send a Decode message that contains the bytes as a buffer. This >> > message also > >> implies that the renderer will allocate a shared memory buffer in >> > order to pass > >> it to the media layer. When that Decode is done, the buffer can be >> > recycled back > >> to the plugin as usual. So I think it would replace the GetShm >> > message. > > If you think this is too complicated, maybe ignore for now. But it >> > will probably > >> reduce latency and might even simplify some things, since you wouldn't >> > have to > >> store the last Decode buffer in local memory. It would also mean you >> > wouldn't > >> have to throttle the plugin until you reached your max number of >> > decodes. > > WDYT? >> > > Not always. In the steady state, Decode calls TryDecode, and if there is > a free buffer that's big enough, does the copy and returns > synchronously. > > I took your suggestion to add a new DecodeBuffer message that copies the > buffer and reduces the latency of creating / resizing a shm buffer. > It worked out well, simplifying the code and making the example plugin > noticeably smoother at startup, with both streams closely in sync. > > Marking as Done. > > https://codereview.chromium.org/270213004/diff/450001/ > ppapi/proxy/video_decoder_resource.cc#newcode418 > > ppapi/proxy/video_decoder_resource.cc:418: // to call Decode again. > On 2014/05/20 22:40:38, dmichael wrote: > >> I think it would be clearer with this comment inside the: >> if (!decode_buffer_) >> ...conditional instead >> > > Done. > > https://codereview.chromium.org/270213004/diff/450001/ > ppapi/proxy/video_decoder_resource.cc#newcode558 > > ppapi/proxy/video_decoder_resource.cc:558: pp_picture->decode_id = > decode_ids_[decode_id]; > On 2014/05/20 22:40:38, dmichael wrote: > >> How do you know that the picture.decode_id will map to the right index >> > in to > >> decode_ids_? I must have missed something. I don't see where your >> > index in to > >> the array goes that the Picture can know about it. >> > > The resource and host both count decodes to give each a uid. Those > should match given IPC ordering guarantees. I documented this where I > declare the |num_decodes_| fields. > > https://codereview.chromium.org/270213004/diff/450001/ > ppapi/proxy/video_decoder_resource.h > File ppapi/proxy/video_decoder_resource.h (right): > > https://codereview.chromium.org/270213004/diff/450001/ > ppapi/proxy/video_decoder_resource.h#newcode76 > ppapi/proxy/video_decoder_resource.h:76: uint32_t size; > > On 2014/05/20 22:40:38, dmichael wrote: > >> is this different that SharedMemory::actual_size()? If so, maybe you >> > could > >> comment what it means (size of the decode buffer, while the shared >> > memory region > >> might be larger?) >> > > This is just shm->mapped_size(). > Removed the extra field. > > https://codereview.chromium.org/270213004/diff/450001/ > ppapi/proxy/video_decoder_resource.h#newcode166 > > ppapi/proxy/video_decoder_resource.h:166: static const int > kMaximumPictureLatency = 128; > On 2014/05/20 22:40:38, dmichael wrote: > >> nit: I'm used to Latency meaning time. Maybe there's a better name, >> > like... > >> kMaximumPictureBacklog? >> > > And do you actually need this? I would have assumed that the delay >> > would be > >> effectively bounded by... >> const uint32_t kMaximumPendingDecodes = 8; >> > > Renamed to kMaximumPictureDelay. Unfortunately for us, this is a > property of the stream, not of number of simultaneous decodes. The > software decoder (FFmpeg) only supports 16 buffers of delay. This > constant matches what is done elsewhere in Chrome, picking a large value > because it's not expensive and should work, even with pathological > cases. > > https://codereview.chromium.org/270213004/diff/450001/ > ppapi/proxy/video_decoder_resource.h#newcode172 > > ppapi/proxy/video_decoder_resource.h:172: scoped_ptr<char[]> > temp_buffer_; > On 2014/05/20 22:40:38, dmichael wrote: > >> scoped_ptr<uint8_t[]>? >> > > Done. > > https://codereview.chromium.org/270213004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/270213004/diff/510001/content/renderer/pepper... File content/renderer/pepper/pepper_video_decoder_host.cc (right): https://codereview.chromium.org/270213004/diff/510001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.cc:138: if (channel) { either DCHECK or if, but not both. I think the DCHECK is right. https://codereview.chromium.org/270213004/diff/510001/ppapi/proxy/video_decod... File ppapi/proxy/video_decoder_resource.cc (right): https://codereview.chromium.org/270213004/diff/510001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.cc:190: std::vector<uint8_t> copy(buffer_bytes, buffer_bytes + size); I don't like the idea of having 2 separate paths that are taken arbitrarily based on scheduling. It's hard to test, and bugs are hard to track down. Also it means the memory errors are reported as an asynchronous failure to Decode, which means that at least the first N decodes have to be asynchronous, which makes it hard to ensure the decoder is kept fed. I don't think it's needed. Why do we need this? I thought we agreed on (re-)allocating shm, synchronously. If you allow a pool of N buffers, for the first N-1 buffers, you'd allocate the shm, copy the data in it, and return success. For buffer N, you'd allocate the shm and return COMPLETIONPENDING. When the callback comes back, you have one free buffer, and you allow the next Decode. When the next Decode comes, you reuse the free buffer if it's large enough, otherwise you delete it and reallocate it. If we're going to add complexity, I'd rather we spend it on making the shm allocation go to the browser IO thread so that it's fast (it's probably not needed as a first pass, but could be a follow-up). https://codereview.chromium.org/270213004/diff/510001/ppapi/proxy/video_decod... File ppapi/proxy/video_decoder_resource_unittest.cc (right): https://codereview.chromium.org/270213004/diff/510001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource_unittest.cc:61: const PPB_Graphics3D_1_0* graphics3d_iface; nit: order is methods, then fields (fields should be together). Since this is a class, the fields should have a trailing _ (it's confusing at the callsite otherwise, since they look like local variables instead).
https://codereview.chromium.org/270213004/diff/510001/content/renderer/pepper... File content/renderer/pepper/pepper_video_decoder_host.cc (right): https://codereview.chromium.org/270213004/diff/510001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.cc:138: if (channel) { On 2014/05/22 20:43:19, piman wrote: > either DCHECK or if, but not both. > > I think the DCHECK is right. Done. https://codereview.chromium.org/270213004/diff/510001/ppapi/proxy/video_decod... File ppapi/proxy/video_decoder_resource.cc (right): https://codereview.chromium.org/270213004/diff/510001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.cc:190: std::vector<uint8_t> copy(buffer_bytes, buffer_bytes + size); On 2014/05/22 20:43:19, piman wrote: > I don't like the idea of having 2 separate paths that are taken arbitrarily > based on scheduling. It's hard to test, and bugs are hard to track down. Also it > means the memory errors are reported as an asynchronous failure to Decode, which > means that at least the first N decodes have to be asynchronous, which makes it > hard to ensure the decoder is kept fed. > > I don't think it's needed. Why do we need this? I thought we agreed on > (re-)allocating shm, synchronously. > > If you allow a pool of N buffers, for the first N-1 buffers, you'd allocate the > shm, copy the data in it, and return success. For buffer N, you'd allocate the > shm and return COMPLETIONPENDING. > > When the callback comes back, you have one free buffer, and you allow the next > Decode. When the next Decode comes, you reuse the free buffer if it's large > enough, otherwise you delete it and reallocate it. > > > If we're going to add complexity, I'd rather we spend it on making the shm > allocation go to the browser IO thread so that it's fast (it's probably not > needed as a first pass, but could be a follow-up). I made the shm creation synchronous. https://codereview.chromium.org/270213004/diff/510001/ppapi/proxy/video_decod... File ppapi/proxy/video_decoder_resource_unittest.cc (right): https://codereview.chromium.org/270213004/diff/510001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource_unittest.cc:61: const PPB_Graphics3D_1_0* graphics3d_iface; On 2014/05/22 20:43:19, piman wrote: > nit: order is methods, then fields (fields should be together). > > Since this is a class, the fields should have a trailing _ (it's confusing at > the callsite otherwise, since they look like local variables instead). Done.
https://codereview.chromium.org/270213004/diff/550001/content/renderer/pepper... File content/renderer/pepper/pepper_video_decoder_host.cc (right): https://codereview.chromium.org/270213004/diff/550001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.cc:158: return PP_ERROR_FAILED; Would it be useful to add some granularity, say, a few 10s or 100s of kb, to avoid having to reallocate too much at startup if the stream is roughly consistent in bitrate, which may result in small variations in size? https://codereview.chromium.org/270213004/diff/550001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.cc:237: texture_ids[i], // Use the texture_id to identify the buffer. Would anything bad happen if texture_ids are not all distinct? https://codereview.chromium.org/270213004/diff/550001/ppapi/proxy/video_decod... File ppapi/proxy/video_decoder_resource.cc (right): https://codereview.chromium.org/270213004/diff/550001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.cc:149: nit: it would be useful here, for documentation purpose, to have a DCHECK(!available_shm_buffers_.empty() || shm_buffers_.size() < kMaximumPendingDecodes); That condition is true at the beginning, and a post-condition of the previous Decode if we didn't store a callback. https://codereview.chromium.org/270213004/diff/550001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.cc:191: if (!shm_buffer->addr) This test is too late, there's a DCHECK in the ShmBuffer constructor. Well, the condition could happen for several reasons (out of address space, out of file descriptors, ...), so maybe we should just remove the DCHECK. https://codereview.chromium.org/270213004/diff/550001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.cc:289: decode_callback_->PostAbort(); What happens to the shm buffers at this point? https://codereview.chromium.org/270213004/diff/550001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.cc:406: get_picture_callback_ = NULL; What about the other callbacks? https://codereview.chromium.org/270213004/diff/550001/ppapi/proxy/video_decod... File ppapi/proxy/video_decoder_resource.h (right): https://codereview.chromium.org/270213004/diff/550001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.h:156: // number is less than 16. We make it much larger just to be safe. I think this has more to do with how pipelining works inside of Chrome and the decoder than the bitstream itself - what prevents the decoder from copying many buffers (especially if they're small) into its internal pool, before sending back the first picture? Should we throttle Decodes to make sure this doesn't happen? Or do we have such a guarantee at the hardware decoder level?
https://codereview.chromium.org/270213004/diff/550001/content/renderer/pepper... File content/renderer/pepper/pepper_video_decoder_host.cc (right): https://codereview.chromium.org/270213004/diff/550001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.cc:158: return PP_ERROR_FAILED; On 2014/05/23 20:07:27, piman wrote: > Would it be useful to add some granularity, say, a few 10s or 100s of kb, to > avoid having to reallocate too much at startup if the stream is roughly > consistent in bitrate, which may result in small variations in size? The std::max above does that for smaller buffers. Are you thinking of the case where shm_size is > kMinimumBitstreamBufferSize? I thought about doing that, but I don't know enough about decoders and bitstream characteristics to know if this would help. FWIW, this allocation scheme is copied from GPUVideoDecoder: https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/gpu_... https://codereview.chromium.org/270213004/diff/550001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.cc:237: texture_ids[i], // Use the texture_id to identify the buffer. On 2014/05/23 20:07:27, piman wrote: > Would anything bad happen if texture_ids are not all distinct? The ownership of textures would be confused, so that the decoder might overwrite a texture the plugin hadn't rendered yet. We also would double delete the texture (in GL) which I hope is benign. https://codereview.chromium.org/270213004/diff/550001/ppapi/proxy/video_decod... File ppapi/proxy/video_decoder_resource.cc (right): https://codereview.chromium.org/270213004/diff/550001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.cc:149: On 2014/05/23 20:07:27, piman wrote: > nit: it would be useful here, for documentation purpose, to have a > DCHECK(!available_shm_buffers_.empty() || shm_buffers_.size() < > kMaximumPendingDecodes); > > That condition is true at the beginning, and a post-condition of the previous > Decode if we didn't store a callback. Done. https://codereview.chromium.org/270213004/diff/550001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.cc:191: if (!shm_buffer->addr) On 2014/05/23 20:07:27, piman wrote: > This test is too late, there's a DCHECK in the ShmBuffer constructor. > > Well, the condition could happen for several reasons (out of address space, out > of file descriptors, ...), so maybe we should just remove the DCHECK. Removed the DCHECK in the ShmBuffer ctor. https://codereview.chromium.org/270213004/diff/550001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.cc:289: decode_callback_->PostAbort(); On 2014/05/23 20:07:27, piman wrote: > What happens to the shm buffers at this point? We hold on to them. Reset is used to implement seek, so they're likely to be reusable. The video decoder in the host should notify us (OnPluginMsgDecodeDone) that bitstream buffers are no longer busy as it resets, so we can return them to the available list before completing the Reset operation. https://codereview.chromium.org/270213004/diff/550001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.cc:406: get_picture_callback_ = NULL; On 2014/05/23 20:07:27, piman wrote: > What about the other callbacks? I should add checks for them too. Done. https://codereview.chromium.org/270213004/diff/550001/ppapi/proxy/video_decod... File ppapi/proxy/video_decoder_resource.h (right): https://codereview.chromium.org/270213004/diff/550001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.h:156: // number is less than 16. We make it much larger just to be safe. On 2014/05/23 20:07:27, piman wrote: > I think this has more to do with how pipelining works inside of Chrome and the > decoder than the bitstream itself - what prevents the decoder from copying many > buffers (especially if they're small) into its internal pool, before sending > back the first picture? > > Should we throttle Decodes to make sure this doesn't happen? Or do we have such > a guarantee at the hardware decoder level? I chatted with Ami. Chrome uses this number to deal with essentially the same problem: https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/gpu_... I can try to improve the comment if you'd like.
https://codereview.chromium.org/270213004/diff/550001/content/renderer/pepper... File content/renderer/pepper/pepper_video_decoder_host.cc (right): https://codereview.chromium.org/270213004/diff/550001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.cc:237: texture_ids[i], // Use the texture_id to identify the buffer. On 2014/05/23 20:57:05, bbudge wrote: > On 2014/05/23 20:07:27, piman wrote: > > Would anything bad happen if texture_ids are not all distinct? > > The ownership of textures would be confused, so that the decoder might overwrite > a texture the plugin hadn't rendered yet. We also would double delete the > texture (in GL) which I hope is benign. It's not exactly benign, if another texture got reallocated in the mean time, it's likely to reuse ids, so you'd delete that one. Kinda like double-closing FDs. But that doesn't bother me, if the plugin breaks it, it gets to keep both pieces. What I wanted to make sure is that nothing would break in the trusted side wrt PictureBiffer ids being not unique (since they come directly from the untrusted side). https://codereview.chromium.org/270213004/diff/550001/ppapi/proxy/video_decod... File ppapi/proxy/video_decoder_resource.cc (right): https://codereview.chromium.org/270213004/diff/550001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.cc:289: decode_callback_->PostAbort(); On 2014/05/23 20:57:05, bbudge wrote: > On 2014/05/23 20:07:27, piman wrote: > > What happens to the shm buffers at this point? > > We hold on to them. Reset is used to implement seek, so they're likely to be > reusable. > > The video decoder in the host should notify us (OnPluginMsgDecodeDone) that > bitstream buffers are no longer busy as it resets, so we can return them to the > available list before completing the Reset operation. But, so, if we have a decode callback, that means all the buffers were full. When does the plugin know we have a free buffer again, so Decode can be called? Is that guaranteed by the time the reset callback comes? Can we add DCHECKs about that?
https://codereview.chromium.org/270213004/diff/550001/content/renderer/pepper... File content/renderer/pepper/pepper_video_decoder_host.cc (right): https://codereview.chromium.org/270213004/diff/550001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.cc:237: texture_ids[i], // Use the texture_id to identify the buffer. On 2014/05/23 21:19:25, piman wrote: > On 2014/05/23 20:57:05, bbudge wrote: > > On 2014/05/23 20:07:27, piman wrote: > > > Would anything bad happen if texture_ids are not all distinct? > > > > The ownership of textures would be confused, so that the decoder might > overwrite > > a texture the plugin hadn't rendered yet. We also would double delete the > > texture (in GL) which I hope is benign. > > It's not exactly benign, if another texture got reallocated in the mean time, > it's likely to reuse ids, so you'd delete that one. Kinda like double-closing > FDs. > > But that doesn't bother me, if the plugin breaks it, it gets to keep both > pieces. What I wanted to make sure is that nothing would break in the trusted > side wrt PictureBiffer ids being not unique (since they come directly from the > untrusted side). That's a good point. I think in this case the 'id' field is only used by the decoder to identify the picture when it is returned to the client, i.e. it's not used by the decoder internally. We can pick any value and there's no uniqueness requirement, though that would likely wreak havoc with the pictures. https://codereview.chromium.org/270213004/diff/550001/ppapi/proxy/video_decod... File ppapi/proxy/video_decoder_resource.cc (right): https://codereview.chromium.org/270213004/diff/550001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.cc:289: decode_callback_->PostAbort(); On 2014/05/23 21:19:25, piman wrote: > On 2014/05/23 20:57:05, bbudge wrote: > > On 2014/05/23 20:07:27, piman wrote: > > > What happens to the shm buffers at this point? > > > > We hold on to them. Reset is used to implement seek, so they're likely to be > > reusable. > > > > The video decoder in the host should notify us (OnPluginMsgDecodeDone) that > > bitstream buffers are no longer busy as it resets, so we can return them to > the > > available list before completing the Reset operation. > > But, so, if we have a decode callback, that means all the buffers were full. > When does the plugin know we have a free buffer again, so Decode can be called? > Is that guaranteed by the time the reset callback comes? Can we add DCHECKs > about that? Done in OnPluginMsgResetComplete (and OnPluginMsgFlushComplete by the same reasoning).
I like the sync way, that seems simpler. I still haven't fully understood the need for the decode number stuff. I'm still confused by the need to track the ids on both sides, rather than using another number that you already have (like the plugin provided one or the shm_id). Or... why it wouldn't make sense to send the id across so that the host will obviously use the same one as the plugin (instead of relying on the counts to match). Anyway, I'll look some more, but I wanted you to know I like the simpler sync way. https://codereview.chromium.org/270213004/diff/450001/content/renderer/pepper... File content/renderer/pepper/pepper_video_decoder_host.cc (right): https://codereview.chromium.org/270213004/diff/450001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.cc:96: ignore_result(decoder_.release()); Sure... being consistent makes sense. I still think the way the Video..codeAccelerator classes are designed w.r.t. destruction is confusing, so I threw together: https://codereview.chromium.org/292183011/ ...we'll see what Ami or somebody else on media thinks.
On 2014/05/23 22:44:46, dmichael wrote: > I like the sync way, that seems simpler. I still haven't fully understood the > need for the decode number stuff. I'm still confused by the need to track the > ids on both sides, rather than using another number that you already have (like > the plugin provided one or the shm_id). Or... why it wouldn't make sense to > send the id across so that the host will obviously use the same one as the > plugin (instead of relying on the counts to match). > > Anyway, I'll look some more, but I wanted you to know I like the simpler sync > way. > > https://codereview.chromium.org/270213004/diff/450001/content/renderer/pepper... > File content/renderer/pepper/pepper_video_decoder_host.cc (right): > > https://codereview.chromium.org/270213004/diff/450001/content/renderer/pepper... > content/renderer/pepper/pepper_video_decoder_host.cc:96: > ignore_result(decoder_.release()); > Sure... being consistent makes sense. > > I still think the way the Video..codeAccelerator classes are designed w.r.t. > destruction is confusing, so I threw together: > https://codereview.chromium.org/292183011/ > ...we'll see what Ami or somebody else on media thinks. As for the decode ids, the plugin can pass any value, with no uniqueness requirement (e.g. all zeroes). shm_id's won't work either, because when we get a picture, we'd have no way to know which piece of data generated the picture. Another way to think about this is if we only had one shm buffer - it would still work, slowly, but it's clear we couldn't match buffers to pictures. Thanks for looking. I also like the new sync way.
lgtm
lgtm
Ami, I think this is ready for a look. I'm specifically most interested in the use of VideoDecodeAccelerator in the host, and assumptions I make about reset and shutdown.
Looks great. Just a bunch of minor comments below. https://codereview.chromium.org/270213004/diff/610001/content/renderer/pepper... File content/renderer/pepper/pepper_video_decoder_host.cc (right): https://codereview.chromium.org/270213004/diff/610001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.cc:347: default: any reason to default: instead of list all known (and known-unreachable) cases here, and letting hte compiler force future adders of new errors to make the consideration explicit? https://codereview.chromium.org/270213004/diff/610001/content/renderer/pepper... File content/renderer/pepper/pepper_video_decoder_host.h (right): https://codereview.chromium.org/270213004/diff/610001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.h:55: ppapi::host::HostMessageContext* context) OVERRIDE; is nice to include for each batch of OVERRIDEs a comment saying where the interface comes from (like you have at l.57 below). Here in particular I'm surprised to see an OVERRIDE in a "private:" section. The base being overridden is protected: so giving it a different access class here is strange to me. Is there a reason to do so? https://codereview.chromium.org/270213004/diff/610001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.h:86: void DoDecode(ppapi::host::HostMessageContext* context, Short method, single caller, and no doco make me wonder: why is this a separate function? https://codereview.chromium.org/270213004/diff/610001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.h:89: void RequestTextures(uint32 num_textures, ditto but more so. https://codereview.chromium.org/270213004/diff/610001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.h:99: ScopedVector<base::SharedMemory> shm_buffers_; I think it's the case that the shm_id variable that shows up elsewhere is an index into this vector; would be nice to doco if true. https://codereview.chromium.org/270213004/diff/610001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.h:101: // Tracks buffers that are in use by the decoder. Not clear what the contents of the vector are (and whether index is same as shm_id / same as shm_buffers_). If this is a parallel vector to shm_buffers_ would it make more sense to make them as single vector of pairs or a struct? Even if not that, would be nice to put them closer together visually. https://codereview.chromium.org/270213004/diff/610001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.h:104: // This number will overflow after 414 days, assuming 60Hz video. I think this sentence is saying "This code will trigger undefined behavior after 414 days" which seems like a bad time. Would you mind doing any of: 1) making it a uint32 so you could s/overflow/wrap-around/ 2) CHECKing for overflow 3) changing to int64_t 4) implement wrap-around manually by testing the value before/after the ++. https://codereview.chromium.org/270213004/diff/610001/ppapi/examples/video_de... File ppapi/examples/video_decode/video_decode.cc (right): https://codereview.chromium.org/270213004/diff/610001/ppapi/examples/video_de... ppapi/examples/video_decode/video_decode.cc:221: for (int i = frame; i > 0; i--) nit: why counting backwards?? https://codereview.chromium.org/270213004/diff/610001/ppapi/examples/video_de... ppapi/examples/video_decode/video_decode.cc:236: seek_frame_ = frame; write-only var? (Seek() seems like dead code) https://codereview.chromium.org/270213004/diff/610001/ppapi/examples/video_de... ppapi/examples/video_decode/video_decode.cc:320: RequestInputEvents(PP_INPUTEVENT_CLASS_MOUSE); Did you mean to tie mouse-clicks to seeking somehow? https://codereview.chromium.org/270213004/diff/610001/ppapi/examples/video_de... File ppapi/examples/video_decode/video_decode_dev.cc (right): https://codereview.chromium.org/270213004/diff/610001/ppapi/examples/video_de... ppapi/examples/video_decode/video_decode_dev.cc:38: #define assertNoGLError() assert(!gles2_if_->GetError(context_->pp_resource())); I assume almost all the edits in this file are clang-format. But what brought this file to clang-format's attention? (there must be some not-whitespace-only change here but I'm missing it) https://codereview.chromium.org/270213004/diff/610001/ppapi/examples/video_de... ppapi/examples/video_decode/video_decode_dev.cc:54: class VideoDecodeDemoInstance : public pp::Instance, Is there a bug tracking moving flapper away from the _Dev interface so that it can be deleted (and with it this example)? https://codereview.chromium.org/270213004/diff/610001/ppapi/proxy/ppapi_messa... File ppapi/proxy/ppapi_messages.h (right): https://codereview.chromium.org/270213004/diff/610001/ppapi/proxy/ppapi_messa... ppapi/proxy/ppapi_messages.h:1842: uint32_t /* decode_id */, decode_id is not an argument to PpapiHostMsg_VideoDecoder_Decode https://codereview.chromium.org/270213004/diff/610001/ppapi/proxy/video_decod... File ppapi/proxy/video_decoder_constants.h (right): https://codereview.chromium.org/270213004/diff/610001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_constants.h:14: static const uint32_t kMaximumPendingDecodes = 8; Instead of allocating static storage in each compilation unit that includes this file you could do enum { kMaximumPendingDecodes = 8, kMin... = 100 << 10, ... }; That would also ensure that only compile-time constants are used. https://codereview.chromium.org/270213004/diff/610001/ppapi/proxy/video_decod... File ppapi/proxy/video_decoder_resource.cc (right): https://codereview.chromium.org/270213004/diff/610001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.cc:101: // Create a new Graphics3D resource that can create texture resources to share comment belongs at l.114 instead? https://codereview.chromium.org/270213004/diff/610001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.cc:122: host_resource = ppb_graphics3d_shared->host_resource(); IDK how "enter" things work in ppapi but they _look_ like scoped objects so it seems strange to take pointers out of them and persist them outside their scope. I trust you/your reviewers know what's going on here... https://codereview.chromium.org/270213004/diff/610001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.cc:371: } else NOTREACHED? https://codereview.chromium.org/270213004/diff/610001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.cc:386: if (TrackedCallback::IsPending(callback)) { so it's ok to call have pictures become ready without a pending get_picture callback, and you just drop the frame on the floor? (might be ok, just want to make sure it's an explicit choice; most video decoders would accumulate instead). https://codereview.chromium.org/270213004/diff/610001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.cc:490: uint32_t decode_id = picture.decode_id % kMaximumPictureDelay; decode_id_id ? https://codereview.chromium.org/270213004/diff/610001/ppapi/proxy/video_decod... File ppapi/proxy/video_decoder_resource.h (right): https://codereview.chromium.org/270213004/diff/610001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.h:76: void* addr; here and below make public data members const? https://codereview.chromium.org/270213004/diff/610001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.h:161: int32_t num_decodes_; ditto don't overflow https://codereview.chromium.org/270213004/diff/610001/ppapi/proxy/video_decod... File ppapi/proxy/video_decoder_resource_unittest.cc (right): https://codereview.chromium.org/270213004/diff/610001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource_unittest.cc:59: class VideoDecoderResourceTest : public PluginProxyTest { Only skimmed this file; hopefully other reviewers more familiar with testing ppapi resources have reviewed it more carefully.
https://codereview.chromium.org/270213004/diff/610001/content/renderer/pepper... File content/renderer/pepper/pepper_video_decoder_host.h (right): https://codereview.chromium.org/270213004/diff/610001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.h:55: ppapi::host::HostMessageContext* context) OVERRIDE; On 2014/05/28 20:50:56, Ami Fischman wrote: > is nice to include for each batch of OVERRIDEs a comment saying where the > interface comes from (like you have at l.57 below). > > Here in particular I'm surprised to see an OVERRIDE in a "private:" section. > The base being overridden is protected: so giving it a different access class > here is strange to me. Is there a reason to do so? IMO, methods should always be protected at the highest level possible. It makes it clearer that this method is never called on this class directly, and doing so might be a mistake. This has come up on chromium-dev before, and while there's not exactly consensus, a lot of folks fall on this side of the debate. darin@ expressed it well: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/CwzjcWd9cYM/TcUUl...
On Wed, May 28, 2014 at 2:00 PM, <dmichael@chromium.org> wrote: > > https://codereview.chromium.org/270213004/diff/610001/ > content/renderer/pepper/pepper_video_decoder_host.h > File content/renderer/pepper/pepper_video_decoder_host.h (right): > > https://codereview.chromium.org/270213004/diff/610001/ > content/renderer/pepper/pepper_video_decoder_host.h#newcode55 > content/renderer/pepper/pepper_video_decoder_host.h:55: > ppapi::host::HostMessageContext* context) OVERRIDE; > On 2014/05/28 20:50:56, Ami Fischman wrote: > >> is nice to include for each batch of OVERRIDEs a comment saying where >> > the > >> interface comes from (like you have at l.57 below). >> > > Here in particular I'm surprised to see an OVERRIDE in a "private:" >> > section. > >> The base being overridden is protected: so giving it a different >> > access class > >> here is strange to me. Is there a reason to do so? >> > IMO, methods should always be protected at the highest level possible. > It makes it clearer that this method is never called on this class > directly, and doing so might be a mistake. This has come up on > chromium-dev before, and while there's not exactly consensus, a lot of > folks fall on this side of the debate. darin@ expressed it well: > https://groups.google.com/a/chromium.org/d/msg/chromium- > dev/CwzjcWd9cYM/TcUUlDFgiW4J I defer to you as owner (and I don't care strongly). bbudge@: please ignore my implied request to move the method to "protected:" but please do add an "// Implements <whatever>" comment above the overridden method. > https://codereview.chromium.org/270213004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/270213004/diff/610001/content/renderer/pepper... File content/renderer/pepper/pepper_video_decoder_host.cc (right): https://codereview.chromium.org/270213004/diff/610001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.cc:347: default: On 2014/05/28 20:50:56, Ami Fischman wrote: > any reason to default: instead of list all known (and known-unreachable) cases > here, and letting hte compiler force future adders of new errors to make the > consideration explicit? The enum is a little strange, but Done. https://codereview.chromium.org/270213004/diff/610001/content/renderer/pepper... File content/renderer/pepper/pepper_video_decoder_host.h (right): https://codereview.chromium.org/270213004/diff/610001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.h:55: ppapi::host::HostMessageContext* context) OVERRIDE; On 2014/05/28 20:50:56, Ami Fischman wrote: > is nice to include for each batch of OVERRIDEs a comment saying where the > interface comes from (like you have at l.57 below). > > Here in particular I'm surprised to see an OVERRIDE in a "private:" section. > The base being overridden is protected: so giving it a different access class > here is strange to me. Is there a reason to do so? Added comment. https://codereview.chromium.org/270213004/diff/610001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.h:86: void DoDecode(ppapi::host::HostMessageContext* context, On 2014/05/28 20:50:56, Ami Fischman wrote: > Short method, single caller, and no doco make me wonder: why is this a separate > function? Yeah, left over from an earlier CL. Removed. https://codereview.chromium.org/270213004/diff/610001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.h:89: void RequestTextures(uint32 num_textures, On 2014/05/28 20:50:56, Ami Fischman wrote: > ditto but more so. Done. https://codereview.chromium.org/270213004/diff/610001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.h:99: ScopedVector<base::SharedMemory> shm_buffers_; On 2014/05/28 20:50:56, Ami Fischman wrote: > I think it's the case that the shm_id variable that shows up elsewhere is an > index into this vector; would be nice to doco if true. Done. https://codereview.chromium.org/270213004/diff/610001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.h:101: // Tracks buffers that are in use by the decoder. On 2014/05/28 20:50:56, Ami Fischman wrote: > Not clear what the contents of the vector are (and whether index is same as > shm_id / same as shm_buffers_). > > If this is a parallel vector to shm_buffers_ would it make more sense to make > them as single vector of pairs or a struct? Even if not that, would be nice to > put them closer together visually. Yes, this is parallel to shm_buffers_. I'm trying to avoid having to define a type that can fit in a vector and own SharedMemory. I use uint8_t to avoid vector<bool>. How about I make these adjacent and add a comment? https://codereview.chromium.org/270213004/diff/610001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.h:104: // This number will overflow after 414 days, assuming 60Hz video. On 2014/05/28 20:50:56, Ami Fischman wrote: > I think this sentence is saying "This code will trigger undefined behavior after > 414 days" which seems like a bad time. > Would you mind doing any of: > 1) making it a uint32 so you could s/overflow/wrap-around/ > 2) CHECKing for overflow > 3) changing to int64_t > 4) implement wrap-around manually by testing the value before/after the ++. These pass through the decoder as int32_t's so that forces my hand. I pick 4, wrapping, which allows us to play indefinitely. I also added a comment that this counts in unison with the resource. Done. https://codereview.chromium.org/270213004/diff/610001/ppapi/examples/video_de... File ppapi/examples/video_decode/video_decode.cc (right): https://codereview.chromium.org/270213004/diff/610001/ppapi/examples/video_de... ppapi/examples/video_decode/video_decode.cc:221: for (int i = frame; i > 0; i--) On 2014/05/28 20:50:56, Ami Fischman wrote: > nit: why counting backwards?? Not sure what I was thinking. Done. https://codereview.chromium.org/270213004/diff/610001/ppapi/examples/video_de... ppapi/examples/video_decode/video_decode.cc:236: seek_frame_ = frame; On 2014/05/28 20:50:56, Ami Fischman wrote: > write-only var? > > (Seek() seems like dead code) It is. I was working on implementing Seek to test Reset but decided to leave it for a future CL. https://codereview.chromium.org/270213004/diff/610001/ppapi/examples/video_de... ppapi/examples/video_decode/video_decode.cc:320: RequestInputEvents(PP_INPUTEVENT_CLASS_MOUSE); On 2014/05/28 20:50:56, Ami Fischman wrote: > Did you mean to tie mouse-clicks to seeking somehow? Yes, I was trying to use the plugin area as a crude scrubber. I've had some difficulty getting this to work. The software decoder doesn't seem to like restarting in random places. Do you think this is something that could work, or should I remove it? It's going to be useful for testing Reset for the software fallback path, where Flush and Reset are more interesting. https://codereview.chromium.org/270213004/diff/610001/ppapi/examples/video_de... File ppapi/examples/video_decode/video_decode_dev.cc (right): https://codereview.chromium.org/270213004/diff/610001/ppapi/examples/video_de... ppapi/examples/video_decode/video_decode_dev.cc:38: #define assertNoGLError() assert(!gles2_if_->GetError(context_->pp_resource())); On 2014/05/28 20:50:56, Ami Fischman wrote: > I assume almost all the edits in this file are clang-format. > But what brought this file to clang-format's attention? > (there must be some not-whitespace-only change here but I'm missing it) I'm not sure what happened here. I'll remove this file from the CL. https://codereview.chromium.org/270213004/diff/610001/ppapi/examples/video_de... ppapi/examples/video_decode/video_decode_dev.cc:54: class VideoDecodeDemoInstance : public pp::Instance, On 2014/05/28 20:50:56, Ami Fischman wrote: > Is there a bug tracking moving flapper away from the _Dev interface so that it > can be deleted (and with it this example)? https://code.google.com/p/chromium/issues/detail?id=378547 https://codereview.chromium.org/270213004/diff/610001/ppapi/proxy/ppapi_messa... File ppapi/proxy/ppapi_messages.h (right): https://codereview.chromium.org/270213004/diff/610001/ppapi/proxy/ppapi_messa... ppapi/proxy/ppapi_messages.h:1842: uint32_t /* decode_id */, On 2014/05/28 20:50:56, Ami Fischman wrote: > decode_id is not an argument to PpapiHostMsg_VideoDecoder_Decode Both sides count Decode calls in unison. 'decode_id' allows the host to identify the decode which generated the picture. I added some comments around the host and resource's num_decodes_ fields. https://codereview.chromium.org/270213004/diff/610001/ppapi/proxy/video_decod... File ppapi/proxy/video_decoder_constants.h (right): https://codereview.chromium.org/270213004/diff/610001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_constants.h:14: static const uint32_t kMaximumPendingDecodes = 8; On 2014/05/28 20:50:56, Ami Fischman wrote: > Instead of allocating static storage in each compilation unit that includes this > file you could do > enum { > kMaximumPendingDecodes = 8, > kMin... = 100 << 10, > ... > }; > > That would also ensure that only compile-time constants are used. Good idea. Done. https://codereview.chromium.org/270213004/diff/610001/ppapi/proxy/video_decod... File ppapi/proxy/video_decoder_resource.cc (right): https://codereview.chromium.org/270213004/diff/610001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.cc:101: // Create a new Graphics3D resource that can create texture resources to share On 2014/05/28 20:50:56, Ami Fischman wrote: > comment belongs at l.114 instead? All of this code is just to create that Graphics3D resource. I intended the comment to describe all of this. I can move this preliminary code inside the 'if (testing_)' statement though. https://codereview.chromium.org/270213004/diff/610001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.cc:122: host_resource = ppb_graphics3d_shared->host_resource(); On 2014/05/28 20:50:56, Ami Fischman wrote: > IDK how "enter" things work in ppapi but they _look_ like scoped objects so it > seems strange to take pointers out of them and persist them outside their scope. > I trust you/your reviewers know what's going on here... The pointer will be valid as long as we hold the resource, which we do in graphics3d_. It is hard to follow. https://codereview.chromium.org/270213004/diff/610001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.cc:371: } On 2014/05/28 20:50:56, Ami Fischman wrote: > else NOTREACHED? What if I DCHECK in the else clause (and remove the 'if (testing_)' ? https://codereview.chromium.org/270213004/diff/610001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.cc:386: if (TrackedCallback::IsPending(callback)) { On 2014/05/28 20:50:56, Ami Fischman wrote: > so it's ok to call have pictures become ready without a pending get_picture > callback, and you just drop the frame on the floor? > > (might be ok, just want to make sure it's an explicit choice; most video > decoders would accumulate instead). Good catch! My intention is definitely to queue up pictures when the plugin can't keep up. The queue length is always bounded since there are a fixed number of textures available. Fixed. https://codereview.chromium.org/270213004/diff/610001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.cc:490: uint32_t decode_id = picture.decode_id % kMaximumPictureDelay; On 2014/05/28 20:50:56, Ami Fischman wrote: > decode_id_id ? I skirt the issue by eliminating the local. Done. https://codereview.chromium.org/270213004/diff/610001/ppapi/proxy/video_decod... File ppapi/proxy/video_decoder_resource.h (right): https://codereview.chromium.org/270213004/diff/610001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.h:76: void* addr; On 2014/05/28 20:50:56, Ami Fischman wrote: > here and below make public data members const? Done, where possible, here and in the host. https://codereview.chromium.org/270213004/diff/610001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.h:161: int32_t num_decodes_; On 2014/05/28 20:50:56, Ami Fischman wrote: > ditto don't overflow Done. https://codereview.chromium.org/270213004/diff/610001/ppapi/proxy/video_decod... File ppapi/proxy/video_decoder_resource_unittest.cc (right): https://codereview.chromium.org/270213004/diff/610001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource_unittest.cc:59: class VideoDecoderResourceTest : public PluginProxyTest { On 2014/05/28 20:50:56, Ami Fischman wrote: > Only skimmed this file; hopefully other reviewers more familiar with testing > ppapi resources have reviewed it more carefully. Me too. I plan to add some browser_tests when we have software fallback so we can run it on all platforms.
https://codereview.chromium.org/270213004/diff/630001/content/renderer/pepper... File content/renderer/pepper/pepper_video_decoder_host.cc (right): https://codereview.chromium.org/270213004/diff/630001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.cc:225: // Wrap to 0 if num_decodes_ overflowed. signed integer overflow is undefined behavior. You can't compare <0 hoping for wrap-around behavior. Replace l.224-227 with int32_t uid = ++num_decodes_; if (uid == std::numeric_limits<int32_t>::max) num_decodes_ = 0; https://codereview.chromium.org/270213004/diff/630001/content/renderer/pepper... File content/renderer/pepper/pepper_video_decoder_host.h (right): https://codereview.chromium.org/270213004/diff/630001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.h:98: // A vector of values indicating if a shm buffer is in use by the decoder. s/values/true\/false/ ? https://codereview.chromium.org/270213004/diff/630001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.h:102: // Number of Decode calls made, mod 2^32, to serve as a uid for each decode. s/32/31/ https://codereview.chromium.org/270213004/diff/630001/ppapi/examples/video_de... File ppapi/examples/video_decode/video_decode.cc (right): https://codereview.chromium.org/270213004/diff/630001/ppapi/examples/video_de... ppapi/examples/video_decode/video_decode.cc:320: RequestInputEvents(PP_INPUTEVENT_CLASS_MOUSE); On 2014/05/29 00:03:34, bbudge wrote: > On 2014/05/28 20:50:56, Ami Fischman wrote: > > Did you mean to tie mouse-clicks to seeking somehow? > > Yes, I was trying to use the plugin area as a crude scrubber. I've had some > difficulty getting this to work. The software decoder doesn't seem to like > restarting in random places. Do you think this is something that could work, or > should I remove it? It's going to be useful for testing Reset for the software > fallback path, where Flush and Reset are more interesting. I agree Seek() would be valuable coverage. You can't seek to arbitrary NALUs, only to seek-points in the stream (in h264, that's IDR: http://en.wikipedia.org/wiki/Network_Abstraction_Layer#Coded_Video_Sequences). You could add enough of an h264 parser to find the IDRs dynamically in the plugin, or you could dump out the key-frame locations from the testdata.h file and hard-code those in testdata-idr.h or somesuch. https://codereview.chromium.org/270213004/diff/630001/ppapi/proxy/ppapi_messa... File ppapi/proxy/ppapi_messages.h (right): https://codereview.chromium.org/270213004/diff/630001/ppapi/proxy/ppapi_messa... ppapi/proxy/ppapi_messages.h:1847: uint32_t /* decode_id */, I thought you said something forced this to be an int32_t, not unsigned, and that's why you had to manually wrap. ? https://codereview.chromium.org/270213004/diff/630001/ppapi/proxy/video_decod... File ppapi/proxy/video_decoder_resource.cc (right): https://codereview.chromium.org/270213004/diff/630001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.cc:158: // Wrap to 0 if num_decodes_ overflowed. ditto
https://codereview.chromium.org/270213004/diff/630001/content/renderer/pepper... File content/renderer/pepper/pepper_video_decoder_host.cc (right): https://codereview.chromium.org/270213004/diff/630001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.cc:225: // Wrap to 0 if num_decodes_ overflowed. On 2014/05/29 02:31:36, Ami Fischman wrote: > signed integer overflow is undefined behavior. You can't compare <0 hoping for > wrap-around behavior. Replace l.224-227 with > int32_t uid = ++num_decodes_; > if (uid == std::numeric_limits<int32_t>::max) > num_decodes_ = 0; Thanks! Done. https://codereview.chromium.org/270213004/diff/630001/content/renderer/pepper... File content/renderer/pepper/pepper_video_decoder_host.h (right): https://codereview.chromium.org/270213004/diff/630001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.h:98: // A vector of values indicating if a shm buffer is in use by the decoder. On 2014/05/29 02:31:36, Ami Fischman wrote: > s/values/true\/false/ ? Done. https://codereview.chromium.org/270213004/diff/630001/content/renderer/pepper... content/renderer/pepper/pepper_video_decoder_host.h:102: // Number of Decode calls made, mod 2^32, to serve as a uid for each decode. On 2014/05/29 02:31:36, Ami Fischman wrote: > s/32/31/ Done. https://codereview.chromium.org/270213004/diff/630001/ppapi/proxy/ppapi_messa... File ppapi/proxy/ppapi_messages.h (right): https://codereview.chromium.org/270213004/diff/630001/ppapi/proxy/ppapi_messa... ppapi/proxy/ppapi_messages.h:1847: uint32_t /* decode_id */, On 2014/05/29 02:31:36, Ami Fischman wrote: > I thought you said something forced this to be an int32_t, not unsigned, and > that's why you had to manually wrap. ? This should be int32_t to match the counters. Done. https://codereview.chromium.org/270213004/diff/630001/ppapi/proxy/video_decod... File ppapi/proxy/video_decoder_resource.cc (right): https://codereview.chromium.org/270213004/diff/630001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.cc:158: // Wrap to 0 if num_decodes_ overflowed. On 2014/05/29 02:31:36, Ami Fischman wrote: > ditto Done.
LGTM https://codereview.chromium.org/270213004/diff/670001/ppapi/proxy/video_decod... File ppapi/proxy/video_decoder_resource.h (right): https://codereview.chromium.org/270213004/diff/670001/ppapi/proxy/video_decod... ppapi/proxy/video_decoder_resource.h:160: // The resource and host count decodes in unison. Remind me again why this (requiring two different classes to count precisely the same way) is better than simply passing the resource's (not the plugin's) notion of a decode_id to the host?
On 2014/05/29 16:50:39, Ami Fischman wrote: > LGTM > > https://codereview.chromium.org/270213004/diff/670001/ppapi/proxy/video_decod... > File ppapi/proxy/video_decoder_resource.h (right): > > https://codereview.chromium.org/270213004/diff/670001/ppapi/proxy/video_decod... > ppapi/proxy/video_decoder_resource.h:160: // The resource and host count decodes > in unison. > Remind me again why this (requiring two different classes to count precisely the > same way) is better than simply passing the resource's (not the plugin's) notion > of a decode_id to the host? This CL has gone through a lot of changes. There was some concern about untrusted or compromised processes confusing each other by passing bad uid's. I don't think that's a very serious problem, so I'll make the resource generate uid's and send them to the host.
histograms.xml 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/270213004/700001
The CQ bit was unchecked by bbudge@chromium.org
Message was sent while issue was closed.
Committed patchset #23 manually as r273920 (presubmit successful).
Message was sent while issue was closed.
https://codereview.chromium.org/270213004/diff/630001/ppapi/examples/video_de... File ppapi/examples/video_decode/video_decode.cc (right): https://codereview.chromium.org/270213004/diff/630001/ppapi/examples/video_de... ppapi/examples/video_decode/video_decode.cc:320: RequestInputEvents(PP_INPUTEVENT_CLASS_MOUSE); On 2014/05/29 02:31:36, Ami Fischman wrote: > On 2014/05/29 00:03:34, bbudge wrote: > > On 2014/05/28 20:50:56, Ami Fischman wrote: > > > Did you mean to tie mouse-clicks to seeking somehow? > > > > Yes, I was trying to use the plugin area as a crude scrubber. I've had some > > difficulty getting this to work. The software decoder doesn't seem to like > > restarting in random places. Do you think this is something that could work, > or > > should I remove it? It's going to be useful for testing Reset for the software > > fallback path, where Flush and Reset are more interesting. > > I agree Seek() would be valuable coverage. > You can't seek to arbitrary NALUs, only to seek-points in the stream (in h264, > that's IDR: > http://en.wikipedia.org/wiki/Network_Abstraction_Layer#Coded_Video_Sequences). > You could add enough of an h264 parser to find the IDRs dynamically in the > plugin, or you could dump out the key-frame locations from the testdata.h file > and hard-code those in testdata-idr.h or somesuch. Looking at the test file, there is apparently only 1 keyframe. I'm not sure how many seek points that implies. I think a simpler approach which gets the same coverage is to Reset on mouse down and just start at 0 again.
On Tue, Jun 3, 2014 at 11:41 AM, <bbudge@chromium.org> wrote: > Looking at the test file, there is apparently only 1 keyframe. I'm not > sure how many seek points that implies. > One, unfortch. > I think a simpler approach which > gets the same coverage is to Reset on mouse down and just start at 0 > again. > Sure, though would be even better to throw a few extra keyframes in :) -a To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On 2014/06/03 21:15:08, Ami GONE FROM CHROMIUM wrote: > On Tue, Jun 3, 2014 at 11:41 AM, <mailto:bbudge@chromium.org> wrote:<font></font> > <font></font> > > Looking at the test file, there is apparently only 1 keyframe. I'm not<font></font> > > sure how many seek points that implies.<font></font> > ><font></font> > <font></font> > One, unfortch.<font></font> > <font></font> > <font></font> > > I think a simpler approach which<font></font> > > gets the same coverage is to Reset on mouse down and just start at 0<font></font> > > again.<font></font> > ><font></font> > <font></font> > Sure, though would be even better to throw a few extra keyframes in :)<font></font> > <font></font> > -a<font></font> > <font></font> > To unsubscribe from this group and stop receiving emails from it, send an email<font></font> > to mailto:chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
dielui94@gmail.com changed reviewers: + dielui94@gmail.com
Message was sent while issue was closed.
Message was sent while issue was closed.
Am 26.11.2014 13:13 schrieb <dielui94@gmail.com>: > > https://codereview.chromium.org/270213004/ https://codereview.chromium.org/270213004/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
thangah002@gmail.com changed reviewers: + thangah002@gmail.com
Message was sent while issue was closed.
Message was sent while issue was closed.
On 2015/11/22 07:36:46, thangah002 wrote: build |
