Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
820823002:
Implement video renderer based on VideoDecode API. (Closed)
|
Created:
6 years ago by Sergey Ulanov Modified:
5 years, 11 months ago Reviewers:
Wez CC:
chromium-reviews, chromoting-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement video renderer based on PPB_VideoDecode API.
The new renderer is now used by default in the remoting client
plugin. If PPB_Graphics3D interface is not available then the
plugin falls back to the old renderer with the in-plugin
decoder and 2D renderer.
BUG=414923
Committed: https://crrev.com/25f8cb723e93b24f19c27e6c77bbe861ec52cbd9
Cr-Commit-Position: refs/heads/master@{#310387}
Patch Set 1 : #
Total comments: 8
Patch Set 2 : #
Total comments: 101
Patch Set 3 : #
Total comments: 15
Patch Set 4 : #
Messages
Total messages: 16 (5 generated)
Patchset #1 (id:1) has been deleted
sergeyu@chromium.org changed reviewers: + wez@chromium.org
nit: It's the PPAPI VideoDecode or pp::VideoDecode API. :)
On 2014/12/20 03:37:57, Wez wrote: > nit: It's the PPAPI VideoDecode or pp::VideoDecode API. :) Updated description.
https://codereview.chromium.org/820823002/diff/20001/remoting/client/plugin/c... File remoting/client/plugin/chromoting_instance.cc (right): https://codereview.chromium.org/820823002/diff/20001/remoting/client/plugin/c... remoting/client/plugin/chromoting_instance.cc:393: protocol::INCOMPATIBLE_PROTOCOL); This seems a strange error to set in case of decoder failure; it might be failing due to a resource allocation issue or something else not protocol-related, surely? https://codereview.chromium.org/820823002/diff/20001/remoting/client/plugin/c... remoting/client/plugin/chromoting_instance.cc:638: bool initialized = Rather than have this bool, it'd be more readable to reset() the |video_renderer_| based on the result of Initialize() being false (and similarly below) - then the CHECK below becomes CHECK(video_renderer_), which seems more self-documenting. https://codereview.chromium.org/820823002/diff/20001/remoting/client/plugin/c... remoting/client/plugin/chromoting_instance.cc:649: CHECK(initialized); Move this CHECK to after the if() block https://codereview.chromium.org/820823002/diff/20001/remoting/client/plugin/p... File remoting/client/plugin/pepper_video_renderer_3d.h (right): https://codereview.chromium.org/820823002/diff/20001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.h:106: GLint shader_texcoord_scale_location_; nit: Please add a brief block comment to explain how these are used. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/c... File remoting/client/plugin/chromoting_instance.cc (right): https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/c... remoting/client/plugin/chromoting_instance.cc:390: video_renderer_.reset(); These are the same steps we perform in HandleDisconnect() - can we fold them into a common function, or call HandleDisconnect() itself, here, to avoid risking them getting out of sync? https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/c... remoting/client/plugin/chromoting_instance.cc:393: protocol::INCOMPATIBLE_PROTOCOL); nit: Please add a one-line comment to clarify that this is in effect faking a protocol::ConnectionToHost protocol failure signal up to JS. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/c... File remoting/client/plugin/chromoting_instance.h (right): https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/c... remoting/client/plugin/chromoting_instance.h:129: void OnVideoDecoderFailed() override; nit: Suggest OnVideoRenderError, OnVideoDecodeError or OnVideoDecodeFailed. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... File remoting/client/plugin/pepper_video_renderer.h (right): https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer.h:33: // Called when decoder has failed for any reason. nit: "Called if video decoding fails, for any reason." https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... File remoting/client/plugin/pepper_video_renderer_3d.cc (right): https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:86: PP_GRAPHICS3DATTRIB_HEIGHT, 1, Are you sure that 1x1 is always supported by Graphics3D? e.g. do some GLES impls have issues with particular context sizes, perhaps? https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:95: if (!instance->BindGraphics(graphics_)) { Do we need to tear-down and re-create/re-bind things when the size of the view changes? https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:156: NOTREACHED(); This means we're defaulting to VP8_ANY; consider triggering the OnVideoDecodeFailed notification directly in this case, instead, since there is no always-bad PP_VideoProfile value that we can set here. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:162: << "video_decoder_.Initialize() returned " << result; Strictly we should be coping with not-supported as a return-value here; probably worth adding a comment to clarify that we assume VP8 & VP9 are not going away any time soon. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:198: if ((!frame_size.is_empty() && !frame_size_.equals(frame_size)) || Feels like |frame_size.is_empty()| is a different special-case to the others; should it even be a case that we early-return, or even CHECK() for? https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:206: if (packet->has_use_desktop_shape()) { nit: You also need to check that use_desktop_shape, if set, is actually set to True. However, if it's not set then its default value will be false, so I think "if (packet->use_desktop_shape())" should suffice. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:216: webrtc::DesktopRegion(webrtc::DesktopRect::MakeSize(frame_size)); nit: This means that unless the remote peer sends the desktop shape with every VideoPacket then the shape will snap back to being rectangular, which isn't really how the shape field was intended to be used, though I see that that's what we also have implemented in the VPX decoder. :-/ https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:230: CHECK_EQ(result, PP_OK) << "VideoDecoder::Initialize() failed"; Cope gracefully with initialization errors here? https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:232: DoDecode(); nit: Add a brief comment to clarify why we start the decode process here. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:235: void PepperVideoRenderer3D::DoGetPicture() { Move this method down to appear immediately before OnPictureReady, to aid readability? https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:242: CHECK_EQ(result, PP_OK_COMPLETIONPENDING); nit: Cope gracefully w/ errors here? https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:252: FrameDecodeTimer(last_frame_id_, base::TimeTicks::Now())); Since you can only ever have a single decode operation in-progress at any one time, and https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:257: last_frame_id_, packet->data().size(), packet->data().data(), nit: The Decode() API unfortunately doesn't specify whether it copies the supplied data before returning; I assume that in general it's best to assume that it does not, but in that case the API docs should be updated to clarify that point. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:259: CHECK_EQ(result, PP_OK_COMPLETIONPENDING); The docs on GetPicture imply that Decode() could return PP_OK; this should probably be "proper" error handling where we signal OnVideoDecodeError rather than crashing. :) https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:264: decode_pending_ = false; nit: DCHECK(decode_pending_)? https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:281: get_picture_pending_ = false; nit: DCHECK(get_picture_pending_) https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:283: if (result == PP_ERROR_ABORTED) What does PP_ERROR_ABORTED actually mean? Should we be throwing the error notification in that case as well? If this is only generated when we ask the API to cancel decoding then perhaps add a brief comment to explain that here. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:292: CHECK(!frames_decode_timers_.empty()); Do you need this check? If it's violated then front() will return a not-dereferencable value and crash us, won't it? https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:294: DCHECK_EQ(picture.decode_id, frame_timer.frame_id); This check will fail if we receive a packet that does not result in a frame, since we'll get out-of-sync with the |frame_decode_timers_| queue - in that case we'll crash in a debug build and return bogus numbers in release builds. I think it'd be better to always return some obviously-bogus value in this case, and to log something obvious since that's not a behaviour we expect streams generated by current hosts to trigger. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:296: (base::TimeTicks::Now() - frame_timer.decode_started_time) nit: Suggest calculating this into a const base::TimeTicks local variable, to aid readability. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:310: void PepperVideoRenderer3D::RecyclePictures(bool keep_first) { It looks like this only keeps the first if |keep_first| is true, which isn't what the comment on the picture queue member says, in the header - that comment implies that we always keep the most recent picture ready so that we can re-paint it if need be? https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:313: ++it; This is keeping the first, i.e. the _next_ picture, rather than the most-recently-queued one - is that what you intended? https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:322: if (paint_pending_ || !need_repaint_ || pending_pictures_.empty()) This logic means that if |need_repaint_| is false then we'll early-exit without doing anything, even if |pending_pictures_| contains more content to paint - is that what you intend? It seems to me that it is, because you always keep the frontmost frame in the queue when you paint, but that approach seems to complicate matters; consider keeping just two PP_VideoPicture members in this class, one to hold the currently-being-painted picture, and one to hold the next one to paint, if any - I think that will make some of this logic clearer, and you can then do away with |needs_repaint_|. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:362: // Request PPAPI display the queue texture. typo: queued https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:374: (base::TimeTicks::Now() - last_paint_started_time_).InMilliseconds()); nit: See above; consider doing the subtraction into a suitably named local variable, for clarity. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:437: CHECK(false); Suggest NOTREACHED() or LOG(FATAL) https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:491: } nit: Blank line https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... File remoting/client/plugin/pepper_video_renderer_3d.h (right): https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.h:17: #include "ppapi/lib/gl/include/GLES2/gl2.h" nit: It's really unfortunate to have to pollute the calling code's namespace with a load of PPAPI stuff, especially GL. Easiest way to avoid that would be to hide the implementation details of the 3D renderer behind a Create() function, so that all these includes end up in the cc file. WDYT? https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.h:28: // PepperVideoRenderer that uses PPB_VideoDecoder interface for video decoding nit: "... uses the PPB_VideoDecoder interface for ..." or "... uses pp:VideoDecode for ..." https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.h:32: explicit PepperVideoRenderer3D(); nit: No need for explicit here. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.h:48: struct FrameDecodeTimer { nit: This isn't a timer in the base::Timer sense, but rather a record of the decode start time, so perhaps call it FrameDecodeTimestamp? https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.h:61: void OnPaintDone(int32_t result); nit: Break the members above into groups of logically related functions, e.g. those involved in receiving a picture & painting it, those involved in doing decoding, etc, with one-liner comments on each block? It's not clear from the names what DoDecode(), DoGetPicture() or DoPaint()do, in particular why they need "Do" in front - why is DoDecode() not DecodePacket(), DoGetPicture() not simply Get[Next]Picture(), etc? https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.h:66: void CheckGLError(); nit: Suggest one-line comments be added for each of these methods, e.g. to clarify that CreateProgram() creates a render program in |shader_program_|, that includes the necessary shader programs. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.h:79: int view_height_; nit: Why not store these in a DesktopSize, for consistency? https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.h:84: bool initialized_; nit: Could you derive |initialized_| from something higher level like |!graphics_.is_null()| and have an initialized() getter rather than a member variable? https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.h:89: uint32_t last_frame_id_; nit: Be consistent "last" here versus "latest" above. Why do we need both a sequence number _and_ a frame-Id? Could we get away with just one of the two? https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.h:97: std::deque<FrameDecodeTimer> frames_decode_timers_; nit: frame_decode_timers_ Or, since the things being decoded are packets, not frames, consider packet_decode_timers_. Given that you can only have one decode in-progress at any given time, I wonder if you can dispense with the queue and instead embed the decode start-time as a callback parameter, e.g. use NewCallback(...::OnDecodeDone, start_time) etc - then you just store the decode time with the PP_VideoPicture when you see OnPictureReady (see comments elsewhere re dispensing with the picture queue in favour of a pair of PP_VideoPicture members). https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.h:100: // frame is always to kept in this vector, in case it needs to be repainted. Why might it contain two frames, rather than containing exactly one? https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.h:103: // Set to true if the screen needs to be repainted. nit: Clarify why that would be the case, e.g. due to View resize? https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.h:107: base::TimeTicks last_paint_started_time_; nit: See last/latest comment, above.
Patchset #3 (id:60001) has been deleted
https://codereview.chromium.org/820823002/diff/20001/remoting/client/plugin/c... File remoting/client/plugin/chromoting_instance.cc (right): https://codereview.chromium.org/820823002/diff/20001/remoting/client/plugin/c... remoting/client/plugin/chromoting_instance.cc:393: protocol::INCOMPATIBLE_PROTOCOL); On 2014/12/24 17:11:13, Wez wrote: > This seems a strange error to set in case of decoder failure; it might be > failing due to a resource allocation issue or something else not > protocol-related, surely? Added TODO to fix it. https://codereview.chromium.org/820823002/diff/20001/remoting/client/plugin/c... remoting/client/plugin/chromoting_instance.cc:638: bool initialized = On 2014/12/24 17:11:13, Wez wrote: > Rather than have this bool, it'd be more readable to reset() the > |video_renderer_| based on the result of Initialize() being false (and similarly > below) - then the CHECK below becomes CHECK(video_renderer_), which seems more > self-documenting. Done. https://codereview.chromium.org/820823002/diff/20001/remoting/client/plugin/c... remoting/client/plugin/chromoting_instance.cc:649: CHECK(initialized); On 2014/12/24 17:11:13, Wez wrote: > Move this CHECK to after the if() block Done. https://codereview.chromium.org/820823002/diff/20001/remoting/client/plugin/p... File remoting/client/plugin/pepper_video_renderer_3d.h (right): https://codereview.chromium.org/820823002/diff/20001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.h:106: GLint shader_texcoord_scale_location_; On 2014/12/24 17:11:13, Wez wrote: > nit: Please add a brief block comment to explain how these are used. Done. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/c... File remoting/client/plugin/chromoting_instance.cc (right): https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/c... remoting/client/plugin/chromoting_instance.cc:390: video_renderer_.reset(); On 2014/12/24 17:11:13, Wez wrote: > These are the same steps we perform in HandleDisconnect() - can we fold them > into a common function, or call HandleDisconnect() itself, here, to avoid > risking them getting out of sync? Done. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/c... remoting/client/plugin/chromoting_instance.cc:393: protocol::INCOMPATIBLE_PROTOCOL); On 2014/12/24 17:11:13, Wez wrote: > nit: Please add a one-line comment to clarify that this is in effect faking a > protocol::ConnectionToHost protocol failure signal up to JS. Done. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/c... File remoting/client/plugin/chromoting_instance.h (right): https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/c... remoting/client/plugin/chromoting_instance.h:129: void OnVideoDecoderFailed() override; On 2014/12/24 17:11:14, Wez wrote: > nit: Suggest OnVideoRenderError, OnVideoDecodeError or OnVideoDecodeFailed. Renamed to OnVideoDecodeError() https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... File remoting/client/plugin/pepper_video_renderer.h (right): https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer.h:33: // Called when decoder has failed for any reason. On 2014/12/24 17:11:14, Wez wrote: > nit: "Called if video decoding fails, for any reason." Done. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... File remoting/client/plugin/pepper_video_renderer_3d.cc (right): https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:86: PP_GRAPHICS3DATTRIB_HEIGHT, 1, On 2014/12/24 17:11:15, Wez wrote: > Are you sure that 1x1 is always supported by Graphics3D? e.g. do some GLES impls > have issues with particular context sizes, perhaps? changed this to 640x480 https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:95: if (!instance->BindGraphics(graphics_)) { On 2014/12/24 17:11:14, Wez wrote: > Do we need to tear-down and re-create/re-bind things when the size of the view > changes? No. Graphics3D.ResizeBuffers() allows to change the size dynamically. (if it didn't it would be be big PITA to handle resizes, as we would have to reinitialize the decoder somehow with the new context). https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:156: NOTREACHED(); On 2014/12/24 17:11:15, Wez wrote: > This means we're defaulting to VP8_ANY; consider triggering the > OnVideoDecodeFailed notification directly in this case, instead, since there is > no always-bad PP_VideoProfile value that we can set here. Protocol should not allow any codec other than VP8 or VP9, so this case can only be reached if there is a bug in the handshake code. So I think NOTREACHED() is appropriate here. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:162: << "video_decoder_.Initialize() returned " << result; On 2014/12/24 17:11:14, Wez wrote: > Strictly we should be coping with not-supported as a return-value here; probably > worth adding a comment to clarify that we assume VP8 & VP9 are not going away > any time soon. We always expect PP_OK_COMPLETIONPENDING here because the callback is not optional. Non-supported codec would need to be handled in OnInitialized(). Added a comment there. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:198: if ((!frame_size.is_empty() && !frame_size_.equals(frame_size)) || On 2014/12/24 17:11:14, Wez wrote: > Feels like |frame_size.is_empty()| is a different special-case to the others; > should it even be a case that we early-return, or even CHECK() for? Done. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:206: if (packet->has_use_desktop_shape()) { On 2014/12/24 17:11:14, Wez wrote: > nit: You also need to check that use_desktop_shape, if set, is actually set to > True. However, if it's not set then its default value will be false, so I think > "if (packet->use_desktop_shape())" should suffice. This code is equivalent to what we have in VideoDecoderVpx::DecodePacket(). I think it's better to fix it in both places in a separate CL. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:216: webrtc::DesktopRegion(webrtc::DesktopRect::MakeSize(frame_size)); On 2014/12/24 17:11:15, Wez wrote: > nit: This means that unless the remote peer sends the desktop shape with every > VideoPacket then the shape will snap back to being rectangular, which isn't > really how the shape field was intended to be used, though I see that that's > what we also have implemented in the VPX decoder. :-/ Exactly. It doesn't look right, but it's not related to this CL. I've opened crbug.com/446288 to address it. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:230: CHECK_EQ(result, PP_OK) << "VideoDecoder::Initialize() failed"; On 2014/12/24 17:11:14, Wez wrote: > Cope gracefully with initialization errors here? I think we can assume that we can always initialize the decoder, because VP8 and VP9 should always be supported. Added a comment https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:232: DoDecode(); On 2014/12/24 17:11:15, Wez wrote: > nit: Add a brief comment to clarify why we start the decode process here. Done. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:235: void PepperVideoRenderer3D::DoGetPicture() { On 2014/12/24 17:11:14, Wez wrote: > Move this method down to appear immediately before OnPictureReady, to aid > readability? Done. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:242: CHECK_EQ(result, PP_OK_COMPLETIONPENDING); On 2014/12/24 17:11:14, Wez wrote: > nit: Cope gracefully w/ errors here? We always expect PP_OK_COMPLETIONPENDING here because the callback is not optional. Getting anything else here would indicated a bug in the browser. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:252: FrameDecodeTimer(last_frame_id_, base::TimeTicks::Now())); On 2014/12/24 17:11:14, Wez wrote: > Since you can only ever have a single decode operation in-progress at any one > time, and looks like this comment was cropped somehow. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:257: last_frame_id_, packet->data().size(), packet->data().data(), On 2014/12/24 17:11:14, Wez wrote: > nit: The Decode() API unfortunately doesn't specify whether it copies the > supplied data before returning; I assume that in general it's best to assume > that it does not, but in that case the API docs should be updated to clarify > that point. It does copy the data and it's documented: https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/cpp/video_de... We keep the packet until OnDecodeDone() is called. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:259: CHECK_EQ(result, PP_OK_COMPLETIONPENDING); On 2014/12/24 17:11:14, Wez wrote: > The docs on GetPicture imply that Decode() could return PP_OK; this should > probably be "proper" error handling where we signal OnVideoDecodeError rather > than crashing. :) The callback is not optional, so we always expect PP_OK_COMPLETIONPENDING here and OnDecodeDone() to be called asynchronously https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:264: decode_pending_ = false; On 2014/12/24 17:11:14, Wez wrote: > nit: DCHECK(decode_pending_)? Done. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:281: get_picture_pending_ = false; On 2014/12/24 17:11:14, Wez wrote: > nit: DCHECK(get_picture_pending_) Done. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:283: if (result == PP_ERROR_ABORTED) On 2014/12/24 17:11:15, Wez wrote: > What does PP_ERROR_ABORTED actually mean? Should we be throwing the error > notification in that case as well? If this is only generated when we ask the API > to cancel decoding then perhaps add a brief comment to explain that here. GetPicture() may be aborted when the pp::VideoDecoder::Reset() is called, but we never need to reset the decoder, so I've removed this check. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:292: CHECK(!frames_decode_timers_.empty()); On 2014/12/24 17:11:15, Wez wrote: > Do you need this check? If it's violated then front() will return a > not-dereferencable value and crash us, won't it? front() behavior is undefined when the list is empty, so there is no guarantee that it will crash below in that case. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:294: DCHECK_EQ(picture.decode_id, frame_timer.frame_id); On 2014/12/24 17:11:15, Wez wrote: > This check will fail if we receive a packet that does not result in a frame, > since we'll get out-of-sync with the |frame_decode_timers_| queue - in that case > we'll crash in a debug build and return bogus numbers in release builds. I think > it'd be better to always return some obviously-bogus value in this case, and to > log something obvious since that's not a behaviour we expect streams generated > by current hosts to trigger. If picture.decode_id != frame_timer.frame_id it means that the buffer that we passed to Decode() didn't contain full frame. So this should be treated as a protocol error. Replaced this DCHECK with a OnVideoDecodeError() call. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:296: (base::TimeTicks::Now() - frame_timer.decode_started_time) On 2014/12/24 17:11:14, Wez wrote: > nit: Suggest calculating this into a const base::TimeTicks local variable, to > aid readability. Done. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:310: void PepperVideoRenderer3D::RecyclePictures(bool keep_first) { On 2014/12/24 17:11:15, Wez wrote: > It looks like this only keeps the first if |keep_first| is true, which isn't > what the comment on the picture queue member says, in the header - that comment > implies that we always keep the most recent picture ready so that we can > re-paint it if need be? Removed this method. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:313: ++it; On 2014/12/24 17:11:15, Wez wrote: > This is keeping the first, i.e. the _next_ picture, rather than the > most-recently-queued one - is that what you intended? Yes. The first picture is the one being rendered, and we want to keep it until rendering is finished. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:322: if (paint_pending_ || !need_repaint_ || pending_pictures_.empty()) On 2014/12/24 17:11:15, Wez wrote: > This logic means that if |need_repaint_| is false then we'll early-exit without > doing anything, even if |pending_pictures_| contains more content to paint - is > that what you intend? After the first frame is received pending_pictures_ is never empty. We always keep one frame in case it needs to be repainted when resizing. > > It seems to me that it is, because you always keep the frontmost frame in the > queue when you paint, but that approach seems to complicate matters; consider > keeping just two PP_VideoPicture members in this class, one to hold the > currently-being-painted picture, and one to hold the next one to paint, if any - > I think that will make some of this logic clearer, and you can then do away with > |needs_repaint_|. Removed |pending_pictures_| queue, but I think we still need |need_repaint_| for the case when view size changes while paint is pending and there is no another frame in the queue. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:362: // Request PPAPI display the queue texture. On 2014/12/24 17:11:15, Wez wrote: > typo: queued Done. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:374: (base::TimeTicks::Now() - last_paint_started_time_).InMilliseconds()); On 2014/12/24 17:11:14, Wez wrote: > nit: See above; consider doing the subtraction into a suitably named local > variable, for clarity. Done. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:437: CHECK(false); On 2014/12/24 17:11:15, Wez wrote: > Suggest NOTREACHED() or LOG(FATAL) Done. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:491: } On 2014/12/24 17:11:15, Wez wrote: > nit: Blank line Done. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... File remoting/client/plugin/pepper_video_renderer_3d.h (right): https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.h:17: #include "ppapi/lib/gl/include/GLES2/gl2.h" On 2014/12/24 17:11:15, Wez wrote: > nit: It's really unfortunate to have to pollute the calling code's namespace > with a load of PPAPI stuff, especially GL. Easiest way to avoid that would be to > hide the implementation details of the 3D renderer behind a Create() function, > so that all these includes end up in the cc file. WDYT? This header was necessary only for GL[u]int types. Removed this #include and replaced GL types with unsigned int and int. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.h:28: // PepperVideoRenderer that uses PPB_VideoDecoder interface for video decoding On 2014/12/24 17:11:15, Wez wrote: > nit: "... uses the PPB_VideoDecoder interface for ..." or "... uses > pp:VideoDecode for ..." Done. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.h:32: explicit PepperVideoRenderer3D(); On 2014/12/24 17:11:15, Wez wrote: > nit: No need for explicit here. Done. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.h:48: struct FrameDecodeTimer { On 2014/12/24 17:11:15, Wez wrote: > nit: This isn't a timer in the base::Timer sense, but rather a record of the > decode start time, so perhaps call it FrameDecodeTimestamp? Done. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.h:61: void OnPaintDone(int32_t result); On 2014/12/24 17:11:16, Wez wrote: > nit: Break the members above into groups of logically related functions, e.g. > those involved in receiving a picture & painting it, those involved in doing > decoding, etc, with one-liner comments on each block? Done. > It's not clear from the names what DoDecode(), DoGetPicture() or DoPaint()do, in > particular why they need "Do" in front - why is DoDecode() not DecodePacket(), > DoGetPicture() not simply Get[Next]Picture(), etc? Renamed all Do*() methods. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.h:66: void CheckGLError(); On 2014/12/24 17:11:16, Wez wrote: > nit: Suggest one-line comments be added for each of these methods, e.g. to > clarify that CreateProgram() creates a render program in |shader_program_|, that > includes the necessary shader programs. Done. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.h:79: int view_height_; On 2014/12/24 17:11:16, Wez wrote: > nit: Why not store these in a DesktopSize, for consistency? Done. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.h:84: bool initialized_; On 2014/12/24 17:11:15, Wez wrote: > nit: Could you derive |initialized_| from something higher level like > |!graphics_.is_null()| and have an initialized() getter rather than a member > variable? No. VideoDecoder initialization is asynchronous and we want to hold off calling Decode() until initialization is finished - see DoDecode(). Renamed to initialization_finished_ https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.h:89: uint32_t last_frame_id_; On 2014/12/24 17:11:15, Wez wrote: > nit: Be consistent "last" here versus "latest" above. > > Why do we need both a sequence number _and_ a frame-Id? Could we get away with > just one of the two? client_sequence_number is a misnomer - it's actually an input event timestamp we send to the host with input events. It may not change between consecutive frames. frame_id is used to identify the frames we pass to pp::VideoDecoder, and we want it to be unique for each frame. Renamed latest_sequence_number_to last_input_event_timestamp_ in this CL. Will rename it on the protocol level in a separate CL. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.h:97: std::deque<FrameDecodeTimer> frames_decode_timers_; On 2014/12/24 17:11:16, Wez wrote: > nit: frame_decode_timers_ > > Or, since the things being decoded are packets, not frames, consider > packet_decode_timers_. > > Given that you can only have one decode in-progress at any given time, I wonder > if you can dispense with the queue and instead embed the decode start-time as a > callback parameter, e.g. use NewCallback(...::OnDecodeDone, start_time) etc - > then you just store the decode time with the PP_VideoPicture when you see > OnPictureReady (see comments elsewhere re dispensing with the picture queue in > favour of a pair of PP_VideoPicture members). Decoding a frame is a two-step process. First we call Decode() and then GetPicture() to receive the result. Decoder is allowed to decode multiple frames in parallel, so there may be multiple frames in the queue and we want to keep timestamp for each of them. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.h:100: // frame is always to kept in this vector, in case it needs to be repainted. On 2014/12/24 17:11:15, Wez wrote: > Why might it contain two frames, rather than containing exactly one? If a previous frame is still being rendered we want to keep it until rendering is finished even if another one has been received. Replaced it with two variables |current_frame_| and |next_frame_| https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.h:103: // Set to true if the screen needs to be repainted. On 2014/12/24 17:11:15, Wez wrote: > nit: Clarify why that would be the case, e.g. due to View resize? Done. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.h:107: base::TimeTicks last_paint_started_time_; On 2014/12/24 17:11:15, Wez wrote: > nit: See last/latest comment, above. Done.
https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... File remoting/client/plugin/pepper_video_renderer_3d.cc (right): https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:95: if (!instance->BindGraphics(graphics_)) { On 2015/01/06 01:16:32, Sergey Ulanov wrote: > On 2014/12/24 17:11:14, Wez wrote: > > Do we need to tear-down and re-create/re-bind things when the size of the view > > changes? > > No. Graphics3D.ResizeBuffers() allows to change the size dynamically. (if it > didn't it would be be big PITA to handle resizes, as we would have to > reinitialize the decoder somehow with the new context). Agreed! I vaguely recalled that the example seemed to do that, though. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:206: if (packet->has_use_desktop_shape()) { On 2015/01/06 01:16:32, Sergey Ulanov wrote: > On 2014/12/24 17:11:14, Wez wrote: > > nit: You also need to check that use_desktop_shape, if set, is actually set to > > True. However, if it's not set then its default value will be false, so I > think > > "if (packet->use_desktop_shape())" should suffice. > > This code is equivalent to what we have in VideoDecoderVpx::DecodePacket(). I > think it's better to fix it in both places in a separate CL. Acknowledged. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:216: webrtc::DesktopRegion(webrtc::DesktopRect::MakeSize(frame_size)); On 2015/01/06 01:16:32, Sergey Ulanov wrote: > On 2014/12/24 17:11:15, Wez wrote: > > nit: This means that unless the remote peer sends the desktop shape with every > > VideoPacket then the shape will snap back to being rectangular, which isn't > > really how the shape field was intended to be used, though I see that that's > > what we also have implemented in the VPX decoder. :-/ > > Exactly. It doesn't look right, but it's not related to this CL. I've opened > crbug.com/446288 to address it. Acknowledged. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:230: CHECK_EQ(result, PP_OK) << "VideoDecoder::Initialize() failed"; On 2015/01/06 01:16:32, Sergey Ulanov wrote: > On 2014/12/24 17:11:14, Wez wrote: > > Cope gracefully with initialization errors here? > > I think we can assume that we can always initialize the decoder, because VP8 and > VP9 should always be supported. Added a comment Acknowledged. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:257: last_frame_id_, packet->data().size(), packet->data().data(), On 2015/01/06 01:16:32, Sergey Ulanov wrote: > On 2014/12/24 17:11:14, Wez wrote: > > nit: The Decode() API unfortunately doesn't specify whether it copies the > > supplied data before returning; I assume that in general it's best to assume > > that it does not, but in that case the API docs should be updated to clarify > > that point. > > It does copy the data and it's documented: > https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/cpp/video_de... > We keep the packet until OnDecodeDone() is called. If it copies the data then we don't need to keep the packet around until OnDecodeDone(), though, was my point. :) https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:259: CHECK_EQ(result, PP_OK_COMPLETIONPENDING); On 2015/01/06 01:16:32, Sergey Ulanov wrote: > On 2014/12/24 17:11:14, Wez wrote: > > The docs on GetPicture imply that Decode() could return PP_OK; this should > > probably be "proper" error handling where we signal OnVideoDecodeError rather > > than crashing. :) > > The callback is not optional, so we always expect PP_OK_COMPLETIONPENDING here > and OnDecodeDone() to be called asynchronously Acknowledged. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:283: if (result == PP_ERROR_ABORTED) On 2015/01/06 01:16:33, Sergey Ulanov wrote: > On 2014/12/24 17:11:15, Wez wrote: > > What does PP_ERROR_ABORTED actually mean? Should we be throwing the error > > notification in that case as well? If this is only generated when we ask the > API > > to cancel decoding then perhaps add a brief comment to explain that here. > > GetPicture() may be aborted when the pp::VideoDecoder::Reset() is called, but we > never need to reset the decoder, so I've removed this check. Acknowledged. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:322: if (paint_pending_ || !need_repaint_ || pending_pictures_.empty()) On 2015/01/06 01:16:32, Sergey Ulanov wrote: > On 2014/12/24 17:11:15, Wez wrote: > > This logic means that if |need_repaint_| is false then we'll early-exit > without > > doing anything, even if |pending_pictures_| contains more content to paint - > is > > that what you intend? > > After the first frame is received pending_pictures_ is never empty. We always > keep one > frame in case it needs to be repainted when resizing. > > > > > It seems to me that it is, because you always keep the frontmost frame in the > > queue when you paint, but that approach seems to complicate matters; consider > > keeping just two PP_VideoPicture members in this class, one to hold the > > currently-being-painted picture, and one to hold the next one to paint, if any > - > > I think that will make some of this logic clearer, and you can then do away > with > > |needs_repaint_|. > > Removed |pending_pictures_| queue, but I think we still need |need_repaint_| for > the case when view size changes while paint is pending and there is no another > frame in the queue. Yes, although you could make it an explicit was_resized_ flag, or a force_repaint_ flag, so that PaintIfNeeded(0 repaints if there is a new frame OR if force_repaint_ is set - then there's no need for a new picture to set needs_repaint_. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... File remoting/client/plugin/pepper_video_renderer_3d.h (right): https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.h:89: uint32_t last_frame_id_; On 2015/01/06 01:16:34, Sergey Ulanov wrote: > On 2014/12/24 17:11:15, Wez wrote: > > nit: Be consistent "last" here versus "latest" above. > > > > Why do we need both a sequence number _and_ a frame-Id? Could we get away with > > just one of the two? > > client_sequence_number is a misnomer - it's actually an input event timestamp we > send to the host with input events. It may not change between consecutive > frames. frame_id is used to identify the frames we pass to pp::VideoDecoder, and > we want it to be unique for each frame. > > Renamed latest_sequence_number_to last_input_event_timestamp_ in this CL. Will > rename it on the protocol level in a separate CL. nit: Prefer "latest" to "last" since it's more obvious that it means "most recent" versus "final". https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.h:97: std::deque<FrameDecodeTimer> frames_decode_timers_; On 2015/01/06 01:16:34, Sergey Ulanov wrote: > On 2014/12/24 17:11:16, Wez wrote: > > nit: frame_decode_timers_ > > > > Or, since the things being decoded are packets, not frames, consider > > packet_decode_timers_. > > > > Given that you can only have one decode in-progress at any given time, I > wonder > > if you can dispense with the queue and instead embed the decode start-time as > a > > callback parameter, e.g. use NewCallback(...::OnDecodeDone, start_time) etc - > > then you just store the decode time with the PP_VideoPicture when you see > > OnPictureReady (see comments elsewhere re dispensing with the picture queue in > > favour of a pair of PP_VideoPicture members). > > Decoding a frame is a two-step process. First we call Decode() and then > GetPicture() to receive the result. Decoder is allowed to decode multiple frames > in parallel, so there may be multiple frames in the queue and we want to keep > timestamp for each of them. Right, I was thinking we could embed the timestamp in the completion callbacks to the decode and get-picture processes, but of without the queue we can't tally the received picture with the decoded packet, so I suppose that won't really work. Seems like it should since they're 1:1 but nevermind... https://codereview.chromium.org/820823002/diff/80001/remoting/client/plugin/c... File remoting/client/plugin/chromoting_instance.cc (right): https://codereview.chromium.org/820823002/diff/80001/remoting/client/plugin/c... remoting/client/plugin/chromoting_instance.cc:641: video_renderer_.reset(); nit: This would be cleaner if you replaced the new+Initialize with FooVideoRenderer::Create() methods. https://codereview.chromium.org/820823002/diff/80001/remoting/client/plugin/c... File remoting/client/plugin/chromoting_instance.h (right): https://codereview.chromium.org/820823002/diff/80001/remoting/client/plugin/c... remoting/client/plugin/chromoting_instance.h:196: // Disconnects the client. This comment doesn't really add anything over the method name; consider removing the comment or, preferably, explaining when it gets called? https://codereview.chromium.org/820823002/diff/80001/remoting/client/plugin/p... File remoting/client/plugin/pepper_video_renderer_3d.cc (right): https://codereview.chromium.org/820823002/diff/80001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:259: // Start decoding in case a frame was received during decoded initialization. s/decoded/decoder https://codereview.chromium.org/820823002/diff/80001/remoting/client/plugin/p... File remoting/client/plugin/pepper_video_renderer_3d.h (right): https://codereview.chromium.org/820823002/diff/80001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.h:73: void Paint(); Suggest renaming to PaintIfNeeded(). https://codereview.chromium.org/820823002/diff/80001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.h:124: scoped_ptr<Picture> next_picture_; nit: Might be worth noting that this and current_picture_ must be deleted _before_ video_decoder_. https://codereview.chromium.org/820823002/diff/80001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.h:127: // received of the plugin was resized. s/of/or https://codereview.chromium.org/820823002/diff/80001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.h:134: // 0, GL_TEXTURE_2D, GL_TEXTURE_RECTANGLE_ARB or GL_TEXTURE_EXTERNAL_OES. What does a value of 0 mean, though?
https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... File remoting/client/plugin/pepper_video_renderer_3d.cc (right): https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:257: last_frame_id_, packet->data().size(), packet->data().data(), On 2015/01/06 22:17:09, Wez wrote: > On 2015/01/06 01:16:32, Sergey Ulanov wrote: > > On 2014/12/24 17:11:14, Wez wrote: > > > nit: The Decode() API unfortunately doesn't specify whether it copies the > > > supplied data before returning; I assume that in general it's best to assume > > > that it does not, but in that case the API docs should be updated to clarify > > > that point. > > > > It does copy the data and it's documented: > > > https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/cpp/video_de... > > We keep the packet until OnDecodeDone() is called. > > If it copies the data then we don't need to keep the packet around until > OnDecodeDone(), though, was my point. :) Ah, I see. Yes, it's not clear. I think it's better assume that it copies the data before completion, e.g. before the callback is called, but not necessarily before returning. https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:322: if (paint_pending_ || !need_repaint_ || pending_pictures_.empty()) On 2015/01/06 22:17:10, Wez wrote: > On 2015/01/06 01:16:32, Sergey Ulanov wrote: > > On 2014/12/24 17:11:15, Wez wrote: > > > This logic means that if |need_repaint_| is false then we'll early-exit > > without > > > doing anything, even if |pending_pictures_| contains more content to paint - > > is > > > that what you intend? > > > > After the first frame is received pending_pictures_ is never empty. We always > > keep one > > frame in case it needs to be repainted when resizing. > > > > > > > > It seems to me that it is, because you always keep the frontmost frame in > the > > > queue when you paint, but that approach seems to complicate matters; > consider > > > keeping just two PP_VideoPicture members in this class, one to hold the > > > currently-being-painted picture, and one to hold the next one to paint, if > any > > - > > > I think that will make some of this logic clearer, and you can then do away > > with > > > |needs_repaint_|. > > > > Removed |pending_pictures_| queue, but I think we still need |need_repaint_| > for > > the case when view size changes while paint is pending and there is no another > > frame in the queue. > > Yes, although you could make it an explicit was_resized_ flag, or a > force_repaint_ flag, so that PaintIfNeeded(0 repaints if there is a new frame OR > if force_repaint_ is set - then there's no need for a new picture to set > needs_repaint_. Replaced need_repaint_ with force_repaint_ https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... File remoting/client/plugin/pepper_video_renderer_3d.h (right): https://codereview.chromium.org/820823002/diff/40001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.h:89: uint32_t last_frame_id_; On 2015/01/06 22:17:10, Wez wrote: > On 2015/01/06 01:16:34, Sergey Ulanov wrote: > > On 2014/12/24 17:11:15, Wez wrote: > > > nit: Be consistent "last" here versus "latest" above. > > > > > > Why do we need both a sequence number _and_ a frame-Id? Could we get away > with > > > just one of the two? > > > > client_sequence_number is a misnomer - it's actually an input event timestamp > we > > send to the host with input events. It may not change between consecutive > > frames. frame_id is used to identify the frames we pass to pp::VideoDecoder, > and > > we want it to be unique for each frame. > > > > Renamed latest_sequence_number_to last_input_event_timestamp_ in this CL. Will > > rename it on the protocol level in a separate CL. > > nit: Prefer "latest" to "last" since it's more obvious that it means "most > recent" versus "final". Done. https://codereview.chromium.org/820823002/diff/80001/remoting/client/plugin/c... File remoting/client/plugin/chromoting_instance.cc (right): https://codereview.chromium.org/820823002/diff/80001/remoting/client/plugin/c... remoting/client/plugin/chromoting_instance.cc:641: video_renderer_.reset(); On 2015/01/06 22:17:10, Wez wrote: > nit: This would be cleaner if you replaced the new+Initialize with > FooVideoRenderer::Create() methods. Yes, but given that this is the only place were the renderer needs to be initialized I don't think that adding the Create() function is worth it. https://codereview.chromium.org/820823002/diff/80001/remoting/client/plugin/c... File remoting/client/plugin/chromoting_instance.h (right): https://codereview.chromium.org/820823002/diff/80001/remoting/client/plugin/c... remoting/client/plugin/chromoting_instance.h:196: // Disconnects the client. On 2015/01/06 22:17:10, Wez wrote: > This comment doesn't really add anything over the method name; consider removing > the comment or, preferably, explaining when it gets called? Done. https://codereview.chromium.org/820823002/diff/80001/remoting/client/plugin/p... File remoting/client/plugin/pepper_video_renderer_3d.cc (right): https://codereview.chromium.org/820823002/diff/80001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.cc:259: // Start decoding in case a frame was received during decoded initialization. On 2015/01/06 22:17:10, Wez wrote: > s/decoded/decoder Done. https://codereview.chromium.org/820823002/diff/80001/remoting/client/plugin/p... File remoting/client/plugin/pepper_video_renderer_3d.h (right): https://codereview.chromium.org/820823002/diff/80001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.h:73: void Paint(); On 2015/01/06 22:17:10, Wez wrote: > Suggest renaming to PaintIfNeeded(). Done. https://codereview.chromium.org/820823002/diff/80001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.h:124: scoped_ptr<Picture> next_picture_; On 2015/01/06 22:17:10, Wez wrote: > nit: Might be worth noting that this and current_picture_ must be deleted > _before_ video_decoder_. Done. https://codereview.chromium.org/820823002/diff/80001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.h:127: // received of the plugin was resized. On 2015/01/06 22:17:10, Wez wrote: > s/of/or Done. https://codereview.chromium.org/820823002/diff/80001/remoting/client/plugin/p... remoting/client/plugin/pepper_video_renderer_3d.h:134: // 0, GL_TEXTURE_2D, GL_TEXTURE_RECTANGLE_ARB or GL_TEXTURE_EXTERNAL_OES. On 2015/01/06 22:17:10, Wez wrote: > What does a value of 0 mean, though? Done.
Patchset #4 (id:100001) has been deleted
The CQ bit was checked by wez@chromium.org
lgtm https://codereview.chromium.org/820823002/diff/80001/remoting/client/plugin/c... File remoting/client/plugin/chromoting_instance.cc (right): https://codereview.chromium.org/820823002/diff/80001/remoting/client/plugin/c... remoting/client/plugin/chromoting_instance.cc:641: video_renderer_.reset(); On 2015/01/07 19:37:30, Sergey Ulanov wrote: > On 2015/01/06 22:17:10, Wez wrote: > > nit: This would be cleaner if you replaced the new+Initialize with > > FooVideoRenderer::Create() methods. > > Yes, but given that this is the only place were the renderer needs to be > initialized I don't think that adding the Create() function is worth it. Acknowledged.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/820823002/120001
Message was sent while issue was closed.
Committed patchset #4 (id:120001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/25f8cb723e93b24f19c27e6c77bbe861ec52cbd9 Cr-Commit-Position: refs/heads/master@{#310387} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
