Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
877283004:
MediaStreamRemoteVideoSource: Wrap cricket::VideoFrame instead of copying. (Closed)
|
Created:
5 years, 10 months ago by magjed_chromium Modified:
5 years, 9 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org, wjia+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMediaStreamRemoteVideoSource: Wrap cricket::VideoFrame instead of copying.
We can use cricket::VideoFrame::Copy() to make a shallow reference counted copy.
Committed: https://crrev.com/e8ea0c7b982aa6a5a144fa6c4454352018524682
Cr-Commit-Position: refs/heads/master@{#319866}
Patch Set 1 : #
Total comments: 6
Patch Set 2 : tommi@s and perkj@s comments #Patch Set 3 : Introduce |is_read_only_| to media::VideoFrame #
Total comments: 4
Patch Set 4 : const_cast #Patch Set 5 : TODO about const_cast #Messages
Total messages: 19 (7 generated)
magjed@chromium.org changed reviewers: + perkj@chromium.org, tommi@chromium.org
Patchset #1 (id:1) has been deleted
Please take a look.
lgtm https://codereview.chromium.org/877283004/diff/20001/content/renderer/media/w... File content/renderer/media/webrtc/media_stream_remote_video_source.cc (right): https://codereview.chromium.org/877283004/diff/20001/content/renderer/media/w... content/renderer/media/webrtc/media_stream_remote_video_source.cc:23: // Release |frame| when it goes out of scope. nit: s/Release/Delete (since Release usually refers to reference counting)
https://codereview.chromium.org/877283004/diff/20001/content/renderer/media/w... File content/renderer/media/webrtc/media_stream_remote_video_source.cc (left): https://codereview.chromium.org/877283004/diff/20001/content/renderer/media/w... content/renderer/media/webrtc/media_stream_remote_video_source.cc:89: video_frame = frame_pool_.CreateFrame( frame_pool_ is not needed now. Please remove it and the header file. https://codereview.chromium.org/877283004/diff/20001/content/renderer/media/w... File content/renderer/media/webrtc/media_stream_remote_video_source.cc (right): https://codereview.chromium.org/877283004/diff/20001/content/renderer/media/w... content/renderer/media/webrtc/media_stream_remote_video_source.cc:102: // Take a shallow copy. Both |frame| and |copied_frame| will share a single nit Make a shallow copy...
perkj@ pointed out that the subclass we receive is always WebRtcVideoRenderFrame, both from webrtcvideoengine.cc and webrtcvideoengine2.cc. So currently, this CL just moves the copy from chromium to webrtc. In the current code, we at least avoid allocating new memory for every frame, so I propose we wait until the deep copy in WebRtcVideoRenderFrame is fixed before landing this. https://codereview.chromium.org/877283004/diff/20001/content/renderer/media/w... File content/renderer/media/webrtc/media_stream_remote_video_source.cc (left): https://codereview.chromium.org/877283004/diff/20001/content/renderer/media/w... content/renderer/media/webrtc/media_stream_remote_video_source.cc:89: video_frame = frame_pool_.CreateFrame( On 2015/01/30 08:57:40, perkj wrote: > frame_pool_ is not needed now. Please remove it and the header file. Done. https://codereview.chromium.org/877283004/diff/20001/content/renderer/media/w... File content/renderer/media/webrtc/media_stream_remote_video_source.cc (right): https://codereview.chromium.org/877283004/diff/20001/content/renderer/media/w... content/renderer/media/webrtc/media_stream_remote_video_source.cc:23: // Release |frame| when it goes out of scope. On 2015/01/29 14:21:41, tommi wrote: > nit: s/Release/Delete > (since Release usually refers to reference counting) Done. https://codereview.chromium.org/877283004/diff/20001/content/renderer/media/w... content/renderer/media/webrtc/media_stream_remote_video_source.cc:102: // Take a shallow copy. Both |frame| and |copied_frame| will share a single On 2015/01/30 08:57:40, perkj wrote: > nit Make a shallow copy... Done.
WebRTC is updated, so this CL will remove one frame copy now. Per can you take a look again?
exciting lgtm
Patchset #3 (id:60001) has been deleted
mcasas@chromium.org changed reviewers: + mcasas@chromium.org
drive-by. https://codereview.chromium.org/877283004/diff/80001/content/renderer/media/w... File content/renderer/media/webrtc/media_stream_remote_video_source.cc (right): https://codereview.chromium.org/877283004/diff/80001/content/renderer/media/w... content/renderer/media/webrtc/media_stream_remote_video_source.cc:96: frame->GetYPlane(), frame->GetUPlane(), frame->GetVPlane(), timestamp, If I read correctly [1], there is a non-const version of cricket::VideoFrame::GetYPlane() etc, and although I dig the meaning of this CL, I'm wary of adding yet another factory method to the already-fat-enough VideoFrame. Could we just somehow reuse WrapExternalYuvData()? [1] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin... https://codereview.chromium.org/877283004/diff/80001/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/877283004/diff/80001/media/base/video_frame.h... media/base/video_frame.h:403: bool is_read_only_; Having a flag and |data_[]| separated might be an accident waiting to happen? Specially since you only use it for DCHECKing. I see two options, one is biting the bullet and substituting data_+is_read_only_ with a class doing policy check and access, another is throwing away this check. As it is, is not adding much... Or if you feel ready for a big CL, add a policy or traits to VideoFrame, being that Read-Only or ReadWrite, which would control the access to it :)
magjed@chromium.org changed reviewers: + scherkus@chromium.org
scherkus - Can you take a look at patch set #3? Do you have any ideas how to elegantly wrap a const frame? https://codereview.chromium.org/877283004/diff/80001/content/renderer/media/w... File content/renderer/media/webrtc/media_stream_remote_video_source.cc (right): https://codereview.chromium.org/877283004/diff/80001/content/renderer/media/w... content/renderer/media/webrtc/media_stream_remote_video_source.cc:96: frame->GetYPlane(), frame->GetUPlane(), frame->GetVPlane(), timestamp, On 2015/03/06 17:29:59, mcasas wrote: > If I read correctly [1], there is a non-const version of > cricket::VideoFrame::GetYPlane() etc, and although I dig > the meaning of this CL, I'm wary of adding yet another > factory method to the already-fat-enough VideoFrame. > Could we just somehow reuse WrapExternalYuvData()? > > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin... Yes we can reuse WrapExternalYuvData, by const casting. That is what I did in patch set #3 together with DCHECKing that no one overwrites the data. I have inlined it now without any checks, but I don’t really like that. https://codereview.chromium.org/877283004/diff/80001/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/877283004/diff/80001/media/base/video_frame.h... media/base/video_frame.h:403: bool is_read_only_; On 2015/03/06 17:29:59, mcasas wrote: > Having a flag and |data_[]| separated might be an > accident waiting to happen? Specially since you > only use it for DCHECKing. > > I see two options, one is biting the bullet and > substituting data_+is_read_only_ with a class > doing policy check and access, another is throwing > away this check. As it is, is not adding much... > > Or if you feel ready for a big CL, add a policy or > traits to VideoFrame, being that Read-Only or > ReadWrite, which would control the access to it :) DCHECKing a read_only flag adds enough to find places where we incorrectly extract non-const pointers. For example, here is the first place for WebRtcBrowserTest.CanForwardRemoteStream720p: https://code.google.com/p/chromium/codesearch#chromium/src/media/blink/skcanv... Of course, I would prefer static checks, but that would require a huge CL. For example, no one is using 'scoped_refptr<const VideoFrame>’.
I want to remove this frame copy as soon as possible, so I will const_cast in this CL and fix the const_cast in a future CL.
The CQ bit was checked by magjed@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from perkj@chromium.org, tommi@chromium.org Link to the patchset: https://codereview.chromium.org/877283004/#ps120001 (title: "TODO about const_cast")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/877283004/120001
Message was sent while issue was closed.
Committed patchset #5 (id:120001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/e8ea0c7b982aa6a5a144fa6c4454352018524682 Cr-Commit-Position: refs/heads/master@{#319866} |
