Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2528143002:
Delete deprecated versions of Copy, ScaleFrom and CropAndScaleFrom. (Closed)
|
Created:
4 years ago by nisse-webrtc Modified:
4 years ago Reviewers:
magjed_webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionDelete deprecated versions of Copy, ScaleFrom and CropAndScaleFrom.
BUG=webrtc:6672
Committed: https://crrev.com/f15a2c51939e9ff401ef45fab1335253d4c74ef3
Cr-Commit-Position: refs/heads/master@{#15317}
Patch Set 1 #Patch Set 2 : Rebased #
Total comments: 2
Messages
Total messages: 16 (6 generated)
Description was changed from ========== Delete deprecated versions of ScaleFrom and CropAndScaleFrom. BUG=webrtc:6672 ========== to ========== Delete deprecated versions of Copy, ScaleFrom and CropAndScaleFrom. BUG=webrtc:6672 ==========
nisse@webrtc.org changed reviewers: + magjed@webrtc.org
Initial cleanup of I420Buffer. I think this should be landable after some minor downstream updates.
lgtm
https://codereview.webrtc.org/2528143002/diff/20001/webrtc/common_video/inclu... File webrtc/common_video/include/video_frame_buffer.h (right): https://codereview.webrtc.org/2528143002/diff/20001/webrtc/common_video/inclu... webrtc/common_video/include/video_frame_buffer.h:37: virtual const uint8_t* DataY() const = 0; This is unrelated to this CL, but before we move I420Buffer to api/, maybe we should consider moving the Data*() and Stride*() functions from VideoFrameBuffer to I420Buffer, and change the return type of NativeToI420Buffer() to: virtual rtc::scoped_refptr<I420Buffer> NativeToI420Buffer() = 0; What do you think?
https://codereview.webrtc.org/2528143002/diff/20001/webrtc/common_video/inclu... File webrtc/common_video/include/video_frame_buffer.h (right): https://codereview.webrtc.org/2528143002/diff/20001/webrtc/common_video/inclu... webrtc/common_video/include/video_frame_buffer.h:37: virtual const uint8_t* DataY() const = 0; On 2016/11/29 13:43:07, magjed_webrtc wrote: > This is unrelated to this CL, but before we move I420Buffer to api/, maybe we > should consider moving the Data*() and Stride*() functions from VideoFrameBuffer > to I420Buffer, and change the return type of NativeToI420Buffer() to: > virtual rtc::scoped_refptr<I420Buffer> NativeToI420Buffer() = 0; > > What do you think? Makes some sense (provided we also find a better name for that method), but I'm not sure it's a good idea. In one way it is convenient to be able to render a VideoFrameBuffer directly. And I really want to keep the distinction between mutable and immutable video buffers, and to do that, if I420Buffer::NativeToI420Buffer should return itself rather than make a new copy, the return value would need to be rtc::scoped_refptr<const I420Buffer>.
On 2016/11/29 14:04:06, nisse-webrtc wrote: > https://codereview.webrtc.org/2528143002/diff/20001/webrtc/common_video/inclu... > File webrtc/common_video/include/video_frame_buffer.h (right): > > https://codereview.webrtc.org/2528143002/diff/20001/webrtc/common_video/inclu... > webrtc/common_video/include/video_frame_buffer.h:37: virtual const uint8_t* > DataY() const = 0; > On 2016/11/29 13:43:07, magjed_webrtc wrote: > > This is unrelated to this CL, but before we move I420Buffer to api/, maybe we > > should consider moving the Data*() and Stride*() functions from > VideoFrameBuffer > > to I420Buffer, and change the return type of NativeToI420Buffer() to: > > virtual rtc::scoped_refptr<I420Buffer> NativeToI420Buffer() = 0; > > > > What do you think? > > Makes some sense (provided we also find a better name for that method), but I'm > not sure it's a good idea. In one way it is convenient to be able to render a > VideoFrameBuffer directly. This is not the case today. Data*() and Stride*() will simply crash for native frames. The point is that maybe it's good to make the frames more opaque. Native Android and iOS frames are simply not YUV frames. > And I really want to keep the distinction between mutable and immutable video > buffers, and to do that, if I420Buffer::NativeToI420Buffer should return itself > rather than make a new copy, the return value would need to be > rtc::scoped_refptr<const I420Buffer>. Yes, returning a const I420Buffer is probably fine.
On 2016/11/29 15:49:37, magjed_webrtc wrote: > This is not the case today. Data*() and Stride*() will simply crash for native > frames. The point is that maybe it's good to make the frames more opaque. Native > Android and iOS frames are simply not YUV frames. Maybe we'de need to introduce another abstract interface then, inheriting VideoFrameBuffer, but providing Data and Stride. It would be implemented by both I420Buffer and WrappedI420Buffer. Makes some sense, but it will be quite a lot of work to be able to retire VideoFrameBuffer::Data*. We'd get a lot if video_frame->video_frame_buffer()->GetI420Buffer(). > Yes, returning a const I420Buffer is probably fine. Ok. It's just that scoped_refptr<const AnyThing> seems a bit unusual in our code.
On 2016/11/29 16:03:19, nisse-webrtc wrote: > On 2016/11/29 15:49:37, magjed_webrtc wrote: > > > This is not the case today. Data*() and Stride*() will simply crash for native > > frames. The point is that maybe it's good to make the frames more opaque. > Native > > Android and iOS frames are simply not YUV frames. > > Maybe we'de need to introduce another abstract interface then, inheriting > VideoFrameBuffer, but providing Data and Stride. It would be implemented by both > I420Buffer and WrappedI420Buffer. Yeah, we probably need a new interface for Data and Stride, but it shouldn't need to inherit VideoFrameBuffer. > Makes some sense, but it will be quite a lot of work to be able to retire > VideoFrameBuffer::Data*. > We'd get a lot if video_frame->video_frame_buffer()->GetI420Buffer(). > > > Yes, returning a const I420Buffer is probably fine. > > Ok. It's just that scoped_refptr<const AnyThing> seems a bit unusual in our > code. Ok, I only got 20 hits for VideoFrameBuffer::DataY() call sites in code search, most of them in tests, so it looked feasible, but maybe not. Anyway, unrelated to this CL, so lgtm again :)
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/...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1480497572634340,
"parent_rev": "ceeb37dfef7985da2e49ba28c8f20e5b4ce641c8", "commit_rev":
"d98619308167aad16f2e09c2169f7c4ebaa3345b"}
Message was sent while issue was closed.
Description was changed from ========== Delete deprecated versions of Copy, ScaleFrom and CropAndScaleFrom. BUG=webrtc:6672 ========== to ========== Delete deprecated versions of Copy, ScaleFrom and CropAndScaleFrom. BUG=webrtc:6672 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Delete deprecated versions of Copy, ScaleFrom and CropAndScaleFrom. BUG=webrtc:6672 ========== to ========== Delete deprecated versions of Copy, ScaleFrom and CropAndScaleFrom. BUG=webrtc:6672 Committed: https://crrev.com/f15a2c51939e9ff401ef45fab1335253d4c74ef3 Cr-Commit-Position: refs/heads/master@{#15317} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/f15a2c51939e9ff401ef45fab1335253d4c74ef3 Cr-Commit-Position: refs/heads/master@{#15317} |
