Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2285693002:
New static I420Buffer::Rotate method, to replace GetCopyWithRotationApplied. (Closed)
|
Created:
4 years, 3 months ago by nisse-webrtc Modified:
4 years, 3 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, the sun Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionNew static I420Buffer::Rotate method, to replace GetCopyWithRotationApplied.
GetCopyWithRotationApplied is not yet deleted; downstream projects
must be updated first.
BUG=webrtc:5682
Committed: https://crrev.com/d50747269e6a3de5c05288184c5f58a11eb6d268
Cr-Commit-Position: refs/heads/master@{#13973}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Address magjed's comments. And add missing include. #
Total comments: 4
Patch Set 3 : windows: Update include line. #Patch Set 4 : Address comments. #Patch Set 5 : Change back to static method, now in I420Buffer. #Messages
Total messages: 47 (25 generated)
The CQ bit was checked by nisse@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
nisse@webrtc.org changed reviewers: + magjed@webrtc.org, perkj@webrtc.org
PTAL. Need to land this first, then update downstream projects (at least one use in Chrome), and finally kill GetCopyWithRotationApplied.
https://codereview.webrtc.org/2285693002/diff/1/webrtc/common_video/video_fra... File webrtc/common_video/video_frame_buffer.cc (right): https://codereview.webrtc.org/2285693002/diff/1/webrtc/common_video/video_fra... webrtc/common_video/video_frame_buffer.cc:57: int current_width = src->width(); I would prefer if you removed current_width and use src->width() directly. https://codereview.webrtc.org/2285693002/diff/1/webrtc/common_video/video_fra... webrtc/common_video/video_frame_buffer.cc:70: // TODO(guoweis): Add a function in webrtc_libyuv.cc to convert from Remove this TODO. I think guoweis has moved on from working with this. Static cast works for me. https://codereview.webrtc.org/2285693002/diff/1/webrtc/examples/peerconnectio... File webrtc/examples/peerconnection/client/main_wnd.cc (right): https://codereview.webrtc.org/2285693002/diff/1/webrtc/examples/peerconnectio... webrtc/examples/peerconnection/client/main_wnd.cc:617: image_.get(), you need to adjust indentation here https://codereview.webrtc.org/2285693002/diff/1/webrtc/media/devices/carbonvi... File webrtc/media/devices/carbonvideorenderer.cc (left): https://codereview.webrtc.org/2285693002/diff/1/webrtc/media/devices/carbonvi... webrtc/media/devices/carbonvideorenderer.cc:111: if (!SetSize(frame->width(), frame->height())) { Don't we need this SetSize call anymore? It looks dangerous to remove it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_baremetal/builds/13646)
https://codereview.webrtc.org/2285693002/diff/1/webrtc/common_video/video_fra... File webrtc/common_video/video_frame_buffer.cc (right): https://codereview.webrtc.org/2285693002/diff/1/webrtc/common_video/video_fra... webrtc/common_video/video_frame_buffer.cc:57: int current_width = src->width(); On 2016/08/26 12:04:15, magjed_webrtc wrote: > I would prefer if you removed current_width and use src->width() directly. Done. https://codereview.webrtc.org/2285693002/diff/1/webrtc/common_video/video_fra... webrtc/common_video/video_frame_buffer.cc:70: // TODO(guoweis): Add a function in webrtc_libyuv.cc to convert from On 2016/08/26 12:04:15, magjed_webrtc wrote: > Remove this TODO. I think guoweis has moved on from working with this. Static > cast works for me. Done. https://codereview.webrtc.org/2285693002/diff/1/webrtc/examples/peerconnectio... File webrtc/examples/peerconnection/client/main_wnd.cc (right): https://codereview.webrtc.org/2285693002/diff/1/webrtc/examples/peerconnectio... webrtc/examples/peerconnection/client/main_wnd.cc:617: image_.get(), On 2016/08/26 12:04:15, magjed_webrtc wrote: > you need to adjust indentation here Done. https://codereview.webrtc.org/2285693002/diff/1/webrtc/media/devices/carbonvi... File webrtc/media/devices/carbonvideorenderer.cc (left): https://codereview.webrtc.org/2285693002/diff/1/webrtc/media/devices/carbonvi... webrtc/media/devices/carbonvideorenderer.cc:111: if (!SetSize(frame->width(), frame->height())) { On 2016/08/26 12:04:15, magjed_webrtc wrote: > Don't we need this SetSize call anymore? It looks dangerous to remove it. Good catch, that was not an intended change. (And we should try to delete webrtc/media/devices/, the code isn't used within webrtc, and there are no tests, so it'll inevitably suffer some bitrot. Users of the code should at some point transition to using the renderers under webrtc/test).
lgtm
https://codereview.webrtc.org/2285693002/diff/20001/webrtc/common_video/inclu... File webrtc/common_video/include/video_frame_buffer.h (right): https://codereview.webrtc.org/2285693002/diff/20001/webrtc/common_video/inclu... webrtc/common_video/include/video_frame_buffer.h:67: static rtc::scoped_refptr<VideoFrameBuffer> Rotate( I think I would prefer this to be a virtual member function just like NativeTo if this helper method should be part of the interface. ... Then NativeHandles and other - none i420 frames have a chance to override it. The implentation in this cl can be the default implementation. Another totally crazy idea that would cause a lot of work would be to change NativeToI420Buffer to return an I420Buffer. - remove all methods that returns pointers to i420 data and force them to call NativeToI420Buffer().... Just a crazy idea that would allow VideoFrameBuffer to not know about that it represents,(i420, textures, rgb etc..) https://codereview.webrtc.org/2285693002/diff/20001/webrtc/common_video/video... File webrtc/common_video/video_frame_buffer.cc (right): https://codereview.webrtc.org/2285693002/diff/20001/webrtc/common_video/video... webrtc/common_video/video_frame_buffer.cc:52: VideoRotation rotation) { please dcheck that src->DataY and U and V returns a pointer!=nullptr. This may be a native handle for all we know.
Made Rotate a real virtual method. Is the name ok, or does it sound like a destructive operation? I was tempted to mark the method as const, but that makes the "return this" case a bit awkward. https://codereview.webrtc.org/2285693002/diff/20001/webrtc/common_video/inclu... File webrtc/common_video/include/video_frame_buffer.h (right): https://codereview.webrtc.org/2285693002/diff/20001/webrtc/common_video/inclu... webrtc/common_video/include/video_frame_buffer.h:67: static rtc::scoped_refptr<VideoFrameBuffer> Rotate( On 2016/08/26 15:22:17, perkj_webrtc wrote: > I think I would prefer this to be a virtual member function just like NativeTo > if this helper method should be part of the interface. ... > Then NativeHandles and other - none i420 frames have a chance to override it. > The implentation in this cl can be the default implementation. Makes sense to me. (magjed suggested using a static method). If we make it non-static, is Rotate still a good enough name? > Another totally crazy idea that would cause a lot of work would be to change > NativeToI420Buffer to return an I420Buffer. - remove all methods that returns > pointers to i420 data and force them to call NativeToI420Buffer().... We'd need an abstract interface for a read-only I420Frame then, which could be inherited by both I420Buffer, WrappedI420Buffer, and possibly others. We really shouldn't allow frame->video_frame_buffer()->NativeToI420Buffer()->MutableDataX()
The CQ bit was checked by nisse@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_tsan2 on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/14591)
lgtm
The CQ bit was checked by nisse@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org Link to the patchset: https://codereview.webrtc.org/2285693002/#ps60001 (title: "Address comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/7796)
nisse@webrtc.org changed reviewers: + mflodman@webrtc.org
mflodman: PTAL, OWNER's approval needed. Should we adjust the common_video/OWNERS, adding perkj and/or myself? Current owners are stefan, marpan, henrik.lundin, and pbos.
not lgtm, please remove Rotate from the interface. https://codereview.webrtc.org/2285693002/diff/20001/webrtc/common_video/inclu... File webrtc/common_video/include/video_frame_buffer.h (right): https://codereview.webrtc.org/2285693002/diff/20001/webrtc/common_video/inclu... webrtc/common_video/include/video_frame_buffer.h:67: static rtc::scoped_refptr<VideoFrameBuffer> Rotate( On 2016/08/29 09:16:50, nisse-webrtc wrote: > On 2016/08/26 15:22:17, perkj_webrtc wrote: > > I think I would prefer this to be a virtual member function just like NativeTo > > if this helper method should be part of the interface. ... > > Then NativeHandles and other - none i420 frames have a chance to override it. > > The implentation in this cl can be the default implementation. > > Makes sense to me. (magjed suggested using a static method). > > If we make it non-static, is Rotate still a good enough name? > Please don't add unnecessary functions to the interface! It will cause the same trouble in the future as GetCopyWithRotationApplied is causing now. This Rotate helper function should not be part of the interface. It's used for legacy clients that don't support CVO and they have to fall back to NativeToI420Buffer and then rotate, so there is no use case for rotating non-i420 frames. We want to keep the interface as small as possible and only add stuff that we need. Adding rotation support outside rendering for iOS CVPixelBuffers and Chrome GpuMemoryBuffers would require a lot of work and I don't want to do that unless it's needed.
The CQ bit was checked by nisse@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Description was changed from ========== New VideoFrameBuffer::Rotate method, to replace GetCopyWithRotationApplied. GetCopyWithRotationApplied is not yet deleted; downstream projects must be updated first. BUG=webrtc:5682 ========== to ========== New static I420Buffer::Rotate method, to replace GetCopyWithRotationApplied. GetCopyWithRotationApplied is not yet deleted; downstream projects must be updated first. BUG=webrtc:5682 ==========
Changed back to a static method, as discussed offline.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/15968)
On 2016/08/30 08:42:04, nisse-webrtc wrote: > Changed back to a static method, as discussed offline. lgtm
On 2016/08/30 08:42:04, nisse-webrtc wrote: > Changed back to a static method, as discussed offline. lgtm again
lgtm for common_video
The CQ bit was checked by nisse@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/15984)
The CQ bit was checked by nisse@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded global retry quota
The CQ bit was checked by nisse@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== New static I420Buffer::Rotate method, to replace GetCopyWithRotationApplied. GetCopyWithRotationApplied is not yet deleted; downstream projects must be updated first. BUG=webrtc:5682 ========== to ========== New static I420Buffer::Rotate method, to replace GetCopyWithRotationApplied. GetCopyWithRotationApplied is not yet deleted; downstream projects must be updated first. BUG=webrtc:5682 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== New static I420Buffer::Rotate method, to replace GetCopyWithRotationApplied. GetCopyWithRotationApplied is not yet deleted; downstream projects must be updated first. BUG=webrtc:5682 ========== to ========== New static I420Buffer::Rotate method, to replace GetCopyWithRotationApplied. GetCopyWithRotationApplied is not yet deleted; downstream projects must be updated first. BUG=webrtc:5682 Committed: https://crrev.com/d50747269e6a3de5c05288184c5f58a11eb6d268 Cr-Commit-Position: refs/heads/master@{#13973} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/d50747269e6a3de5c05288184c5f58a11eb6d268 Cr-Commit-Position: refs/heads/master@{#13973} |
