Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
311853005:
Implement software fallback for PPB_VideoDecoder. (Closed)
|
Created:
6 years, 6 months ago by bbudge Modified:
6 years, 6 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImplement software fallback for PPB_VideoDecoder.
This modifies the proxy to implement software fallback mode.
The main change is to the host, which now can work with
media::VideoDecoders.
media::VideoDecoder works differently from media::VideoDecodeAccelerator
so an adapter object, content::VideoDecoderShim is defined. It lives on the main thread and drives the actual decoder on the media thread via a child DecoderImpl class, which sends back frames of video. VideoDecoderShim receives those and converts frames to GL textures.
gpu::Mailboxes are used so the host can create textures that are aliased
to the plugin's textures.
The test plugin has been changed to include bitstream data for VP8 in order to
test the software decoder. The data is in ppapi/examples/video_decode/testdata.h
alongside the H264 data. The file diff is too large for this site but is structured
something like this:
const unsigned char kData[] = {
#if defined USE_VP8_TESTDATA_INSTEAD_OF_H264
... lots of VP8 data
#else // !USE_VP8_TESTDATA_INSTEAD_OF_H264
... lots of H264 data
#endif // USE_VP8_TESTDATA_INSTEAD_OF_H264
};
There is a TODO to convert the example to load a file. I'm not sure how to go
about that but am willing to do the work if someone can point the way.
BUG=281689
R=dmichael@chromium.org, fischman@chromium.org, sievers@chromium.org, tsepez@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277012
Patch Set 1 : #
Total comments: 14
Patch Set 2 : Move SoftwareDecoder to its own file, split into Proxy/Delegate classes, State enum. #
Total comments: 1
Patch Set 3 : Use WeakPtr to tie VideoDecoderProxy and Delegate. #
Total comments: 109
Patch Set 4 : fischman's and dmichael's review comments. #
Total comments: 52
Patch Set 5 : VideoDecoderShim implements media::VDA, file rename coming. #Patch Set 6 : Rename files, fix Destroy(). #
Total comments: 1
Patch Set 7 : Rebase, fix Windows compile, restore test. #
Total comments: 14
Patch Set 8 : Ami's comments. #Patch Set 9 : Initialize GL to fix Windows tests. #
Total comments: 4
Patch Set 10 : Try to make win_chromium_rel happy. #Patch Set 11 : Fix gpu gyp, try again to make Windows bot happy. #Patch Set 12 : Fix bug in VideoDecoderResource, add a ref to graphics context. #Patch Set 13 : Experiment to fix tests. #Patch Set 14 : Update to new media::VideoDecoder API. Test creates Graphics3D earlier. #Patch Set 15 : Don't fail Initialize test if Graphics3D isn't available. #Patch Set 16 : Fix compile issue, refine gpu build fix.x #Patch Set 17 : Fix include order. #
Created: 6 years, 6 months ago
(Patch set is too large to download)
Messages
Total messages: 39 (0 generated)
https://codereview.chromium.org/311853005/diff/20001/content/renderer/pepper/... File content/renderer/pepper/pepper_video_decoder_host.h (left): https://codereview.chromium.org/311853005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_decoder_host.h:92: scoped_refptr<PPB_Graphics3D_Impl> graphics3d_; This isn't necessary, since the resource keeps the graphics 3d object alive.
We probably talked about this before, but it looks like the vector of GPU mailboxes is moving from a less trusted process to a more-trusted process, so how can it know that it's OK to use them? Or is it the other way around? Thanks.
https://codereview.chromium.org/311853005/diff/20001/content/renderer/pepper/... File content/renderer/pepper/pepper_video_decoder_host.cc (right): https://codereview.chromium.org/311853005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_decoder_host.cc:483: case media::VideoDecoder::kNotEnoughData: This is not an error in H264 case when we feed individual NALU's. Together with not requiring entire frames, this fixes H264 example.
On 2014/06/03 20:26:07, Tom Sepez wrote: > We probably talked about this before, but it looks like the vector of GPU > mailboxes is moving from a less trusted process to a more-trusted process, so > how can it know that it's OK to use them? Or is it the other way around? > Thanks. It's the other way around, looks like I misread "Host" somewhere. LGTM.
https://codereview.chromium.org/311853005/diff/20001/content/renderer/pepper/... File content/renderer/pepper/pepper_video_decoder_host.cc (right): https://codereview.chromium.org/311853005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_decoder_host.cc:483: case media::VideoDecoder::kNotEnoughData: On 2014/06/03 20:26:32, igorc wrote: > This is not an error in H264 case when we feed individual NALU's. Together with > not requiring entire frames, this fixes H264 example. Do you mean that we should silently ignore this error?
https://codereview.chromium.org/311853005/diff/20001/content/renderer/pepper/... File content/renderer/pepper/pepper_video_decoder_host.cc (right): https://codereview.chromium.org/311853005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_decoder_host.cc:165: bool resetting_; suggestion: Might be clearer to use a 3-state enum. (something like normal, resetting, flushing) https://codereview.chromium.org/311853005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_decoder_host.cc:173: PendingDecodeQueue pending_decodes_; This is a bit hard to follow with the mix of operations and data members on the two threads. Without thinking too hard about it, I wonder if it would be any cleaner to split in to two classes? The outer SoftwareDecoder would live on the main thread. There could be an inner Delegate (or whatever) that handles all the work on the Media thread. SoftwareDecoder could provide SoftwareDecoder::Delegate with a WeakPtr. When Delegate needs to PostTask to SoftwareDecoder, it could just use the WeakPtr. So long as the WeakPtr is always dereferenced on the main thread, it's safe to use this way. That might allow you to do away with the "if (!host_)" stuff. I also think the destruction semantics could be simpler this way. The SoftwareDecode can just let itself be deleted, and use something like DeleteSoon to shutdown the Delegate part on the media thread. WDYT? https://codereview.chromium.org/311853005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_decoder_host.cc:604: I think this class should be in its own file. https://codereview.chromium.org/311853005/diff/20001/content/renderer/pepper/... File content/renderer/pepper/pepper_video_decoder_host.h (right): https://codereview.chromium.org/311853005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_decoder_host.h:55: }; I think in this case, I think you could just make a deleter that uses DeleteSoon and use it directly when specializing scoped_ptr. However, it maybe isn't necessary at all...
https://codereview.chromium.org/311853005/diff/20001/content/renderer/pepper/... File content/renderer/pepper/pepper_video_decoder_host.cc (right): https://codereview.chromium.org/311853005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_decoder_host.cc:165: bool resetting_; On 2014/06/03 22:27:47, dmichael wrote: > suggestion: Might be clearer to use a 3-state enum. (something like normal, > resetting, flushing) UNITIALIZED, DECODING, FLUSHING, RESETTING. Done. https://codereview.chromium.org/311853005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_decoder_host.cc:173: PendingDecodeQueue pending_decodes_; On 2014/06/03 22:27:47, dmichael wrote: > This is a bit hard to follow with the mix of operations and data members on the > two threads. Without thinking too hard about it, I wonder if it would be any > cleaner to split in to two classes? The outer SoftwareDecoder would live on the > main thread. There could be an inner Delegate (or whatever) that handles all the > work on the Media thread. > > SoftwareDecoder could provide SoftwareDecoder::Delegate with a WeakPtr. When > Delegate needs to PostTask to SoftwareDecoder, it could just use the WeakPtr. So > long as the WeakPtr is always dereferenced on the main thread, it's safe to use > this way. That might allow you to do away with the "if (!host_)" stuff. > > I also think the destruction semantics could be simpler this way. The > SoftwareDecode can just let itself be deleted, and use something like DeleteSoon > to shutdown the Delegate part on the media thread. > > WDYT? I moved SoftwareDecoder to its own files (video_decoder_proxy.*) and split the media thread stuff into a child Delegate class. This helps keep the thread separation clearer. I don't think it's necessary to make Delegate heap allocated. I can just embed it in its parent VideoDecoderProxy class. As for the 'host_' pointer, I'm trying to avoid the need for making PepperVideoDecoderHost support weak pointers. If you'd like, I can try to make the destruction process clearer with more comments. The process is: 1) the host scoped_ptr causes Destroy to be called in VDP. 2) VDP sets its host_ ptr to NULL, and calls the delegate Destroy (media thread). 3) The Delegate calls Destroy, calling all callbacks on media thread to complete, potentially posting tasks to the main thread. 4) Delegate posts the final task on main thread, to call VDP OnDestroyComplete. 5) VDP delete this. https://codereview.chromium.org/311853005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_decoder_host.cc:483: case media::VideoDecoder::kNotEnoughData: On 2014/06/03 20:59:20, bbudge wrote: > On 2014/06/03 20:26:32, igorc wrote: > > This is not an error in H264 case when we feed individual NALU's. Together > with > > not requiring entire frames, this fixes H264 example. > > Do you mean that we should silently ignore this error? Changed to not call NotifyError on kNotEnoughData. https://codereview.chromium.org/311853005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_decoder_host.cc:604: On 2014/06/03 22:27:46, dmichael wrote: > I think this class should be in its own file. Done.
https://codereview.chromium.org/311853005/diff/30001/content/renderer/pepper/... File content/renderer/pepper/pepper_video_decoder_host.cc (right): https://codereview.chromium.org/311853005/diff/30001/content/renderer/pepper/... content/renderer/pepper/pepper_video_decoder_host.cc:7: #include <GLES2/gl2extchromium.h> unnecessary.
https://codereview.chromium.org/311853005/diff/20001/content/renderer/pepper/... File content/renderer/pepper/pepper_video_decoder_host.cc (right): https://codereview.chromium.org/311853005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_decoder_host.cc:173: PendingDecodeQueue pending_decodes_; On 2014/06/04 14:10:12, bbudge wrote: > On 2014/06/03 22:27:47, dmichael wrote: > > This is a bit hard to follow with the mix of operations and data members on > the > > two threads. Without thinking too hard about it, I wonder if it would be any > > cleaner to split in to two classes? The outer SoftwareDecoder would live on > the > > main thread. There could be an inner Delegate (or whatever) that handles all > the > > work on the Media thread. > > > > SoftwareDecoder could provide SoftwareDecoder::Delegate with a WeakPtr. When > > Delegate needs to PostTask to SoftwareDecoder, it could just use the WeakPtr. > So > > long as the WeakPtr is always dereferenced on the main thread, it's safe to > use > > this way. That might allow you to do away with the "if (!host_)" stuff. > > > > I also think the destruction semantics could be simpler this way. The > > SoftwareDecode can just let itself be deleted, and use something like > DeleteSoon > > to shutdown the Delegate part on the media thread. > > > > WDYT? > > I moved SoftwareDecoder to its own files (video_decoder_proxy.*) and split the > media thread stuff into a child Delegate class. This helps keep the thread > separation clearer. > > I don't think it's necessary to make Delegate heap allocated. I can just embed > it in its parent VideoDecoderProxy class. > > As for the 'host_' pointer, I'm trying to avoid the need for making > PepperVideoDecoderHost support weak pointers. Do you mean you don't want to inherit SupportsWeakPtr? I'd agree with that. But you can just pass a WeakPtr to the Delegate when you create it, and it can always use that when posting a task back to the SoftwareDecoder on the main thread. That, or you can use base::Callback<>s instead, and the Delegate wouldn't even need to have knowledge of the class it's calling. The SoftwareDecoder would be responsible for binding them to the right methods with WeakPtrs. This has the additional advantage that all those methods can be private, and you don't need friendship. The downside with using Callback<> is that it adds more indirection, so might make things harder to follow. So I'm not hard over on that. But I suspect at least using WeakPtr would be a little cleaner than checking host_ and making SoftwareDecoder live beyond its useful life. WDYT?
https://codereview.chromium.org/311853005/diff/20001/content/renderer/pepper/... File content/renderer/pepper/pepper_video_decoder_host.cc (right): https://codereview.chromium.org/311853005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_decoder_host.cc:173: PendingDecodeQueue pending_decodes_; Delegate now references VideoDecoderProxy (formerly SoftwareDecoder) via WeakPtr. We can't use callbacks since 'replies' don't correspond 1:1 with calls (e.g. PictureReady).
https://codereview.chromium.org/311853005/diff/20001/content/renderer/pepper/... File content/renderer/pepper/pepper_video_decoder_host.cc (right): https://codereview.chromium.org/311853005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_decoder_host.cc:173: PendingDecodeQueue pending_decodes_; On 2014/06/04 20:52:46, bbudge wrote: > Delegate now references VideoDecoderProxy (formerly SoftwareDecoder) via > WeakPtr. > > We can't use callbacks since 'replies' don't correspond 1:1 with calls (e.g. > PictureReady). FYI, base::Callbacks have no restriction on the number of times they're called. You could map them to the same methods you already have defined and pass base::Callback<>s instead of a WeakPtr. Only advantage being the Delegate doesn't need to directly refer to the class. Can be better for unit testing, for example. But it would increase indirection so might make the code slightly harder to follow. I haven't looked at the current patchset yet; it's likely it's good this way. But if you think base::Callback would be better, it's probably feasible to do it that way.
+sievers for gpu/ This is needed to fix a link error for the nacl_ipc_64 target. I'm really not sure what is the right thing to do here.
+sievers for real
On 2014/06/04 22:01:06, bbudge wrote: > +sievers for real +piman It feels a bit weird that this would be nacl-specific. It probably just happens to work for the other components since all the files end up in a target together, while strictly speaking the dependencies are slightly strange (gpu_ipc really needs types from gpu_common, while gpu depends on gpu_ipc). Also, gpu_command_buffer_traits.cc depends on MailboxHolder, so why do you get away without adding mailbox_holder.h and mailbox_holder.cc also? Alternatively, moving mailbox/mailbox_holder.cc/.h to gpu/ipc seems a bit odd too. We could also add another gpu_base.gypi for these basic types. Or am I overthinking it and it just needs to compile+link? :)
On 2014/06/04 22:48:01, sievers wrote: > On 2014/06/04 22:01:06, bbudge wrote: > > +sievers for real > > +piman > > It feels a bit weird that this would be nacl-specific. It probably just happens > to work for the other components since all the files end up in a target > together, while strictly speaking the dependencies are slightly strange (gpu_ipc > really needs types from gpu_common, while gpu depends on gpu_ipc). > > Also, gpu_command_buffer_traits.cc depends on MailboxHolder, so why do you get > away without adding mailbox_holder.h and mailbox_holder.cc also? > > Alternatively, moving mailbox/mailbox_holder.cc/.h to gpu/ipc seems a bit odd > too. We could also add another gpu_base.gypi for these basic types. > > Or am I overthinking it and it just needs to compile+link? :) The NaCl Win64 helper (nacl64.exe) is strange. It needs to pull in a small amount of IPC-related stuff, because of NaClIPCAdapter, which intercepts and translates IPC on Windows.
On 2014/06/04 22:59:53, bbudge wrote: > On 2014/06/04 22:48:01, sievers wrote: > > On 2014/06/04 22:01:06, bbudge wrote: > > > +sievers for real > > > > +piman > > > > It feels a bit weird that this would be nacl-specific. It probably just > happens > > to work for the other components since all the files end up in a target > > together, while strictly speaking the dependencies are slightly strange > (gpu_ipc > > really needs types from gpu_common, while gpu depends on gpu_ipc). > > > > Also, gpu_command_buffer_traits.cc depends on MailboxHolder, so why do you get > > away without adding mailbox_holder.h and mailbox_holder.cc also? > > > > Alternatively, moving mailbox/mailbox_holder.cc/.h to gpu/ipc seems a bit odd > > too. We could also add another gpu_base.gypi for these basic types. > > > > Or am I overthinking it and it just needs to compile+link? :) > > The NaCl Win64 helper (nacl64.exe) is strange. It needs to pull in a small > amount of IPC-related > stuff, because of NaClIPCAdapter, which intercepts and translates IPC on > Windows. Well, let's not block your patch on it. lgtm.
https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... File content/renderer/pepper/pepper_video_decoder_host.cc (right): https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/pepper_video_decoder_host.cc:396: void PepperVideoDecoderHost::RequestTextures( single call-site and single-statement impl; why not inline into caller? https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... File content/renderer/pepper/video_decoder_proxy.cc (right): https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.cc:51: DCHECK(pending_decodes_.empty()); what guarantees this? I think you might be relying on decoder_->Stop() to fire pending ConvertFrame's, which would keep draining pending_decodes_. But that implies that VideoDecoder is re-entrant this way (that you can call VD::Decode from the callback that is invoked as a result of calling VD::Stop) which seems like a bad assumption. I think you need to explicitly drop pending decodes before calling VD::Stop in Destroy. https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.cc:63: base::Unretained(this))); I guess the only reason this isn't a UAF waiting to happen is that VideoDecoder::Stop() is synchronous and always happens before |this| is destroyed. Might be worth a comment. https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.cc:78: if (!decoder_busy) FWIW can drop decoder_busy if you do if (pending_decodes_.size() > 1) here https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.cc:78: if (!decoder_busy) AFAICT a client that never calls Reset() can never see this decoder_busy condition; am I missing something? https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.cc:84: PendingDecode& next_decode = pending_decodes_.front(); const& is less surprising https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.cc:98: if (frame) { You're using frame!=NULL as a proxy for status==kOk even though you have the status right here. I'd use status instead. That might make it clearer that you need to also test for frame->end_of_stream(). https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.cc:100: pending_frame->pixels.resize(frame->coded_size().width() * It's strange that you do the w*h*4 calc here _and_ in the ctor and that hte ctor is always passed a 0x0 size. https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.cc:121: if (!pending_decodes_.empty()) per comment at l.78, can this arise in the absence of Reset()? If Reset() _was_ called, shouldn't in-flight frames & buffers be dropped on the floor instead of being drained? https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.cc:126: decoder_->Reset(base::Bind(&VideoDecoderProxy::Delegate::OnResetComplete, set some state so that decodes that arrive from now until OnResetComplete aren't sent to the decoder? https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.cc:131: // Cancel all remaining decodes, and notify the host so it can free the shm is it obvious why this shouldn't have been done in Reset()? https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.cc:161: num_pending_decodes_(0), the logic around this member requires that Decode() calls correspond 1:1 with frames, but this isn't doco'd in the header. https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.cc:181: clear the map once done? https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.cc:202: NULL /* extra_data */, durr, does this really work? e.g. does the h264 test pass on linux? https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.cc:207: new media::FFmpegVideoDecoder(media_message_loop_)); Why not VpxVideoDecoder (for VP9) or one of the decrypting ones? (seems like if you're gonna hard-code this here then VideoDecoderProxy is not as generic a proxy as the name might imply; worth a class header doco) https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.cc:215: state_ = DECODING; shouldn't this transition only happen in the OK case of OnPipelineStatus()? https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.cc:238: DCHECK(texture_ids.size()); DCHECK(texture_id_map_.empty()); ? https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.cc:240: uint32_t num_textures = static_cast<GLuint>(texture_ids.size()); checked_cast to avoid silent overflow https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.cc:251: std::make_pair(texture_ids[i], local_texture_ids[i])); since you're ignoring return value you may as well texture_id_map_[texture_ids[i]] = local_texture_ids[i]; https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.cc:346: const uint32_t num_textures = 8; o rly 8? https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.cc:358: host_->NotifyEndOfBitstreamBuffer(frame->decode_id); In case of error this is wrong. https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.cc:368: case media::VideoDecoder::kDecryptError: _decrypt_ error should be unreachable, right? https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.cc:418: host_->NotifyEndOfBitstreamBuffer(frame->decode_id); Not sure this is necessary, FWIW. https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... File content/renderer/pepper/video_decoder_proxy.h (right): https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.h:39: // This class proxies calls to a media::VideoDecoder on the media thread. I think it is the case that this class is meant to be a compatibility shim between a client that wants to talk to a VDA (PepperVideoDecoderHost) doesn't have one, so this class cooks up a media::VideoDecoder for it. You might want to say that in this comment, somehow :) Although, wouldn't it simplify the host significantly if this class actually implemented VDA, so the host could hold either the GVDAH or VDP in the same scoped_ptr<media::VideoDecodeAccelerator> decoder_; member? https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.h:54: protected: why not private? https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.h:72: uint32_t decode_id; Here and below, make const-able members const? https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.h:82: std::vector<uint8_t> pixels; rgba_pixels ? (or comment on their colorspace) https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.h:87: class Delegate { optional: here and above, can fwd-declare inner classes here and fully-declare in the .cc file. https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.h:89: Delegate(const base::WeakPtr<VideoDecoderProxy>& proxy); explicit https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.h:90: ~Delegate(); private b/c Destroy? https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.h:92: void Initialize(scoped_ptr<media::VideoDecoder> decoder, decoder seems like a strange thing to pass to Initialize instead of to the ctor. It could even be new'd _in_ the ctor (and then media_message_loop is implicit as MessageLoopProxy::current()). https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.h:94: void ReceiveBuffer(uint32_t decode_id, I get that you want to distinguish between a caller's notion of "Decode" (which this is) and the Decode call you make from this class (which is what Decode() below is) but ReceiveBuffer is confusingly passive. WDYT of QueueDecode() for this and DrainDecode() for the next? https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.h:102: void ConvertFrame(uint32_t decode_id, "convert"? From what to what? doco please (here and elsewhere where not obvious from name/context) https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.h:107: base::WeakPtr<VideoDecoderProxy> proxy_; // Bound to main_message_loop_. https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.h:120: void OnDestroyComplete(); Not implemented (and thank goodness, because what would that mean??) https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.h:123: void FlushCommandBuffer(); doco should say when it is required to call this (and why). https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.h:146: typedef std::queue<PendingFrame*> PendingFrameQueue; y no queue<linked_ptr<PF> > ? https://codereview.chromium.org/311853005/diff/50001/ppapi/examples/video_dec... File ppapi/examples/video_decode/video_decode.cc (right): https://codereview.chromium.org/311853005/diff/50001/ppapi/examples/video_dec... ppapi/examples/video_decode/video_decode.cc:26: #define TEST_VP8 If you made these USE_VP8_TESTDATA_INSTEAD_OF_H264 then: - you wouldn't have to edit the _dev.cc example - you wouldn't risk someone defining both, which is invalid - you woulnd't risk colliding with far-flung defines from other places since nobody in their right mind would use such an unwieldy macro name. https://codereview.chromium.org/311853005/diff/50001/ppapi/examples/video_dec... ppapi/examples/video_decode/video_decode.cc:199: uint32_t frame_size = *reinterpret_cast<const uint32_t*>(&kData[current_pos]); This is a type-punning violation; please see base/macros.h:bit_cast's commentary for a description of the problem. You probably just want to construct the uint32 with bit-shifts and addition, instead of replicating bit_cast's memcpy strategy. https://codereview.chromium.org/311853005/diff/50001/ppapi/examples/video_dec... ppapi/examples/video_decode/video_decode.cc:205: #endif #else #error Oops! unless you take my suggestion above. https://codereview.chromium.org/311853005/diff/50001/ppapi/examples/video_dec... ppapi/examples/video_decode/video_decode.cc:221: PP_VIDEOPROFILE_H264MAIN; This should probably be part of the header file alongside its data. https://codereview.chromium.org/311853005/diff/50001/ppapi/examples/video_dec... ppapi/examples/video_decode/video_decode.cc:386: video_decoders_[i]->Reset(); I'm surprised you don't end up having to pause/cancel any other things like decode or pictureready waiting.
This is looking good overall to me, but I'll defer to Ami for the video decode stuff. Thanks! https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... File content/renderer/pepper/pepper_video_decoder_host.cc (right): https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/pepper_video_decoder_host.cc:384: NotifyError(pp_error); I don't think we should overload this method, especially with the types so similar. Maybe the new one should be NotifyPPError or something. https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... File content/renderer/pepper/video_decoder_proxy.cc (right): https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.cc:58: decoder_.reset(decoder.release()); tiny nit: I think I would write this as decoder_ = decoder.Pass(); (Even though your code is good, I try to prefer Pass whenever possible, since it's a little harder to mess up) https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.cc:141: base::Passed(&pending_frame))); Any reason you can't pass a null pointer instead of an empty frame? https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.cc:150: decoder_->Stop(); Would it be safe to instead put this in the destructor, and use DeleteSoon from VideoDecoderProxy? I'd prefer simpler destruction semantics if it would work. https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.cc:238: DCHECK(texture_ids.size()); On 2014/06/05 00:06:24, Ami leaving Chromium June 9th wrote: > DCHECK(texture_id_map_.empty()); > ? With a "!", of course :) https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.cc:297: delete this; Can we just do all this in a destructor? (aside from delete this :) ) https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... File content/renderer/pepper/video_decoder_proxy.h (right): https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.h:42: class VideoDecoderProxy { I can see why you used "Proxy", but I'm so used to that mostly referring to a proxy for something in another process in Chromium. This is basically Adapter pattern, so maybe VideoDecoderAdapter? (Or Wrapper, or Helper, or ??) WDYT? https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.h:170: }; (Note, if you are able to get rid of Destroy and just do stuff in destructors, then you can get rid of this) https://codereview.chromium.org/311853005/diff/50001/gpu/gpu_ipc.gypi File gpu/gpu_ipc.gypi (right): https://codereview.chromium.org/311853005/diff/50001/gpu/gpu_ipc.gypi#newcode24 gpu/gpu_ipc.gypi:24: 'command_buffer/common/mailbox.h', Isn't the IRT also going to need these? It seems like ideally maybe gpu::Mailbox should also live in gpu/ipc, and go right under sources with ipc/gpu_command_buffer_traits.*? https://codereview.chromium.org/311853005/diff/50001/ppapi/proxy/video_decode... File ppapi/proxy/video_decoder_resource.cc (right): https://codereview.chromium.org/311853005/diff/50001/ppapi/proxy/video_decode... ppapi/proxy/video_decoder_resource.cc:339: DCHECK(!mailboxes.size() || mailboxes.size() == num_textures); !mailboxes.size() -> mailboxes.empty() https://codereview.chromium.org/311853005/diff/50001/ppapi/tests/test_video_d... File ppapi/tests/test_video_decoder.cc (right): https://codereview.chromium.org/311853005/diff/50001/ppapi/tests/test_video_d... ppapi/tests/test_video_decoder.cc:8: #include "ppapi/c/ppb_var.h" nit: unused? https://codereview.chromium.org/311853005/diff/50001/ppapi/tests/test_video_d... ppapi/tests/test_video_decoder.cc:29: } You don't seem to use this function https://codereview.chromium.org/311853005/diff/50001/ppapi/tests/test_video_d... File ppapi/tests/test_video_decoder.h (right): https://codereview.chromium.org/311853005/diff/50001/ppapi/tests/test_video_d... ppapi/tests/test_video_decoder.h:18: // TestCase implementation. nit: everything below the constructor can be private.
https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... File content/renderer/pepper/pepper_video_decoder_host.cc (right): https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/pepper_video_decoder_host.cc:396: void PepperVideoDecoderHost::RequestTextures( On 2014/06/05 00:06:24, Ami leaving Chromium June 9th wrote: > single call-site and single-statement impl; why not inline into caller? This is also called by VideoDecoderProxy, which produces the mailboxes. https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... File content/renderer/pepper/video_decoder_proxy.cc (right): https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.cc:51: DCHECK(pending_decodes_.empty()); On 2014/06/05 00:06:24, Ami leaving Chromium June 9th wrote: > what guarantees this? > I think you might be relying on decoder_->Stop() to fire pending ConvertFrame's, > which would keep draining pending_decodes_. But that implies that VideoDecoder > is re-entrant this way (that you can call VD::Decode from the callback that is > invoked as a result of calling VD::Stop) which seems like a bad assumption. I > think you need to explicitly drop pending decodes before calling VD::Stop in > Destroy. Done. https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.cc:58: decoder_.reset(decoder.release()); On 2014/06/05 23:00:43, dmichael wrote: > tiny nit: I think I would write this as decoder_ = decoder.Pass(); > (Even though your code is good, I try to prefer Pass whenever possible, since > it's a little harder to mess up) This is no longer an argument. https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.cc:63: base::Unretained(this))); On 2014/06/05 00:06:24, Ami leaving Chromium June 9th wrote: > I guess the only reason this isn't a UAF waiting to happen is that > VideoDecoder::Stop() is synchronous and always happens before |this| is > destroyed. Might be worth a comment. Done. https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.cc:78: if (!decoder_busy) On 2014/06/05 00:06:24, Ami leaving Chromium June 9th wrote: > FWIW can drop decoder_busy if you do > if (pending_decodes_.size() > 1) > here I changed this and now call VD::GetMaxDecodeRequests() to get the max number (apparently it's 1 for FfmpegVideoDecoder.) I maintain the count of pending decodes and check if I can just call Decode before enqueueing. https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.cc:78: if (!decoder_busy) On 2014/06/05 00:06:24, Ami leaving Chromium June 9th wrote: > AFAICT a client that never calls Reset() can never see this decoder_busy > condition; am I missing something? It happens at startup and on reset. In the steady state, you're right, since we notify that the bitstream is done only when we've completed decoding. In reality, we could notify much earlier when we copy into the DecoderBuffer, but this way puts back pressure against the plugin so we don't copy their entire stream. https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.cc:84: PendingDecode& next_decode = pending_decodes_.front(); On 2014/06/05 00:06:24, Ami leaving Chromium June 9th wrote: > const& is less surprising Done. https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.cc:98: if (frame) { On 2014/06/05 00:06:24, Ami leaving Chromium June 9th wrote: > You're using frame!=NULL as a proxy for status==kOk even though you have the > status right here. I'd use status instead. That might make it clearer that you > need to also test for frame->end_of_stream(). So: if (status == kOk && frame && !frame->end_of_stream()) ? https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.cc:100: pending_frame->pixels.resize(frame->coded_size().width() * On 2014/06/05 00:06:24, Ami leaving Chromium June 9th wrote: > It's strange that you do the w*h*4 calc here _and_ in the ctor and that hte ctor > is always passed a 0x0 size. Added a ctor to create an empty frame, and defer construction of frames until I know the size. Done. https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.cc:121: if (!pending_decodes_.empty()) On 2014/06/05 00:06:24, Ami leaving Chromium June 9th wrote: > per comment at l.78, can this arise in the absence of Reset()? > If Reset() _was_ called, shouldn't in-flight frames & buffers be dropped on the > floor instead of being drained? Stop() and Reset() now clear pending frames first, so this won't happen. Added a comment where I clear. https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.cc:126: decoder_->Reset(base::Bind(&VideoDecoderProxy::Delegate::OnResetComplete, On 2014/06/05 00:06:24, Ami leaving Chromium June 9th wrote: > set some state so that decodes that arrive from now until OnResetComplete aren't > sent to the decoder? No decodes can arrive until Reset completes because of VideoDecoderProxy's state checks. Also, pending_decodes_ is cleared here, so we won't kick off a Decode in OnDecodeComplete. https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.cc:131: // Cancel all remaining decodes, and notify the host so it can free the shm On 2014/06/05 00:06:24, Ami leaving Chromium June 9th wrote: > is it obvious why this shouldn't have been done in Reset()? No, it should be done in Reset() as you point out. https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.cc:141: base::Passed(&pending_frame))); On 2014/06/05 23:00:43, dmichael wrote: > Any reason you can't pass a null pointer instead of an empty frame? The frame has the decode_id, which is used to identify the shm buffer that is now available. https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.cc:150: decoder_->Stop(); On 2014/06/05 23:00:43, dmichael wrote: > Would it be safe to instead put this in the destructor, and use DeleteSoon from > VideoDecoderProxy? I'd prefer simpler destruction semantics if it would work. Stop can cause callbacks to run, and I'd rather not do those while we're in the body of the destructor. https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.cc:161: num_pending_decodes_(0), On 2014/06/05 00:06:24, Ami leaving Chromium June 9th wrote: > the logic around this member requires that Decode() calls correspond 1:1 with > frames, but this isn't doco'd in the header. Done. https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.cc:181: On 2014/06/05 00:06:24, Ami leaving Chromium June 9th wrote: > clear the map once done? It will be destroyed once we exit the body. https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.cc:202: NULL /* extra_data */, On 2014/06/05 00:06:24, Ami leaving Chromium June 9th wrote: > durr, does this really work? > e.g. does the h264 test pass on linux? I don't know. I'll have to try it. I have no idea what to pass in here in any case. https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.cc:207: new media::FFmpegVideoDecoder(media_message_loop_)); On 2014/06/05 00:06:24, Ami leaving Chromium June 9th wrote: > Why not VpxVideoDecoder (for VP9) or one of the decrypting ones? > (seems like if you're gonna hard-code this here then VideoDecoderProxy is not as > generic a proxy as the name might imply; worth a class header doco) You're right. I moved VideoDecoder construction into the Delegate class. I plan to add VP9 to the API and implementation in a future CL. https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.cc:215: state_ = DECODING; On 2014/06/05 00:06:24, Ami leaving Chromium June 9th wrote: > shouldn't this transition only happen in the OK case of OnPipelineStatus()? Yep. https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.cc:238: DCHECK(texture_ids.size()); On 2014/06/05 00:06:24, Ami leaving Chromium June 9th wrote: > DCHECK(texture_id_map_.empty()); > ? I don't think so. We AssignTextures when the video size changes, but we can't dismiss textures until the plugin recycles them. Couldn't the plugin hang on to an old texture indefinitely? https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.cc:240: uint32_t num_textures = static_cast<GLuint>(texture_ids.size()); On 2014/06/05 00:06:24, Ami leaving Chromium June 9th wrote: > checked_cast to avoid silent overflow Done. https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.cc:251: std::make_pair(texture_ids[i], local_texture_ids[i])); On 2014/06/05 00:06:24, Ami leaving Chromium June 9th wrote: > since you're ignoring return value you may as well > texture_id_map_[texture_ids[i]] = local_texture_ids[i]; Done. https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.cc:297: delete this; On 2014/06/05 23:00:43, dmichael wrote: > Can we just do all this in a destructor? (aside from delete this :) ) That's too late. There might be pending callbacks on the main message loop. I've added some comments to make this clearer. https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.cc:346: const uint32_t num_textures = 8; On 2014/06/05 00:06:24, Ami leaving Chromium June 9th wrote: > o rly 8? Yeah, this is just a guess. It seems like 1 might cause things to slow down. I'm open to suggestions. Is it dependent on decoder's GetMaxDecodeRequests number? https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.cc:358: host_->NotifyEndOfBitstreamBuffer(frame->decode_id); On 2014/06/05 00:06:24, Ami leaving Chromium June 9th wrote: > In case of error this is wrong. I don't see why. With VideoDecodeAccelerator we notify when we kick off the Decode. In VDP, we defer that to exert back pressure on the plugin, but the behavior matches that of VDA. https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.cc:368: case media::VideoDecoder::kDecryptError: On 2014/06/05 00:06:24, Ami leaving Chromium June 9th wrote: > _decrypt_ error should be unreachable, right? Done. https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.cc:418: host_->NotifyEndOfBitstreamBuffer(frame->decode_id); On 2014/06/05 00:06:24, Ami leaving Chromium June 9th wrote: > Not sure this is necessary, FWIW. It makes it a little simpler in the host, since I don't need special code to return all shm buffers to the available list. https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... File content/renderer/pepper/video_decoder_proxy.h (right): https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.h:39: // This class proxies calls to a media::VideoDecoder on the media thread. On 2014/06/05 00:06:24, Ami leaving Chromium June 9th wrote: > I think it is the case that this class is meant to be a compatibility shim > between a client that wants to talk to a VDA (PepperVideoDecoderHost) doesn't > have one, so this class cooks up a media::VideoDecoder for it. > You might want to say that in this comment, somehow :) > > Although, wouldn't it simplify the host significantly if this class actually > implemented VDA, so the host could hold either the GVDAH or VDP in the same > scoped_ptr<media::VideoDecodeAccelerator> decoder_; > member? It would definitely simplify the host if this was an adapter that implemented VDA around a VD. I considered this. The two APIs are tantalizingly close but there are a few differences that make it difficult. The first is that VDA::Decode takes a shm handle, which is difficult to map to the address that VD needs to create its DecodeBuffer. A lesser problem is that VDP has to create textures that alias the plugin's, so it creates mailboxes, but there is no way to pass these back via the VDA::Client interface. I could define some methods so VDP could set or query to get missing information but this is messy. Shared memory handles make poor keys. https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.h:42: class VideoDecoderProxy { On 2014/06/05 23:00:43, dmichael wrote: > I can see why you used "Proxy", but I'm so used to that mostly referring to a > proxy for something in another process in Chromium. > > This is basically Adapter pattern, so maybe VideoDecoderAdapter? (Or Wrapper, or > Helper, or ??) > > WDYT? Adapter it is. Done. https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.h:54: protected: On 2014/06/05 00:06:24, Ami leaving Chromium June 9th wrote: > why not private? Done. https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.h:72: uint32_t decode_id; On 2014/06/05 00:06:24, Ami leaving Chromium June 9th wrote: > Here and below, make const-able members const? Done. https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.h:82: std::vector<uint8_t> pixels; On 2014/06/05 00:06:24, Ami leaving Chromium June 9th wrote: > rgba_pixels ? > (or comment on their colorspace) Changed to argb_pixels, to match the conversion function I call. It's confusing to me that the API promises GL_BGRA pixels. Did I get this wrong? https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/c/pp_codecs.... https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.h:87: class Delegate { On 2014/06/05 00:06:24, Ami leaving Chromium June 9th wrote: > optional: here and above, can fwd-declare inner classes here and fully-declare > in the .cc file. Done. https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.h:89: Delegate(const base::WeakPtr<VideoDecoderProxy>& proxy); On 2014/06/05 00:06:24, Ami leaving Chromium June 9th wrote: > explicit Done. https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.h:90: ~Delegate(); On 2014/06/05 00:06:24, Ami leaving Chromium June 9th wrote: > private b/c Destroy? Destroy is a misleading name here, since it doesn't call the dtor. Changing the name to Stop(). https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.h:92: void Initialize(scoped_ptr<media::VideoDecoder> decoder, On 2014/06/05 00:06:24, Ami leaving Chromium June 9th wrote: > decoder seems like a strange thing to pass to Initialize instead of to the ctor. > It could even be new'd _in_ the ctor (and then media_message_loop is implicit as > MessageLoopProxy::current()). Yeah, that works out well. Thanks. https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.h:94: void ReceiveBuffer(uint32_t decode_id, On 2014/06/05 00:06:24, Ami leaving Chromium June 9th wrote: > I get that you want to distinguish between a caller's notion of "Decode" (which > this is) and the Decode call you make from this class (which is what Decode() > below is) but ReceiveBuffer is confusingly passive. > WDYT of QueueDecode() for this and DrainDecode() for the next? I eliminated the second method, and renamed this method Decode. See the implementation for how it works. https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.h:102: void ConvertFrame(uint32_t decode_id, On 2014/06/05 00:06:24, Ami leaving Chromium June 9th wrote: > "convert"? From what to what? > doco please (here and elsewhere where not obvious from name/context) I changed some other method names, and now OnDecodeComplete seems clearest. Done. https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.h:107: base::WeakPtr<VideoDecoderProxy> proxy_; On 2014/06/05 00:06:24, Ami leaving Chromium June 9th wrote: > // Bound to main_message_loop_. I don't understand this comment. I construct and destruct on the main loop but FfmpegVideoDecoder doesn't seem to bind to it. https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.h:120: void OnDestroyComplete(); On 2014/06/05 00:06:24, Ami leaving Chromium June 9th wrote: > Not implemented (and thank goodness, because what would that mean??) Left over from previous patchset. Removed. https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.h:123: void FlushCommandBuffer(); On 2014/06/05 00:06:24, Ami leaving Chromium June 9th wrote: > doco should say when it is required to call this (and why). Done. https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.h:146: typedef std::queue<PendingFrame*> PendingFrameQueue; On 2014/06/05 00:06:24, Ami leaving Chromium June 9th wrote: > y no queue<linked_ptr<PF> > ? Good idea. Done. https://codereview.chromium.org/311853005/diff/50001/content/renderer/pepper/... content/renderer/pepper/video_decoder_proxy.h:170: }; On 2014/06/05 23:00:43, dmichael wrote: > (Note, if you are able to get rid of Destroy and just do stuff in destructors, > then you can get rid of this) I don't think it's possible for this class. https://codereview.chromium.org/311853005/diff/50001/gpu/gpu_ipc.gypi File gpu/gpu_ipc.gypi (right): https://codereview.chromium.org/311853005/diff/50001/gpu/gpu_ipc.gypi#newcode24 gpu/gpu_ipc.gypi:24: 'command_buffer/common/mailbox.h', On 2014/06/05 23:00:44, dmichael wrote: > Isn't the IRT also going to need these? > > It seems like ideally maybe gpu::Mailbox should also live in gpu/ipc, and go > right under sources with ipc/gpu_command_buffer_traits.*? It is in the IRT build, pulled in by gpu_nacl.gyp. This is to pull it into the NaCl helper on Windows. I admit, this seems like a hack to me. https://codereview.chromium.org/311853005/diff/50001/ppapi/examples/video_dec... File ppapi/examples/video_decode/video_decode.cc (right): https://codereview.chromium.org/311853005/diff/50001/ppapi/examples/video_dec... ppapi/examples/video_decode/video_decode.cc:26: #define TEST_VP8 On 2014/06/05 00:06:24, Ami leaving Chromium June 9th wrote: > If you made these USE_VP8_TESTDATA_INSTEAD_OF_H264 then: > - you wouldn't have to edit the _dev.cc example > - you wouldn't risk someone defining both, which is invalid > - you woulnd't risk colliding with far-flung defines from other places since > nobody in their right mind would use such an unwieldy macro name. I like it. Done. https://codereview.chromium.org/311853005/diff/50001/ppapi/examples/video_dec... ppapi/examples/video_decode/video_decode.cc:199: uint32_t frame_size = *reinterpret_cast<const uint32_t*>(&kData[current_pos]); On 2014/06/05 00:06:24, Ami leaving Chromium June 9th wrote: > This is a type-punning violation; please see base/macros.h:bit_cast's commentary > for a description of the problem. > You probably just want to construct the uint32 with bit-shifts and addition, > instead of replicating bit_cast's memcpy strategy. Done. https://codereview.chromium.org/311853005/diff/50001/ppapi/examples/video_dec... ppapi/examples/video_decode/video_decode.cc:205: #endif On 2014/06/05 00:06:24, Ami leaving Chromium June 9th wrote: > #else > #error Oops! > > unless you take my suggestion above. Done. https://codereview.chromium.org/311853005/diff/50001/ppapi/examples/video_dec... ppapi/examples/video_decode/video_decode.cc:221: PP_VIDEOPROFILE_H264MAIN; On 2014/06/05 00:06:24, Ami leaving Chromium June 9th wrote: > This should probably be part of the header file alongside its data. Done. https://codereview.chromium.org/311853005/diff/50001/ppapi/examples/video_dec... ppapi/examples/video_decode/video_decode.cc:386: video_decoders_[i]->Reset(); On 2014/06/05 00:06:24, Ami leaving Chromium June 9th wrote: > I'm surprised you don't end up having to pause/cancel any other things like > decode or pictureready waiting. The example won't call Decode after Reset, since any completed Decodes are aborted after that. I did play with clearing pending paints. That causes one decoder to hang. I think it's because of the way the example paints. It brings up an interesting question: Are any textures the plugin holds valid after Reset? Looking at the implementation, the textures are implicitly recycled back to the proxy, but they remain valid for rendering until the plugin starts calling Decode again. https://codereview.chromium.org/311853005/diff/50001/ppapi/proxy/video_decode... File ppapi/proxy/video_decoder_resource.cc (right): https://codereview.chromium.org/311853005/diff/50001/ppapi/proxy/video_decode... ppapi/proxy/video_decoder_resource.cc:339: DCHECK(!mailboxes.size() || mailboxes.size() == num_textures); On 2014/06/05 23:00:44, dmichael wrote: > !mailboxes.size() > -> > mailboxes.empty() Done. https://codereview.chromium.org/311853005/diff/50001/ppapi/tests/test_video_d... File ppapi/tests/test_video_decoder.cc (right): https://codereview.chromium.org/311853005/diff/50001/ppapi/tests/test_video_d... ppapi/tests/test_video_decoder.cc:8: #include "ppapi/c/ppb_var.h" On 2014/06/05 23:00:43, dmichael wrote: > nit: unused? Done. https://codereview.chromium.org/311853005/diff/50001/ppapi/tests/test_video_d... ppapi/tests/test_video_decoder.cc:29: } On 2014/06/05 23:00:43, dmichael wrote: > You don't seem to use this function Done. https://codereview.chromium.org/311853005/diff/50001/ppapi/tests/test_video_d... File ppapi/tests/test_video_decoder.h (right): https://codereview.chromium.org/311853005/diff/50001/ppapi/tests/test_video_d... ppapi/tests/test_video_decoder.h:18: // TestCase implementation. On 2014/06/05 23:00:43, dmichael wrote: > nit: everything below the constructor can be private. Done.
https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... File content/renderer/pepper/video_decoder_adapter.cc (right): https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... content/renderer/pepper/video_decoder_adapter.cc:52: std::vector<uint8_t> argb_pixels; On 2014/06/06 02:03:43, bbudge wrote: > On 2014/06/05 00:06:24, Ami leaving Chromium June 9th wrote: > > rgba_pixels ? > > (or comment on their colorspace) > > Changed to argb_pixels, to match the conversion function I call. > > It's confusing to me that the API promises GL_BGRA pixels. Did I get this wrong? > https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/c/pp_codecs.... Experimentation is the only authority :( https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... content/renderer/pepper/video_decoder_adapter.cc:108: if (num_pending_decodes_ < max_pending_decodes_) { num_pending_decodes_ holding a value other than pending_decodes_.size() is an unfortunate naming situation. (elsewhere in the VDA world we use the terminology "at decoder" for what you've named {num,max}_pending_decodes_) https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... content/renderer/pepper/video_decoder_adapter.cc:110: decoder_->Decode( DCHECK(pending_decodes_.empty()); https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... content/renderer/pepper/video_decoder_adapter.cc:143: pending_decodes_.pop(); pending_decodes_.clear() instead of the previous two lines? https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... content/renderer/pepper/video_decoder_adapter.cc:154: if (status == media::VideoDecoder::kOk && frame && !frame->end_of_stream()) { && frame is unnecessary here. https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... content/renderer/pepper/video_decoder_adapter.cc:211: DeleteTexture(it->second); On 2014/06/06 02:03:44, bbudge wrote: > On 2014/06/05 00:06:24, Ami leaving Chromium June 9th wrote: > > clear the map once done? > > It will be destroyed once we exit the body. Yeah, I realize; still that's what I'd do. (SEGV's at 0x0 are always more pleasant to debug than UAFs IMO) https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... content/renderer/pepper/video_decoder_adapter.cc:233: NULL /* extra_data */, On 2014/06/06 02:03:45, bbudge wrote: > On 2014/06/05 00:06:24, Ami leaving Chromium June 9th wrote: > > durr, does this really work? > > e.g. does the h264 test pass on linux? > > I don't know. I'll have to try it. Please add a TODO. > I have no idea what to pass in here in any > case. The demuxer gives you the extradata. https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... content/renderer/pepper/video_decoder_adapter.cc:264: DCHECK(texture_ids.size()); On 2014/06/06 02:03:45, bbudge wrote: > On 2014/06/05 00:06:24, Ami leaving Chromium June 9th wrote: > > DCHECK(texture_id_map_.empty()); > > ? > > I don't think so. We AssignTextures when the video size changes, but we can't > dismiss textures until the plugin recycles them. Couldn't the plugin hang on to > an old texture indefinitely? Wouldn't such a texture be in textures_to_dismiss_ as opposed to t_i_m_? https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... content/renderer/pepper/video_decoder_adapter.cc:377: const uint32_t num_textures = 8; On 2014/06/06 02:03:44, bbudge wrote: > On 2014/06/05 00:06:24, Ami leaving Chromium June 9th wrote: > > o rly 8? > > Yeah, this is just a guess. It seems like 1 might cause things to slow down. I'm > open to suggestions. Is it dependent on decoder's GetMaxDecodeRequests number? In order to saturate both the decoder and the renderer you'll want GetMaxDecodeRequests() + media::limits::kMaxVideoFrames https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... File content/renderer/pepper/video_decoder_adapter.h (right): https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... content/renderer/pepper/video_decoder_adapter.h:43: class VideoDecoderAdapter { Pro-tip for the future: if you address all the comments on a file _before_ renaming it, then upload a second patchset to rietveld, then do the rename and upload a third patchset, then your reviewer can effectively review comments & your responses using the side-by-side PS#1->PS#2 view, and take on faith that the only change in PS#2->PS#3 is the rename. When you upload a single PS#2 that contains both all your responses/edits and the rename, rietveld provides no view that lets the reviewer see a side-by-side view of PS#1 (with comments!) comparing to PS#2 :( https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... content/renderer/pepper/video_decoder_adapter.h:43: class VideoDecoderAdapter { I'll let you & dmichael make a final call here but IMO VideoDecodeAdapter is a particularly unfortunate choice: - It appreviates to VDA which is already a term-of-art in this precise area of the codebase - It implies that a VideoDecoder will be passed in, to be adapted (which is untrue since the VD is new'd as part of the impl internally) - It doesn't say what the VD is being Adapter'd _to_. If it was up to me I think I'd name this something more obviously double-take-inducing, since there are only ever going to be few call-sites and the thing that this class does is atypical. VideoDecoderBasedPseudoVDA VDALikeVideoDecoderWrapper etc. Like I said, up to you & dmichael. (and moot if you're able to take my suggestion below to make this a true VDA) https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... content/renderer/pepper/video_decoder_adapter.h:45: explicit VideoDecoderAdapter(PepperVideoDecoderHost* host); On 2014/06/06 02:03:44, bbudge wrote: > On 2014/06/05 00:06:24, Ami leaving Chromium June 9th wrote: > > I think it is the case that this class is meant to be a compatibility shim > > between a client that wants to talk to a VDA (PepperVideoDecoderHost) doesn't > > have one, so this class cooks up a media::VideoDecoder for it. > > You might want to say that in this comment, somehow :) > > > > Although, wouldn't it simplify the host significantly if this class actually > > implemented VDA, so the host could hold either the GVDAH or VDP in the same > > scoped_ptr<media::VideoDecodeAccelerator> decoder_; > > member? > > It would definitely simplify the host if this was an adapter that implemented > VDA around a VD. I considered this. The two APIs are tantalizingly close but > there are a few differences that make it difficult. The first is that > VDA::Decode takes a shm handle, which is difficult to map to the address that VD > needs to create its DecodeBuffer. Why? Isn't it as simple as SharedMemory shm(handle, true); ...use shm->Map()'s return value? > A lesser problem is that VDP has to create > textures that alias the plugin's, so it creates mailboxes, but there is no way > to pass these back via the VDA::Client interface. Just because _this_ class implements VDA doesn't mean that it is restricted to VDA::Client's interface for talking back to the host. https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... content/renderer/pepper/video_decoder_adapter.h:68: // destructed on the media thread. Comment should also say what this is for. https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... content/renderer/pepper/video_decoder_adapter.h:69: class Delegate { Delegate is used in chrome as a synonym for "Client" and is almost always an interface. This seems like an impl detail whose raison d'etre is the need to trampoline VDAdapter calls from the main thread to the media thread to do the real work there, and then trampoline the results back, maybe? If so rename to Impl? https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... content/renderer/pepper/video_decoder_adapter.h:69: class Delegate { fwd-declare? https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... content/renderer/pepper/video_decoder_adapter.h:86: base::WeakPtr<VideoDecoderAdapter> adapter_; On 2014/06/06 02:03:43, bbudge wrote: > On 2014/06/05 00:06:24, Ami leaving Chromium June 9th wrote: > > // Bound to main_message_loop_. > > I don't understand this comment. I construct and destruct on the main loop but > FfmpegVideoDecoder doesn't seem to bind to it. My comment is about the weakptr's thread-binding, not FFVD's. WeakPtrs can only be dereferenced & invalidated on a single thread, and in that sense they are bound to that thread. I find it helpful to document which thread a weakptr member is bound to. https://codereview.chromium.org/311853005/diff/60001/ppapi/tests/test_video_d... File ppapi/tests/test_video_decoder.cc (right): https://codereview.chromium.org/311853005/diff/60001/ppapi/tests/test_video_d... ppapi/tests/test_video_decoder.cc:58: // Accept PP_ERROR_NOTSUPPORTED since some platforms may not support VP8. how's that? (all shipping chromes support VP8 except for possibly Chrome/iOS but that doesn't support pepper either, so irrelevant for this test, right?)
https://codereview.chromium.org/311853005/diff/50001/gpu/gpu_ipc.gypi File gpu/gpu_ipc.gypi (right): https://codereview.chromium.org/311853005/diff/50001/gpu/gpu_ipc.gypi#newcode24 gpu/gpu_ipc.gypi:24: 'command_buffer/common/mailbox.h', On 2014/06/06 02:03:46, bbudge wrote: > On 2014/06/05 23:00:44, dmichael wrote: > > Isn't the IRT also going to need these? > > > > It seems like ideally maybe gpu::Mailbox should also live in gpu/ipc, and go > > right under sources with ipc/gpu_command_buffer_traits.*? > > It is in the IRT build, pulled in by gpu_nacl.gyp. This is to pull it into the > NaCl helper on Windows. I admit, this seems like a hack to me. Maybe you should make a new appropriate target, like a duplicate of this: https://code.google.com/p/chromium/codesearch#chromium/src/gpu/gpu.gyp&l=570 I'm a little unsure about the organization of files and targets in gpu, though. https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... File content/renderer/pepper/pepper_video_decoder_host.cc (right): https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_video_decoder_host.cc:384: NotifyError(pp_error); I still would prefer that you rename one of the NotifyError functions. https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... File content/renderer/pepper/video_decoder_adapter.cc (right): https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... content/renderer/pepper/video_decoder_adapter.cc:52: std::vector<uint8_t> argb_pixels; private: DISALLOW_COPY_AND_ASSIGN(PendingFrame); ? It's copyable, but that would be slightly expensive, so I think you want to protect yourself from doing it accidentally. https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... content/renderer/pepper/video_decoder_adapter.cc:124: PendingDecode& decode = pending_decodes_.front(); nit: const& ? https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... content/renderer/pepper/video_decoder_adapter.cc:264: DCHECK(texture_ids.size()); I think you should maybe reformat this to something like: if (textured_ids.empty()) { NOTREACHED(); return; } So that you avoid calling front() on an empty vector below if somehow the condition doesn't hold. https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... content/renderer/pepper/video_decoder_adapter.cc:325: base::Owned(delegate_.release()))); You convinced me that VideoDecoderAdapter::Delegate should have Stop and not just do everything in Delegate's destructor. But... this above still looks like the above could all happen in VideoDecoderAdapter's destructor, and there's no need to do anything special for scoped_ptr<> for VideoDecoderAdapter. https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... File content/renderer/pepper/video_decoder_adapter.h (right): https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... content/renderer/pepper/video_decoder_adapter.h:18: #include "media/base/decoder_buffer.h" nit: This could probably be a forward reference https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... content/renderer/pepper/video_decoder_adapter.h:20: #include "media/base/video_decoder_config.h" nit: ditto (I think even by-value params can be forward declared) https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... content/renderer/pepper/video_decoder_adapter.h:86: base::WeakPtr<VideoDecoderAdapter> adapter_; I think Ami's prior comment here was that you should document that adapter_ is bound to the main thread. In other words, you should never de-reference it on the media thread. It should only be used to bind callbacks for invoking on the main thread. https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... content/renderer/pepper/video_decoder_adapter.h:160: }; I still think you can get away without this for VideoDecoderAdapter :). If I'm wrong, sorry for harping on it. (See comment in Destroy definition)
Renamed VideoDecoderAdapter to VideoDecoderShim. VideoDecoderShim implements media::VDA. Renamed Delegate to DecoderImpl. Reworked how the shim calls Decode to fix potential bug. Lots of other clean up. https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... File content/renderer/pepper/pepper_video_decoder_host.cc (right): https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_video_decoder_host.cc:384: NotifyError(pp_error); On 2014/06/06 17:24:01, dmichael wrote: > I still would prefer that you rename one of the NotifyError functions. Whoops, missed that. Let me name it SendNotifyErrorMsg. https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... File content/renderer/pepper/video_decoder_adapter.cc (right): https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... content/renderer/pepper/video_decoder_adapter.cc:52: std::vector<uint8_t> argb_pixels; On 2014/06/06 17:14:33, Ami leaving Chromium June 6th wrote: > On 2014/06/06 02:03:43, bbudge wrote: > > On 2014/06/05 00:06:24, Ami leaving Chromium June 9th wrote: > > > rgba_pixels ? > > > (or comment on their colorspace) > > > > Changed to argb_pixels, to match the conversion function I call. > > > > It's confusing to me that the API promises GL_BGRA pixels. Did I get this > wrong? > > > https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/c/pp_codecs.... > > Experimentation is the only authority :( Done. https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... content/renderer/pepper/video_decoder_adapter.cc:52: std::vector<uint8_t> argb_pixels; On 2014/06/06 17:24:00, dmichael wrote: > private: > DISALLOW_COPY_AND_ASSIGN(PendingFrame); > > ? > > It's copyable, but that would be slightly expensive, so I think you want to > protect yourself from doing it accidentally. Looks like ARGB. I guess as long as there are an even number of endian flips... https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... content/renderer/pepper/video_decoder_adapter.cc:108: if (num_pending_decodes_ < max_pending_decodes_) { On 2014/06/06 17:14:33, Ami leaving Chromium June 6th wrote: > num_pending_decodes_ holding a value other than > pending_decodes_.size() is an unfortunate naming situation. > > (elsewhere in the VDA world we use the terminology "at decoder" for what you've > named {num,max}_pending_decodes_) Done. https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... content/renderer/pepper/video_decoder_adapter.cc:110: decoder_->Decode( On 2014/06/06 17:14:33, Ami leaving Chromium June 6th wrote: > DCHECK(pending_decodes_.empty()); It might not be true. I think you identified a real problem though. I changed back to having a separate DoDecode function, and having Decode just push the latest buffer and then call DoDecode, to ensure that buffers never get sent in the wrong order. This might have been a bug. Is it possible that the example video can tolerate some buffers out of order (since it only has 1 seek point)? https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... content/renderer/pepper/video_decoder_adapter.cc:124: PendingDecode& decode = pending_decodes_.front(); On 2014/06/06 17:24:00, dmichael wrote: > nit: const& ? Done. https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... content/renderer/pepper/video_decoder_adapter.cc:143: pending_decodes_.pop(); On 2014/06/06 17:14:33, Ami leaving Chromium June 6th wrote: > pending_decodes_.clear() instead of the previous two lines? Can't, it's std::queue. https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... content/renderer/pepper/video_decoder_adapter.cc:154: if (status == media::VideoDecoder::kOk && frame && !frame->end_of_stream()) { On 2014/06/06 17:14:33, Ami leaving Chromium June 6th wrote: > && frame is unnecessary here. Done. https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... content/renderer/pepper/video_decoder_adapter.cc:211: DeleteTexture(it->second); On 2014/06/06 17:14:33, Ami leaving Chromium June 6th wrote: > On 2014/06/06 02:03:44, bbudge wrote: > > On 2014/06/05 00:06:24, Ami leaving Chromium June 9th wrote: > > > clear the map once done? > > > > It will be destroyed once we exit the body. > > Yeah, I realize; still that's what I'd do. > (SEGV's at 0x0 are always more pleasant to debug than UAFs IMO) Done. https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... content/renderer/pepper/video_decoder_adapter.cc:233: NULL /* extra_data */, On 2014/06/06 17:14:33, Ami leaving Chromium June 6th wrote: > On 2014/06/06 02:03:45, bbudge wrote: > > On 2014/06/05 00:06:24, Ami leaving Chromium June 9th wrote: > > > durr, does this really work? > > > e.g. does the h264 test pass on linux? > > > > I don't know. I'll have to try it. > > Please add a TODO. > > > I have no idea what to pass in here in any > > case. > > The demuxer gives you the extradata. Done. https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... content/renderer/pepper/video_decoder_adapter.cc:264: DCHECK(texture_ids.size()); On 2014/06/06 17:14:33, Ami leaving Chromium June 6th wrote: > On 2014/06/06 02:03:45, bbudge wrote: > > On 2014/06/05 00:06:24, Ami leaving Chromium June 9th wrote: > > > DCHECK(texture_id_map_.empty()); > > > ? > > > > I don't think so. We AssignTextures when the video size changes, but we can't > > dismiss textures until the plugin recycles them. Couldn't the plugin hang on > to > > an old texture indefinitely? > > Wouldn't such a texture be in textures_to_dismiss_ as opposed to t_i_m_? It would be in both. textures_to_dismiss_ may contain textures that are still in use by the plugin. https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... content/renderer/pepper/video_decoder_adapter.cc:264: DCHECK(texture_ids.size()); On 2014/06/06 17:24:00, dmichael wrote: > I think you should maybe reformat this to something like: > if (textured_ids.empty()) { > NOTREACHED(); > return; > } > > So that you avoid calling front() on an empty vector below if somehow the > condition doesn't hold. Done. https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... content/renderer/pepper/video_decoder_adapter.cc:325: base::Owned(delegate_.release()))); On 2014/06/06 17:24:00, dmichael wrote: > You convinced me that VideoDecoderAdapter::Delegate should have Stop and not > just do everything in Delegate's destructor. But... this above still looks like > the above could all happen in VideoDecoderAdapter's destructor, and there's no > need to do anything special for scoped_ptr<> for VideoDecoderAdapter. We're posting a task to the media loop to Stop the decoder. But there may be tasks on the main loop with base::Unretained's to this adapter, or tasks on the media thread ahead of the Stop which will post such tasks. We can't delete this class until we're sure the Delegate has stopped. https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... content/renderer/pepper/video_decoder_adapter.cc:377: const uint32_t num_textures = 8; On 2014/06/06 17:14:33, Ami leaving Chromium June 6th wrote: > On 2014/06/06 02:03:44, bbudge wrote: > > On 2014/06/05 00:06:24, Ami leaving Chromium June 9th wrote: > > > o rly 8? > > > > Yeah, this is just a guess. It seems like 1 might cause things to slow down. > I'm > > open to suggestions. Is it dependent on decoder's GetMaxDecodeRequests number? > > In order to saturate both the decoder and the renderer you'll want > GetMaxDecodeRequests() + media::limits::kMaxVideoFrames Done. https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... File content/renderer/pepper/video_decoder_adapter.h (right): https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... content/renderer/pepper/video_decoder_adapter.h:18: #include "media/base/decoder_buffer.h" On 2014/06/06 17:24:01, dmichael wrote: > nit: This could probably be a forward reference Done. https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... content/renderer/pepper/video_decoder_adapter.h:20: #include "media/base/video_decoder_config.h" On 2014/06/06 17:24:00, dmichael wrote: > nit: ditto (I think even by-value params can be forward declared) Can't, it's an enum. I was able to fwd decl VideoDecoder though. https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... content/renderer/pepper/video_decoder_adapter.h:43: class VideoDecoderAdapter { On 2014/06/06 17:14:34, Ami leaving Chromium June 6th wrote: > I'll let you & dmichael make a final call here but IMO VideoDecodeAdapter is a > particularly unfortunate choice: > - It appreviates to VDA which is already a term-of-art in this precise area of > the codebase > - It implies that a VideoDecoder will be passed in, to be adapted (which is > untrue since the VD is new'd as part of the impl internally) > - It doesn't say what the VD is being Adapter'd _to_. > > If it was up to me I think I'd name this something more obviously > double-take-inducing, since there are only ever going to be few call-sites and > the thing that this class does is atypical. > > VideoDecoderBasedPseudoVDA > VDALikeVideoDecoderWrapper > etc. > > Like I said, up to you & dmichael. > (and moot if you're able to take my suggestion below to make this a true VDA) OK, how about VideoDecoderShim? https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... content/renderer/pepper/video_decoder_adapter.h:43: class VideoDecoderAdapter { On 2014/06/06 17:14:34, Ami leaving Chromium June 6th wrote: > Pro-tip for the future: if you address all the comments on a file > _before_ renaming it, then upload a second patchset to rietveld, then do > the rename and upload a third patchset, then your reviewer can > effectively review comments & your responses using the side-by-side > PS#1->PS#2 view, and take on faith that the only change in PS#2->PS#3 is > the rename. When you upload a single PS#2 that contains both all your > responses/edits and the rename, rietveld provides no view that lets the > reviewer see a side-by-side view of PS#1 (with comments!) comparing to > PS#2 :( Sorry, I didn't realize that. Thanks for the pro-tip. https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... content/renderer/pepper/video_decoder_adapter.h:45: explicit VideoDecoderAdapter(PepperVideoDecoderHost* host); On 2014/06/06 17:14:34, Ami leaving Chromium June 6th wrote: > On 2014/06/06 02:03:44, bbudge wrote: > > On 2014/06/05 00:06:24, Ami leaving Chromium June 9th wrote: > > > I think it is the case that this class is meant to be a compatibility shim > > > between a client that wants to talk to a VDA (PepperVideoDecoderHost) > doesn't > > > have one, so this class cooks up a media::VideoDecoder for it. > > > You might want to say that in this comment, somehow :) > > > > > > Although, wouldn't it simplify the host significantly if this class actually > > > implemented VDA, so the host could hold either the GVDAH or VDP in the same > > > scoped_ptr<media::VideoDecodeAccelerator> decoder_; > > > member? > > > > It would definitely simplify the host if this was an adapter that implemented > > VDA around a VD. I considered this. The two APIs are tantalizingly close but > > there are a few differences that make it difficult. The first is that > > VDA::Decode takes a shm handle, which is difficult to map to the address that > VD > > needs to create its DecodeBuffer. > > Why? Isn't it as simple as > SharedMemory shm(handle, true); > ...use shm->Map()'s return value? > > > A lesser problem is that VDP has to create > > textures that alias the plugin's, so it creates mailboxes, but there is no way > > to pass these back via the VDA::Client interface. > > Just because _this_ class implements VDA doesn't mean that it is restricted to > VDA::Client's interface for talking back to the host The adapter class now implements media::VDA. Done. https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... content/renderer/pepper/video_decoder_adapter.h:68: // destructed on the media thread. On 2014/06/06 17:14:34, Ami leaving Chromium June 6th wrote: > Comment should also say what this is for. Done. https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... content/renderer/pepper/video_decoder_adapter.h:69: class Delegate { On 2014/06/06 17:14:34, Ami leaving Chromium June 6th wrote: > fwd-declare? Done. https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... content/renderer/pepper/video_decoder_adapter.h:69: class Delegate { On 2014/06/06 17:14:34, Ami leaving Chromium June 6th wrote: > Delegate is used in chrome as a synonym for "Client" and is almost always an > interface. This seems like an impl detail whose raison d'etre is the need to > trampoline VDAdapter calls from the main thread to the media thread to do the > real work there, and then trampoline the results back, maybe? > > If so rename to Impl? Renamed to VideoDecoderShim::DecoderImpl. https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... content/renderer/pepper/video_decoder_adapter.h:86: base::WeakPtr<VideoDecoderAdapter> adapter_; On 2014/06/06 17:14:33, Ami leaving Chromium June 6th wrote: > On 2014/06/06 02:03:43, bbudge wrote: > > On 2014/06/05 00:06:24, Ami leaving Chromium June 9th wrote: > > > // Bound to main_message_loop_. > > > > I don't understand this comment. I construct and destruct on the main loop but > > FfmpegVideoDecoder doesn't seem to bind to it. > > My comment is about the weakptr's thread-binding, not FFVD's. > WeakPtrs can only be dereferenced & invalidated on a single thread, and in that > sense they are bound to that thread. I find it helpful to document which thread > a weakptr member is bound to. Done. https://codereview.chromium.org/311853005/diff/60001/ppapi/tests/test_video_d... File ppapi/tests/test_video_decoder.cc (right): https://codereview.chromium.org/311853005/diff/60001/ppapi/tests/test_video_d... ppapi/tests/test_video_decoder.cc:58: // Accept PP_ERROR_NOTSUPPORTED since some platforms may not support VP8. On 2014/06/06 17:14:33, Ami leaving Chromium June 6th wrote: > how's that? > (all shipping chromes support VP8 except for possibly Chrome/iOS but that > doesn't support pepper either, so irrelevant for this test, right?) I did this because the Create success test was failing on some bots. Unfortunately, my latest try runs fail compile because the testdata.h file doesn't appear to get uploaded and compile fails, so I can't confirm that this fixes it. Could it be that some chromium builds don't support VP8? Here's a sample failing log: http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...
https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... File content/renderer/pepper/video_decoder_adapter.cc (right): https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... content/renderer/pepper/video_decoder_adapter.cc:325: base::Owned(delegate_.release()))); On 2014/06/07 00:31:09, bbudge wrote: > On 2014/06/06 17:24:00, dmichael wrote: > > You convinced me that VideoDecoderAdapter::Delegate should have Stop and not > > just do everything in Delegate's destructor. But... this above still looks > like > > the above could all happen in VideoDecoderAdapter's destructor, and there's no > > need to do anything special for scoped_ptr<> for VideoDecoderAdapter. > > We're posting a task to the media loop to Stop the decoder. But there may be > tasks on the main loop with base::Unretained's to this adapter, or tasks on the > media thread ahead of the Stop which will post such tasks. We can't delete this > class until we're sure the Delegate has stopped. Ugh, there you go being right again. Yeah, since I'm invoking the dtor below, this could all be in the regular dtor and we don't need the Deleter. Done. https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... File content/renderer/pepper/video_decoder_adapter.h (right): https://codereview.chromium.org/311853005/diff/60001/content/renderer/pepper/... content/renderer/pepper/video_decoder_adapter.h:160: }; On 2014/06/06 17:24:00, dmichael wrote: > I still think you can get away without this for VideoDecoderAdapter :). If I'm > wrong, sorry for harping on it. (See comment in Destroy definition) People who are right can harp. Done.
https://codereview.chromium.org/311853005/diff/80001/content/renderer/pepper/... File content/renderer/pepper/video_decoder_shim.cc (right): https://codereview.chromium.org/311853005/diff/80001/content/renderer/pepper/... content/renderer/pepper/video_decoder_shim.cc:124: new media::FFmpegVideoDecoder(base::MessageLoopProxy::current())); Please call set_decode_nalus(true) to allow processing of individual NALU's. See https://codereview.chromium.org/301243008/ that was just committed.
@igorc: please see q to you in my comments below. https://codereview.chromium.org/311853005/diff/70002/content/renderer/pepper/... File content/renderer/pepper/pepper_video_decoder_host.h (right): https://codereview.chromium.org/311853005/diff/70002/content/renderer/pepper/... content/renderer/pepper/pepper_video_decoder_host.h:89: // These methods are needed by VideoDecodeAdapter, to look like a s/Adapter/Shim/ (please make a pass) https://codereview.chromium.org/311853005/diff/70002/content/renderer/pepper/... content/renderer/pepper/pepper_video_decoder_host.h:113: // A map of shm handles to memory addresses, to assist VideoDecoderShim. If you didn't need to go from Handle to addr (only from shm_id to addr) then it would be simpler to just have a parallel vector shm_addrs_ to shm_buffers_ indexed by shm_id, right? And AFAICT you only use ShmHandleTo* from outside this class while holding a BitstreamBuffer, which has the id, too, so I think this should be doable (and result in simpler/safer code). https://codereview.chromium.org/311853005/diff/70002/content/renderer/pepper/... File content/renderer/pepper/video_decoder_shim.cc (right): https://codereview.chromium.org/311853005/diff/70002/content/renderer/pepper/... content/renderer/pepper/video_decoder_shim.cc:131: true /* low_delay */, On 2014/06/07 05:34:34, igorc wrote: > Please call set_decode_nalus(true) to allow processing of individual NALU's. See > https://codereview.chromium.org/301243008/ that was just committed. Igor: the commentary on your new API is that it disables low_delay. Does that mean this true should turn to false to avoid misleading readers? Or should it stay true in case the future improves, but with a comment saying it is ignored by the ffmpeg impl? Or something else? https://codereview.chromium.org/311853005/diff/70002/content/renderer/pepper/... content/renderer/pepper/video_decoder_shim.cc:167: DoDecode(); On 2014/06/07 00:31:09, bbudge wrote: > On 2014/06/06 17:14:33, Ami leaving Chromium June 6th wrote: > > DCHECK(pending_decodes_.empty()); > > It might not be true. I think you identified a real problem though. > > I changed back to having a separate DoDecode function, and having Decode just > push the latest buffer and then call DoDecode, to ensure that buffers never get > sent in the wrong order. > > This might have been a bug. lol I start out thinking it was a bug but convinced myself it wasn't possible that to arrive at a situation where one buffer is queued and a later buffer is passed to the decoder ahead of the queued one. Anyway I like this formulation better. > Is it possible that the example video can tolerate > some buffers out of order (since it only has 1 seek point)? ffmpeg is _very_ forgiving; it will try as hard as it can to keep chugging even in the face of corrupt inputs (at the cost of visual corruption / stuttering / jank). It can be useful to test the returned frames for bit-exactness with some expectation. (see e.g. the hash & thumbnail-related code in content/common/gpu/media's video decode tests) https://codereview.chromium.org/311853005/diff/70002/content/renderer/pepper/... content/renderer/pepper/video_decoder_shim.cc:310: // Ignore the |client| parameter, which is the same as |host_|. DCHECK this instead of the comment? https://codereview.chromium.org/311853005/diff/70002/ppapi/tests/test_video_d... File ppapi/tests/test_video_decoder.cc (right): https://codereview.chromium.org/311853005/diff/70002/ppapi/tests/test_video_d... ppapi/tests/test_video_decoder.cc:58: // Accept PP_ERROR_NOTSUPPORTED since some platforms may not support VP8. On 2014/06/07 00:31:08, bbudge wrote: > On 2014/06/06 17:14:33, Ami leaving Chromium June 6th wrote: > > how's that? > > (all shipping chromes support VP8 except for possibly Chrome/iOS but that > > doesn't support pepper either, so irrelevant for this test, right?) > > I did this because the Create success test was failing on some bots. > Unfortunately, my latest try runs fail compile because the testdata.h file > doesn't appear to get uploaded and compile fails, so I can't confirm that this > fixes it. Could it be that some chromium builds don't support VP8? > > Here's a sample failing log: > http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/... There are no chromium platforms that don't support VP8 but do support pepper. I see PS#7 now asserts OK so I think you just need to drop this comment line now.
https://codereview.chromium.org/311853005/diff/70002/content/renderer/pepper/... File content/renderer/pepper/pepper_video_decoder_host.h (right): https://codereview.chromium.org/311853005/diff/70002/content/renderer/pepper/... content/renderer/pepper/pepper_video_decoder_host.h:89: // These methods are needed by VideoDecodeAdapter, to look like a On 2014/06/07 18:26:39, Ami leaving Chromium June 6th wrote: > s/Adapter/Shim/ > (please make a pass) Done. https://codereview.chromium.org/311853005/diff/70002/content/renderer/pepper/... content/renderer/pepper/pepper_video_decoder_host.h:113: // A map of shm handles to memory addresses, to assist VideoDecoderShim. On 2014/06/07 18:26:39, Ami leaving Chromium June 6th wrote: > If you didn't need to go from Handle to addr (only from shm_id to addr) then it > would be simpler to just have a parallel vector shm_addrs_ to shm_buffers_ > indexed by shm_id, right? > And AFAICT you only use ShmHandleTo* from outside this class while holding a > BitstreamBuffer, which has the id, too, so I think this should be doable (and > result in simpler/safer code). So much better. Done. https://codereview.chromium.org/311853005/diff/70002/content/renderer/pepper/... File content/renderer/pepper/video_decoder_shim.cc (right): https://codereview.chromium.org/311853005/diff/70002/content/renderer/pepper/... content/renderer/pepper/video_decoder_shim.cc:167: DoDecode(); On 2014/06/07 18:26:40, Ami leaving Chromium June 6th wrote: > On 2014/06/07 00:31:09, bbudge wrote: > > On 2014/06/06 17:14:33, Ami leaving Chromium June 6th wrote: > > > DCHECK(pending_decodes_.empty()); > > > > It might not be true. I think you identified a real problem though. > > > > I changed back to having a separate DoDecode function, and having Decode just > > push the latest buffer and then call DoDecode, to ensure that buffers never > get > > sent in the wrong order. > > > > This might have been a bug. > > lol I start out thinking it was a bug but convinced myself it wasn't possible > that to arrive at a situation where one buffer is queued and a later buffer is > passed to the decoder ahead of the queued one. Anyway I like this formulation > better. > > > Is it possible that the example video can tolerate > > some buffers out of order (since it only has 1 seek point)? > > ffmpeg is _very_ forgiving; it will try as hard as it can to keep chugging even > in the face of corrupt inputs (at the cost of visual corruption / stuttering / > jank). It can be useful to test the returned frames for bit-exactness with some > expectation. > (see e.g. the hash & thumbnail-related code in content/common/gpu/media's video > decode tests) I am still searching for some way to test this more rigorously (i.e. browser_tests, content_browsertests). What I need is some compact source of bitstream data, like a test pattern. Any pointers greatly appreciated! https://codereview.chromium.org/311853005/diff/70002/content/renderer/pepper/... content/renderer/pepper/video_decoder_shim.cc:310: // Ignore the |client| parameter, which is the same as |host_|. On 2014/06/07 18:26:39, Ami leaving Chromium June 6th wrote: > DCHECK this instead of the comment? Done. https://codereview.chromium.org/311853005/diff/70002/ppapi/tests/test_video_d... File ppapi/tests/test_video_decoder.cc (right): https://codereview.chromium.org/311853005/diff/70002/ppapi/tests/test_video_d... ppapi/tests/test_video_decoder.cc:58: // Accept PP_ERROR_NOTSUPPORTED since some platforms may not support VP8. On 2014/06/07 18:26:40, Ami leaving Chromium June 6th wrote: > On 2014/06/07 00:31:08, bbudge wrote: > > On 2014/06/06 17:14:33, Ami leaving Chromium June 6th wrote: > > > how's that? > > > (all shipping chromes support VP8 except for possibly Chrome/iOS but that > > > doesn't support pepper either, so irrelevant for this test, right?) > > > > I did this because the Create success test was failing on some bots. > > Unfortunately, my latest try runs fail compile because the testdata.h file > > doesn't appear to get uploaded and compile fails, so I can't confirm that this > > fixes it. Could it be that some chromium builds don't support VP8? > > > > Here's a sample failing log: > > > http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/... > > There are no chromium platforms that don't support VP8 but do support pepper. > I see PS#7 now asserts OK so I think you just need to drop this comment line > now. Yes, the 1 failure I see is because of a bad graphics 3d resource. I'll look into that.
LGTM I'll let you duke out the low_delay vs. set_nalus(true) business with igorc/sergeyu. https://codereview.chromium.org/311853005/diff/100001/content/renderer/pepper... File content/renderer/pepper/video_decoder_shim.cc (right): https://codereview.chromium.org/311853005/diff/100001/content/renderer/pepper... content/renderer/pepper/video_decoder_shim.cc:125: ffmpeg_video_decoder->set_decode_nalus(true); Still concerned that this conflicts with the true low_delay value at l.133. https://codereview.chromium.org/311853005/diff/100001/content/renderer/pepper... content/renderer/pepper/video_decoder_shim.cc:169: DoDecode(); On 2014/06/07 21:28:33, bbudge wrote: > On 2014/06/07 18:26:40, Ami leaving Chromium June 6th wrote: > > On 2014/06/07 00:31:09, bbudge wrote: > > > On 2014/06/06 17:14:33, Ami leaving Chromium June 6th wrote: > > > > DCHECK(pending_decodes_.empty()); > > > > > > It might not be true. I think you identified a real problem though. > > > > > > I changed back to having a separate DoDecode function, and having Decode > just > > > push the latest buffer and then call DoDecode, to ensure that buffers never > > get > > > sent in the wrong order. > > > > > > This might have been a bug. > > > > lol I start out thinking it was a bug but convinced myself it wasn't possible > > that to arrive at a situation where one buffer is queued and a later buffer is > > passed to the decoder ahead of the queued one. Anyway I like this formulation > > better. > > > > > Is it possible that the example video can tolerate > > > some buffers out of order (since it only has 1 seek point)? > > > > ffmpeg is _very_ forgiving; it will try as hard as it can to keep chugging > even > > in the face of corrupt inputs (at the cost of visual corruption / stuttering / > > jank). It can be useful to test the returned frames for bit-exactness with > some > > expectation. > > (see e.g. the hash & thumbnail-related code in content/common/gpu/media's > video > > decode tests) > > I am still searching for some way to test this more rigorously (i.e. > browser_tests, content_browsertests). What I need is some compact source of > bitstream data, like a test pattern. Any pointers greatly appreciated! There are a bunch of encoded test streams in media/test/data. To test from a plugin you'd have to set up a demuxer in the plugin, and also hook up file access.
On 2014/06/04 22:48:01, sievers wrote: > On 2014/06/04 22:01:06, bbudge wrote: > > +sievers for real > > +piman > > It feels a bit weird that this would be nacl-specific. It probably just happens > to work for the other components since all the files end up in a target > together, while strictly speaking the dependencies are slightly strange (gpu_ipc > really needs types from gpu_common, while gpu depends on gpu_ipc). gpu should not depend on gpu_ipc. gpu_ipc is only needed by clients of gpu that use IPC (so that we don't need gpu to depend on ipc). We should just be depending on command_Buffer_common here. > > Also, gpu_command_buffer_traits.cc depends on MailboxHolder, so why do you get > away without adding mailbox_holder.h and mailbox_holder.cc also? > > Alternatively, moving mailbox/mailbox_holder.cc/.h to gpu/ipc seems a bit odd > too. We could also add another gpu_base.gypi for these basic types. > > Or am I overthinking it and it just needs to compile+link? :)
lgtm
On 2014/06/09 at 14:47:36, piman wrote: > On 2014/06/04 22:48:01, sievers wrote: > > On 2014/06/04 22:01:06, bbudge wrote: > > > +sievers for real > > > > +piman > > > > It feels a bit weird that this would be nacl-specific. It probably just happens > > to work for the other components since all the files end up in a target > > together, while strictly speaking the dependencies are slightly strange (gpu_ipc > > really needs types from gpu_common, while gpu depends on gpu_ipc). > > gpu should not depend on gpu_ipc. > gpu_ipc is only needed by clients of gpu that use IPC (so that we don't need gpu to depend on ipc). > > We should just be depending on command_Buffer_common here. > > > > > Also, gpu_command_buffer_traits.cc depends on MailboxHolder, so why do you get > > away without adding mailbox_holder.h and mailbox_holder.cc also? > > > > Alternatively, moving mailbox/mailbox_holder.cc/.h to gpu/ipc seems a bit odd > > too. We could also add another gpu_base.gypi for these basic types. > > > > Or am I overthinking it and it just needs to compile+link? :) As we discussed, I've added a command_buffer_common_win64 target in gpu.gyp. I had to remove an #ifdef that caused the compile to fail. Fortunately, that was all it took to build.
https://codereview.chromium.org/311853005/diff/100001/content/renderer/pepper... File content/renderer/pepper/video_decoder_shim.cc (right): https://codereview.chromium.org/311853005/diff/100001/content/renderer/pepper... content/renderer/pepper/video_decoder_shim.cc:125: ffmpeg_video_decoder->set_decode_nalus(true); On 2014/06/08 17:30:38, Ami leaving Chromium June 6th wrote: > Still concerned that this conflicts with the true low_delay value at l.133. I'll work this out with Igor. https://codereview.chromium.org/311853005/diff/100001/content/renderer/pepper... content/renderer/pepper/video_decoder_shim.cc:169: DoDecode(); On 2014/06/08 17:30:38, Ami leaving Chromium June 6th wrote: > On 2014/06/07 21:28:33, bbudge wrote: > > On 2014/06/07 18:26:40, Ami leaving Chromium June 6th wrote: > > > On 2014/06/07 00:31:09, bbudge wrote: > > > > On 2014/06/06 17:14:33, Ami leaving Chromium June 6th wrote: > > > > > DCHECK(pending_decodes_.empty()); > > > > > > > > It might not be true. I think you identified a real problem though. > > > > > > > > I changed back to having a separate DoDecode function, and having Decode > > just > > > > push the latest buffer and then call DoDecode, to ensure that buffers > never > > > get > > > > sent in the wrong order. > > > > > > > > This might have been a bug. > > > > > > lol I start out thinking it was a bug but convinced myself it wasn't > possible > > > that to arrive at a situation where one buffer is queued and a later buffer > is > > > passed to the decoder ahead of the queued one. Anyway I like this > formulation > > > better. > > > > > > > Is it possible that the example video can tolerate > > > > some buffers out of order (since it only has 1 seek point)? > > > > > > ffmpeg is _very_ forgiving; it will try as hard as it can to keep chugging > > even > > > in the face of corrupt inputs (at the cost of visual corruption / stuttering > / > > > jank). It can be useful to test the returned frames for bit-exactness with > > some > > > expectation. > > > (see e.g. the hash & thumbnail-related code in content/common/gpu/media's > > video > > > decode tests) > > > > I am still searching for some way to test this more rigorously (i.e. > > browser_tests, content_browsertests). What I need is some compact source of > > bitstream data, like a test pattern. Any pointers greatly appreciated! > > There are a bunch of encoded test streams in media/test/data. To test from a > plugin you'd have to set up a demuxer in the plugin, and also hook up file > access. I was thinking along the lines of some very simple series of frames, like a sequence of solid color frames, so the data could be compact and so we could test that the plumbing is correct, rather than exercise the decoder itself. Also, with some interesting cases like changing the size of textures, and malformed input data, so we could exercise more of the proxy code.
The browser_tests and content_browsertests failures are due to a bug in VideoDecoderResource. I wasn't taking a ref to the graphics3d context I create. I also fixed a bug in the example. Now we don't reset if we're resetting or flushing.
https://codereview.chromium.org/311853005/diff/70002/content/renderer/pepper/... File content/renderer/pepper/video_decoder_shim.cc (right): https://codereview.chromium.org/311853005/diff/70002/content/renderer/pepper/... content/renderer/pepper/video_decoder_shim.cc:131: true /* low_delay */, On 2014/06/07 18:26:40, Ami GONE FROM CHROMIUM wrote: > On 2014/06/07 05:34:34, igorc wrote: > > Please call set_decode_nalus(true) to allow processing of individual NALU's. > See > > https://codereview.chromium.org/301243008/ that was just committed. > > Igor: the commentary on your new API is that it disables low_delay. Does that > mean this true should turn to false to avoid misleading readers? > Or should it stay true in case the future improves, but with a comment saying it > is ignored by the ffmpeg impl? > Or something else? It could be poor commenting and/or incomplete understanding on my side. See https://codereview.chromium.org/301243008/. Specifically, comments by dalecurtis@: https://codereview.chromium.org/301243008/diff/1/media/filters/ffmpeg_video_d... And later: "You could dovetail this with the existing low delay flag since that already disables frame threading." It's probably better to set low_delay to false. Will ask dalecurtis@ first though.
https://codereview.chromium.org/311853005/diff/70002/content/renderer/pepper/... File content/renderer/pepper/video_decoder_shim.cc (right): https://codereview.chromium.org/311853005/diff/70002/content/renderer/pepper/... content/renderer/pepper/video_decoder_shim.cc:131: true /* low_delay */, On 2014/06/11 20:13:18, igorc wrote: > On 2014/06/07 18:26:40, Ami GONE FROM CHROMIUM wrote: > > On 2014/06/07 05:34:34, igorc wrote: > > > Please call set_decode_nalus(true) to allow processing of individual NALU's. > > See > > > https://codereview.chromium.org/301243008/ that was just committed. > > > > Igor: the commentary on your new API is that it disables low_delay. Does that > > mean this true should turn to false to avoid misleading readers? > > Or should it stay true in case the future improves, but with a comment saying > it > > is ignored by the ffmpeg impl? > > Or something else? > > It could be poor commenting and/or incomplete understanding on my side. See > https://codereview.chromium.org/301243008/. > > Specifically, comments by dalecurtis@: > https://codereview.chromium.org/301243008/diff/1/media/filters/ffmpeg_video_d... > > And later: "You could dovetail this with the existing low delay flag since that > already disables frame threading." > > It's probably better to set low_delay to false. Will ask dalecurtis@ first > though. The comment is incorrect / opposite and should probably be updated. Setting the thread type to FF_THREAD_SLICE is Chrome's definition of "low delay," despite the fact that FFmpeg does expose a CODEC_FLAG_LOW_DELAY that we don't use. Setting CODEC_FLAG2_CHUNKS as set_decode_nalus() does, transitively causes FFmpeg to use FF_THREAD_SLICE instead of FF_THREAD_FRAME which makes it equivalent to Chrome's "low delay" mode.
https://codereview.chromium.org/311853005/diff/70002/content/renderer/pepper/... File content/renderer/pepper/video_decoder_shim.cc (right): https://codereview.chromium.org/311853005/diff/70002/content/renderer/pepper/... content/renderer/pepper/video_decoder_shim.cc:131: true /* low_delay */, On 2014/06/11 21:08:08, DaleCurtis wrote: > On 2014/06/11 20:13:18, igorc wrote: > > On 2014/06/07 18:26:40, Ami GONE FROM CHROMIUM wrote: > > > On 2014/06/07 05:34:34, igorc wrote: > > > > Please call set_decode_nalus(true) to allow processing of individual > NALU's. > > > See > > > > https://codereview.chromium.org/301243008/ that was just committed. > > > > > > Igor: the commentary on your new API is that it disables low_delay. Does > that > > > mean this true should turn to false to avoid misleading readers? > > > Or should it stay true in case the future improves, but with a comment > saying > > it > > > is ignored by the ffmpeg impl? > > > Or something else? > > > > It could be poor commenting and/or incomplete understanding on my side. See > > https://codereview.chromium.org/301243008/. > > > > Specifically, comments by dalecurtis@: > > > https://codereview.chromium.org/301243008/diff/1/media/filters/ffmpeg_video_d... > > > > And later: "You could dovetail this with the existing low delay flag since > that > > already disables frame threading." > > > > It's probably better to set low_delay to false. Will ask dalecurtis@ first > > though. > > The comment is incorrect / opposite and should probably be updated. Setting the > thread type to FF_THREAD_SLICE is Chrome's definition of "low delay," despite > the fact that FFmpeg does expose a CODEC_FLAG_LOW_DELAY that we don't use. > > Setting CODEC_FLAG2_CHUNKS as set_decode_nalus() does, transitively causes > FFmpeg to use FF_THREAD_SLICE instead of FF_THREAD_FRAME which makes it > equivalent to Chrome's "low delay" mode. Just spoke to Dale, and we agreed to delete "Disables low-latency mode" phrase from set_decode_nalus()'s comment, see https://codereview.chromium.org/301243008/patch/40001/50002. The main reasoning is that there're many other parameters that affect "active_thread_type" selection.
On 2014/06/11 22:30:09, igorc wrote: > https://codereview.chromium.org/311853005/diff/70002/content/renderer/pepper/... > File content/renderer/pepper/video_decoder_shim.cc (right): > > https://codereview.chromium.org/311853005/diff/70002/content/renderer/pepper/... > content/renderer/pepper/video_decoder_shim.cc:131: true /* low_delay */, > On 2014/06/11 21:08:08, DaleCurtis wrote: > > On 2014/06/11 20:13:18, igorc wrote: > > > On 2014/06/07 18:26:40, Ami GONE FROM CHROMIUM wrote: > > > > On 2014/06/07 05:34:34, igorc wrote: > > > > > Please call set_decode_nalus(true) to allow processing of individual > > NALU's. > > > > See > > > > > https://codereview.chromium.org/301243008/ that was just committed. > > > > > > > > Igor: the commentary on your new API is that it disables low_delay. Does > > that > > > > mean this true should turn to false to avoid misleading readers? > > > > Or should it stay true in case the future improves, but with a comment > > saying > > > it > > > > is ignored by the ffmpeg impl? > > > > Or something else? > > > > > > It could be poor commenting and/or incomplete understanding on my side. See > > > https://codereview.chromium.org/301243008/. > > > > > > Specifically, comments by dalecurtis@: > > > > > > https://codereview.chromium.org/301243008/diff/1/media/filters/ffmpeg_video_d... > > > > > > And later: "You could dovetail this with the existing low delay flag since > > that > > > already disables frame threading." > > > > > > It's probably better to set low_delay to false. Will ask dalecurtis@ first > > > though. > > > > The comment is incorrect / opposite and should probably be updated. Setting > the > > thread type to FF_THREAD_SLICE is Chrome's definition of "low delay," despite > > the fact that FFmpeg does expose a CODEC_FLAG_LOW_DELAY that we don't use. > > > > Setting CODEC_FLAG2_CHUNKS as set_decode_nalus() does, transitively causes > > FFmpeg to use FF_THREAD_SLICE instead of FF_THREAD_FRAME which makes it > > equivalent to Chrome's "low delay" mode. > > Just spoke to Dale, and we agreed to delete "Disables low-latency mode" phrase > from set_decode_nalus()'s comment, see > https://codereview.chromium.org/301243008/patch/40001/50002. > > The main reasoning is that there're many other parameters that affect > "active_thread_type" selection. Just to make sure then, I should leave the "low_delay" parameter 'true'?
Message was sent while issue was closed.
Committed patchset #17 manually as r277012 (presubmit successful).
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/336783002/ by binjin@chromium.org. The reason for reverting is: break compilation on clang In file included from ../../ppapi/examples/video_decode/video_decode_dev.cc:25: ../../ppapi/examples/video_decode/testdata.h:20388:7:error: unknown type name 'PP_VideoProfile' const PP_VideoProfile kBitstreamProfile = PP_VIDEOPROFILE_H264MAIN; ^ ../../ppapi/examples/video_decode/testdata.h:20388:43:error: use of undeclared identifier 'PP_VIDEOPROFILE_H264MAIN' const PP_VideoProfile kBitstreamProfile = PP_VIDEOPROFILE_H264MAIN;.
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/337683002/ by schenney@chromium.org. The reason for reverting is: Broke blink Linux tests compile.http://build.chromium.org/p/chromium.webkit/builders/Linux%20Tests/builds/37259. |
