Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
604743005:
VideoCapture: Remove deep frame copy in the border to libJingle (Closed)
|
Created:
6 years, 2 months ago by magjed_chromium Modified:
6 years, 2 months ago CC:
juberti2, chromium-reviews, 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, miu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionVideoCapture: Remove deep frame copy in the border to libJingle
BUG=359587
Committed: https://crrev.com/4087adc5677bb8dd7f414c0afafc8f4215ce0077
Cr-Commit-Position: refs/heads/master@{#300959}
Patch Set 1 #
Total comments: 15
Patch Set 2 : tommi@s comments #
Total comments: 16
Patch Set 3 : perkj@s comments #
Total comments: 4
Patch Set 4 : perkj@s comments 2 #
Total comments: 11
Patch Set 5 : cleanup media::VideoFrame #
Total comments: 6
Patch Set 6 : scherkus@s comments #
Messages
Total messages: 34 (13 generated)
magjed@chromium.org changed reviewers: + mcasas@chromium.org, perkj@chromium.org, tommi@chromium.org
perkj, this is the implementation you suggested. I prefer this one. Then we can clean up cricket::VideoFrame and cricket::VideoCapturer in separate Cls. https://codereview.chromium.org/604743005/diff/1/content/renderer/media/webrt... File content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc (left): https://codereview.chromium.org/604743005/diff/1/content/renderer/media/webrt... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:174: libyuv::I420Scale(src_y, Let the CoordinatedVideoAdapter take care of this instead. The frame might be dropped, then we don't need to do any scaling at all. And if we need to do scaling, it might not be same scale as the CoordinatedVideoAdapter wants, then we do two scalings in a row.
https://codereview.chromium.org/604743005/diff/1/content/renderer/media/webrt... File content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc (right): https://codereview.chromium.org/604743005/diff/1/content/renderer/media/webrt... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:24: class VideoFrameWrapper : public cricket::VideoFrame { Assuming this is a single threaded class, can we add a thread checker to it and DCHECK thread correctness in every method? It helps a lot with maintainability (documentation) and protect against regressions. https://codereview.chromium.org/604743005/diff/1/content/renderer/media/webrt... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:191: class DummyFactory : public cricket::VideoFrameFactory { can you add documentation for what context this class is used in? Should this class and the above code perhaps be in an anonymous namespace inside the content namespace? https://codereview.chromium.org/604743005/diff/1/content/renderer/media/webrt... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:206: const scoped_refptr<media::VideoFrame>& frame_; is the reference intentional? I'd think it'd be safer to actually let the factory hold on to its own reference. As is, this is effectively a pointer to a reference counted object whose lifetime this implementation has no control over. https://codereview.chromium.org/604743005/diff/1/content/renderer/media/webrt... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:249: if (fourccs) Does it make sense to call this function with a NULL pointer? (i.e. should it be an assumption/DCHECK that fourccs is never NULL?) Under normal circumstances, would we expect fourccs to be empty on entry? If so, we might want to add a DCHECK(fourccs->empty()) at the top of the function (or DCHECK(!fourccs || fourccs->empty())) If fourccs is valid, I'm guessing also that we expect that it does not already contain cricket::FOURCC_I420. Can we DCHECK that too? Something like DCHECK_EQ(std::find(fourccs->begin(), fourccs->end(), cricket::FOURCC_I420), fourccs->end()); https://codereview.chromium.org/604743005/diff/1/content/renderer/media/webrt... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:251: return fourccs; hm... does this compile? :) I'm guessing you'd want return fourccs != NULL; https://codereview.chromium.org/604743005/diff/1/content/renderer/media/webrt... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:314: set_frame_factory(new DummyFactory(frame, elapsed_time)); does set_frame_factory accept a scoped_ptr? if not, it would be nice to fix this up to use scoped_ptr's move semantics (and make sure that the implementation can never leak previously set factories). https://codereview.chromium.org/604743005/diff/1/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/604743005/diff/1/media/base/video_frame.h#new... media/base/video_frame.h:272: uint8* visible_data(size_t plane) const; const method returning a pointer to a non-const buffer tends to go against chromium code in general
CoordinatedVideoAdapter does not always scale to the correct size at the moment. Will fix that tomorrow. https://codereview.chromium.org/604743005/diff/1/content/renderer/media/webrt... File content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc (right): https://codereview.chromium.org/604743005/diff/1/content/renderer/media/webrt... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:24: class VideoFrameWrapper : public cricket::VideoFrame { On 2014/09/25 20:06:48, tommi wrote: > Assuming this is a single threaded class, can we add a thread checker to it and > DCHECK thread correctness in every method? It helps a lot with maintainability > (documentation) and protect against regressions. Yes, will fix. https://codereview.chromium.org/604743005/diff/1/content/renderer/media/webrt... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:191: class DummyFactory : public cricket::VideoFrameFactory { On 2014/09/25 20:06:48, tommi wrote: > can you add documentation for what context this class is used in? > > Should this class and the above code perhaps be in an anonymous namespace inside > the content namespace? The context for VideoFrameWrapper and DummyFactory is only within this file. I will put them in an anonymous namespace. https://codereview.chromium.org/604743005/diff/1/content/renderer/media/webrt... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:206: const scoped_refptr<media::VideoFrame>& frame_; On 2014/09/25 20:06:48, tommi wrote: > is the reference intentional? > > I'd think it'd be safer to actually let the factory hold on to its own > reference. As is, this is effectively a pointer to a reference counted object > whose lifetime this implementation has no control over. Ops, that is not intentional. Will fix. https://codereview.chromium.org/604743005/diff/1/content/renderer/media/webrt... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:249: if (fourccs) On 2014/09/25 20:06:48, tommi wrote: > Does it make sense to call this function with a NULL pointer? (i.e. should it be > an assumption/DCHECK that fourccs is never NULL?) > > Under normal circumstances, would we expect fourccs to be empty on entry? > If so, we might want to add a DCHECK(fourccs->empty()) at the top of the > function (or DCHECK(!fourccs || fourccs->empty())) > > If fourccs is valid, I'm guessing also that we expect that it does not already > contain cricket::FOURCC_I420. Can we DCHECK that too? > Something like > DCHECK_EQ(std::find(fourccs->begin(), fourccs->end(), cricket::FOURCC_I420), > fourccs->end()); We only have one real call to this function in Chromium and it looks like: std::vector<uint32> preferred_fourccs; if (!GetPreferredFourccs(&preferred_fourccs)) but we also have a unittest in remotevideocapturer_unittest.cc: EXPECT_FALSE(capturer_.GetPreferredFourccs(NULL)); I guess we can add DCHECK(!fourccs || fourccs->empty()) or remove the unittest and add DCHECK(fourccs && fourccs->empty()). https://codereview.chromium.org/604743005/diff/1/content/renderer/media/webrt... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:251: return fourccs; On 2014/09/25 20:06:48, tommi wrote: > hm... does this compile? :) > > I'm guessing you'd want > > return fourccs != NULL; Yes, sorry about that. It compiled locally. https://codereview.chromium.org/604743005/diff/1/content/renderer/media/webrt... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:314: set_frame_factory(new DummyFactory(frame, elapsed_time)); On 2014/09/25 20:06:48, tommi wrote: > does set_frame_factory accept a scoped_ptr? if not, it would be nice to fix this > up to use scoped_ptr's move semantics (and make sure that the implementation can > never leak previously set factories). No, it does not accept a scoped_ptr. The definition looks like this: rtc::scoped_ptr<VideoFrameFactory> frame_factory_; // Takes ownership. void set_frame_factory(VideoFrameFactory* frame_factory) { frame_factory_.reset(frame_factory); } https://codereview.chromium.org/604743005/diff/1/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/604743005/diff/1/media/base/video_frame.h#new... media/base/video_frame.h:272: uint8* visible_data(size_t plane) const; On 2014/09/25 20:06:48, tommi wrote: > const method returning a pointer to a non-const buffer tends to go against > chromium code in general Yes, agree. I followed the convention in this file for uint8* data(size_t plane) const. I guess we should change to const uint8* data(size_t plane) const; uint8* data(size_t plane); const uint8* visible_data(size_t plane) const; uint8* visible_data(size_t plane);
Will you ping me when there are updates? On 2014/09/25 20:52:08, magjed wrote: > CoordinatedVideoAdapter does not always scale to the correct size at the moment. > Will fix that tomorrow. > > https://codereview.chromium.org/604743005/diff/1/content/renderer/media/webrt... > File content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc (right): > > https://codereview.chromium.org/604743005/diff/1/content/renderer/media/webrt... > content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:24: class > VideoFrameWrapper : public cricket::VideoFrame { > On 2014/09/25 20:06:48, tommi wrote: > > Assuming this is a single threaded class, can we add a thread checker to it > and > > DCHECK thread correctness in every method? It helps a lot with > maintainability > > (documentation) and protect against regressions. > > Yes, will fix. > > https://codereview.chromium.org/604743005/diff/1/content/renderer/media/webrt... > content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:191: class > DummyFactory : public cricket::VideoFrameFactory { > On 2014/09/25 20:06:48, tommi wrote: > > can you add documentation for what context this class is used in? > > > > Should this class and the above code perhaps be in an anonymous namespace > inside > > the content namespace? > > The context for VideoFrameWrapper and DummyFactory is only within this file. I > will put them in an anonymous namespace. > > https://codereview.chromium.org/604743005/diff/1/content/renderer/media/webrt... > content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:206: const > scoped_refptr<media::VideoFrame>& frame_; > On 2014/09/25 20:06:48, tommi wrote: > > is the reference intentional? > > > > I'd think it'd be safer to actually let the factory hold on to its own > > reference. As is, this is effectively a pointer to a reference counted object > > whose lifetime this implementation has no control over. > > Ops, that is not intentional. Will fix. > > https://codereview.chromium.org/604743005/diff/1/content/renderer/media/webrt... > content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:249: if (fourccs) > On 2014/09/25 20:06:48, tommi wrote: > > Does it make sense to call this function with a NULL pointer? (i.e. should it > be > > an assumption/DCHECK that fourccs is never NULL?) > > > > Under normal circumstances, would we expect fourccs to be empty on entry? > > If so, we might want to add a DCHECK(fourccs->empty()) at the top of the > > function (or DCHECK(!fourccs || fourccs->empty())) > > > > If fourccs is valid, I'm guessing also that we expect that it does not already > > contain cricket::FOURCC_I420. Can we DCHECK that too? > > Something like > > DCHECK_EQ(std::find(fourccs->begin(), fourccs->end(), cricket::FOURCC_I420), > > fourccs->end()); > > We only have one real call to this function in Chromium and it looks like: > std::vector<uint32> preferred_fourccs; > if (!GetPreferredFourccs(&preferred_fourccs)) > but we also have a unittest in remotevideocapturer_unittest.cc: > EXPECT_FALSE(capturer_.GetPreferredFourccs(NULL)); > > I guess we can add DCHECK(!fourccs || fourccs->empty()) or remove the unittest > and add DCHECK(fourccs && fourccs->empty()). > > https://codereview.chromium.org/604743005/diff/1/content/renderer/media/webrt... > content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:251: return > fourccs; > On 2014/09/25 20:06:48, tommi wrote: > > hm... does this compile? :) > > > > I'm guessing you'd want > > > > return fourccs != NULL; > > Yes, sorry about that. It compiled locally. > > https://codereview.chromium.org/604743005/diff/1/content/renderer/media/webrt... > content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:314: > set_frame_factory(new DummyFactory(frame, elapsed_time)); > On 2014/09/25 20:06:48, tommi wrote: > > does set_frame_factory accept a scoped_ptr? if not, it would be nice to fix > this > > up to use scoped_ptr's move semantics (and make sure that the implementation > can > > never leak previously set factories). > > No, it does not accept a scoped_ptr. The definition looks like this: > rtc::scoped_ptr<VideoFrameFactory> frame_factory_; > // Takes ownership. > void set_frame_factory(VideoFrameFactory* frame_factory) { > frame_factory_.reset(frame_factory); > } > > https://codereview.chromium.org/604743005/diff/1/media/base/video_frame.h > File media/base/video_frame.h (right): > > https://codereview.chromium.org/604743005/diff/1/media/base/video_frame.h#new... > media/base/video_frame.h:272: uint8* visible_data(size_t plane) const; > On 2014/09/25 20:06:48, tommi wrote: > > const method returning a pointer to a non-const buffer tends to go against > > chromium code in general > > Yes, agree. I followed the convention in this file for uint8* data(size_t plane) > const. I guess we should change to > const uint8* data(size_t plane) const; > uint8* data(size_t plane); > const uint8* visible_data(size_t plane) const; > uint8* visible_data(size_t plane);
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
Patchset #2 (id:100001) has been deleted
Patchset #2 (id:120001) has been deleted
Patchset #2 (id:140001) has been deleted
tommi: Please take another look, I have fixed all your comments. I have also reverted the scaling back to this file when it is needed. I tried to give that responsibility to the CoordinatedVideoAdapter in the previous patch, but it didn't work out.
Nice. Can you please remove my todo in video_destination_handler.cc about this as well? https://codereview.chromium.org/604743005/diff/160001/content/renderer/media/... File content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc (right): https://codereview.chromium.org/604743005/diff/160001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:40: virtual VideoFrame* Copy() const OVERRIDE { use new c++ override instead of OVERRIDE here and elsewhere. https://codereview.chromium.org/604743005/diff/160001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:143: // TODO(magjed): Refactor into base class. I don't think this is used? Return NOTIMPLEMENTED and length 0 instead. https://codereview.chromium.org/604743005/diff/160001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:211: virtual VideoFrame* CreateEmptyFrame(int w, is this really used? https://codereview.chromium.org/604743005/diff/160001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:268: scoped_refptr<media::VideoFrame> video_frame = frame_; frame_.Release to avoid holding a reference when we no longer need it. The reason is that we would like to free the buffer held by the video capture shared memory pool asap and don't wait for the next frame if it can be avoided. https://codereview.chromium.org/604743005/diff/160001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:269: // Check if scaling is needed. SHould we add a todo to further investigate what you started investigating? Ie- do this scaling in libjingle instead? https://codereview.chromium.org/604743005/diff/160001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:408: // libJingle have no assumptions on what thread this signal come from. Can you please remove this line "libJingle have no assumptions on what thread this signal come from." It turned out to be not true. This is on the worker thread and that is how it should be. https://codereview.chromium.org/604743005/diff/160001/content/renderer/media/... File content/renderer/media/webrtc/webrtc_video_capturer_adapter.h (right): https://codereview.chromium.org/604743005/diff/160001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter.h:19: class MediaVideoFrameFactory; nit: forward declare just before you use it line 63 instead.
Patchset #3 (id:180001) has been deleted
perkj: PTAL. https://codereview.chromium.org/604743005/diff/160001/content/renderer/media/... File content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc (right): https://codereview.chromium.org/604743005/diff/160001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:40: virtual VideoFrame* Copy() const OVERRIDE { On 2014/10/16 07:55:09, perkj wrote: > use new c++ override instead of OVERRIDE here and elsewhere. Done. https://codereview.chromium.org/604743005/diff/160001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:143: // TODO(magjed): Refactor into base class. On 2014/10/16 07:55:09, perkj wrote: > I don't think this is used? Return NOTIMPLEMENTED and length 0 instead. It is probably not used, but it is called in a couple of places in the Chromium code base. I would rather keep this bloat than risking a crash. https://codereview.chromium.org/604743005/diff/160001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:211: virtual VideoFrame* CreateEmptyFrame(int w, On 2014/10/16 07:55:09, perkj wrote: > is this really used? Yes, it is used in the VideoAdapter to allocate frames when scaling is needed. https://codereview.chromium.org/604743005/diff/160001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:268: scoped_refptr<media::VideoFrame> video_frame = frame_; On 2014/10/16 07:55:09, perkj wrote: > frame_.Release to avoid holding a reference when we no longer need it. The > reason is that we would like to free the buffer held by the video capture shared > memory pool asap and don't wait for the next frame if it can be avoided. Done, I have added it as a separate call as this function is in a const context. https://codereview.chromium.org/604743005/diff/160001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:269: // Check if scaling is needed. On 2014/10/16 07:55:09, perkj wrote: > SHould we add a todo to further investigate what you started investigating? Ie- > do this scaling in libjingle instead? I would prefer to keep this code and instead refactor VideoAdapter::AdaptFrame to never do any work on the pixel data, but just return if the frame should be dropped, or what resolution the frame should be scaled to. That resolution can then be passed to the VideoFrameFactory::CreateAliasedFrame. I.e. the TODO should be placed in the VideoAdapter and not here. https://codereview.chromium.org/604743005/diff/160001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:408: // libJingle have no assumptions on what thread this signal come from. On 2014/10/16 07:55:09, perkj wrote: > Can you please remove this line "libJingle have no assumptions on what thread > this signal come from." > It turned out to be not true. This is on the worker thread and that is how it > should be. Done. https://codereview.chromium.org/604743005/diff/160001/content/renderer/media/... File content/renderer/media/webrtc/webrtc_video_capturer_adapter.h (right): https://codereview.chromium.org/604743005/diff/160001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter.h:19: class MediaVideoFrameFactory; On 2014/10/16 07:55:09, perkj wrote: > nit: forward declare just before you use it line 63 instead. I want to have it in an anonymous namespace and not in the class namespace.
https://codereview.chromium.org/604743005/diff/160001/content/renderer/media/... File content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc (right): https://codereview.chromium.org/604743005/diff/160001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:143: // TODO(magjed): Refactor into base class. On 2014/10/16 10:35:47, magjed wrote: > On 2014/10/16 07:55:09, perkj wrote: > > I don't think this is used? Return NOTIMPLEMENTED and length 0 instead. > > It is probably not used, but it is called in a couple of places in the Chromium > code base. I would rather keep this bloat than risking a crash. Acknowledged. https://codereview.chromium.org/604743005/diff/160001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:211: virtual VideoFrame* CreateEmptyFrame(int w, On 2014/10/16 10:35:47, magjed wrote: > On 2014/10/16 07:55:09, perkj wrote: > > is this really used? > > Yes, it is used in the VideoAdapter to allocate frames when scaling is needed. Acknowledged. https://codereview.chromium.org/604743005/diff/200001/content/renderer/media/... File content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc (right): https://codereview.chromium.org/604743005/diff/200001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:141: return 0; btw - is this ok on Android? https://codereview.chromium.org/604743005/diff/200001/content/renderer/media/... File content/renderer/media/webrtc/webrtc_video_capturer_adapter.h (right): https://codereview.chromium.org/604743005/diff/200001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter.h:19: namespace { not compatible with the style guide. https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... I would say, make this a private class inside WebRtcVideoCapturerAdapter. Otherwise you will have to put it inside the conent ns and give it a name that indicates its only meaningfull in the scope of WebRtcVideoCapturerAdapter.
perkj: PTAL. https://codereview.chromium.org/604743005/diff/200001/content/renderer/media/... File content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc (right): https://codereview.chromium.org/604743005/diff/200001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:141: return 0; On 2014/10/16 13:47:31, perkj wrote: > btw - is this ok on Android? You mean if I physically rotate the Android device this value should be changed? I think that logic is handled elsewhere. https://codereview.chromium.org/604743005/diff/200001/content/renderer/media/... File content/renderer/media/webrtc/webrtc_video_capturer_adapter.h (right): https://codereview.chromium.org/604743005/diff/200001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter.h:19: namespace { On 2014/10/16 13:47:31, perkj wrote: > not compatible with the style guide. > > https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... > > I would say, make this a private class inside WebRtcVideoCapturerAdapter. > Otherwise you will have to put it inside the conent ns and give it a name that > indicates its only meaningfull in the scope of WebRtcVideoCapturerAdapter. Done.
looks pretty good. just want to double check on one thing. https://codereview.chromium.org/604743005/diff/220001/content/renderer/media/... File content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc (right): https://codereview.chromium.org/604743005/diff/220001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:12: #include "third_party/libjingle/source/talk/media/base/videoframe.h" can you remind me tomorrow to ask you about this? :) including directly from libjingle may have some non obvious consequences so I just want to double check.
lgtm
perkj@chromium.org changed reviewers: + scherkus@chromium.org
lgtm, adding scherkus for media/video_frame
some basic unit tests verifying the math / edge cases would be nice, especially when it comes to edge cases between video frame formats and dealing with rounding https://codereview.chromium.org/604743005/diff/220001/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/604743005/diff/220001/media/base/video_frame.... media/base/video_frame.cc:904: const int offset_x = RoundDown(visible_rect_.x(), 2); could you add a comment briefly explaining the math we're doing here? (e.g., why round down to 2? if x is 1 and I have YV24 content, why will you give me 0 instead of 1?) https://codereview.chromium.org/604743005/diff/220001/media/base/video_frame.... media/base/video_frame.cc:912: return coded_data + offset_y / 2 * coded_stride + offset_x / 2; are you sure the division-by-two makes sense even for YV16 and YV24? https://codereview.chromium.org/604743005/diff/220001/media/base/video_frame.... media/base/video_frame.cc:913: default: can you list these cases explicitly? if we ever change the enum the compiler will whine and the developer will be forced to assess whether this code needs to be changed https://codereview.chromium.org/604743005/diff/220001/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/604743005/diff/220001/media/base/video_frame.... media/base/video_frame.h:269: uint8* data(size_t plane); do we need both the const and non-const versions? could we get away with just having the uint8* version, or does that break things? AFAICT webrtc_video_capturer_adapter.cc should still compile upgrading a uint8* -> const uint8* https://codereview.chromium.org/604743005/diff/220001/media/base/video_frame.... media/base/video_frame.h:273: const uint8* visible_data(size_t plane) const; ditto
(I should say, though, I'm a huge fan of eliminating frame copies so kudos for doing this!!!)
Patchset #5 (id:240001) has been deleted
Patchset #5 (id:260001) has been deleted
scherkus: Can you please take another look? https://codereview.chromium.org/604743005/diff/220001/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/604743005/diff/220001/media/base/video_frame.... media/base/video_frame.cc:904: const int offset_x = RoundDown(visible_rect_.x(), 2); On 2014/10/20 19:30:36, scherkus wrote: > could you add a comment briefly explaining the math we're doing here? (e.g., why > round down to 2? if x is 1 and I have YV24 content, why will you give me 0 > instead of 1?) This is removed now. See comment below. https://codereview.chromium.org/604743005/diff/220001/media/base/video_frame.... media/base/video_frame.cc:912: return coded_data + offset_y / 2 * coded_stride + offset_x / 2; On 2014/10/20 19:30:36, scherkus wrote: > are you sure the division-by-two makes sense even for YV16 and YV24? You are right, it does not make sense. It is hardcoded for I420 at the moment. I didn't want to fix it by adding another huge switch, so I implemented some helper functions and took the liberty to clean up the rest of the code as well. https://codereview.chromium.org/604743005/diff/220001/media/base/video_frame.... media/base/video_frame.cc:913: default: On 2014/10/20 19:30:36, scherkus wrote: > can you list these cases explicitly? if we ever change the enum the compiler > will whine and the developer will be forced to assess whether this code needs to > be changed Done. Not applicable anymore. https://codereview.chromium.org/604743005/diff/220001/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/604743005/diff/220001/media/base/video_frame.... media/base/video_frame.h:269: uint8* data(size_t plane); On 2014/10/20 19:30:36, scherkus wrote: > do we need both the const and non-const versions? > > could we get away with just having the uint8* version, or does that break > things? > > AFAICT webrtc_video_capturer_adapter.cc should still compile upgrading a uint8* > -> const uint8* Everything will compile and work using the old declaration: uint8* data(size_t plane) const but returning a non-const pointer to internal data from a const method goes against the style guide. So that's the reason for having two methods: const method returning const, and non-const method returning non-const. Reference: http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Use_of_const "Declare methods to be const whenever possible. Accessors should almost always be const. Other methods should be const if they do not modify any data members, do not call any non-const methods, and do not return a non-const pointer or non-const reference to a data member." https://codereview.chromium.org/604743005/diff/220001/media/base/video_frame.... media/base/video_frame.h:273: const uint8* visible_data(size_t plane) const; On 2014/10/20 19:30:36, scherkus wrote: > ditto See above.
nice cleanup! you shaved almost ~200 lines de-duping all those switch/case statements :) lgtm w/ nits https://codereview.chromium.org/604743005/diff/280001/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/604743005/diff/280001/media/base/video_frame.... media/base/video_frame.cc:122: const gfx::Size new_coded_size = AdjustCodedSize(format, coded_size); general tip for the future: try to avoid making these unrelated changed in code reviews -- it can make things a bit more confusing when reviewing https://codereview.chromium.org/604743005/diff/280001/media/base/video_frame.... media/base/video_frame.cc:564: DCHECK(width % subsample.width() == 0 && height % subsample.height() == 0); if this fails we won't know which condition failed you should either split into two consecutive DCHECKs or you can emit a string containing the information e.g., DCHECK(...) << "blah blah blah" << width; https://codereview.chromium.org/604743005/diff/280001/media/base/video_frame.... media/base/video_frame.cc:725: offset.y() % subsample.height() == 0); ditto for DCHECK failure
scherkus: Please take another look. Also, I should mention that I changed the original behaviour in two places in the cleanup: 1. The DCHECK in RoundUp didn't previously catch alignment=0, and would return: (value + (alignment - 1)) & ~(alignment - 1) = (value + (0 - 1)) & ~(0 - 1) = (value + 0xFF...) & ~0xFF... = (value + 0xFF...) & 0 = 0, for all values. 2. RowBytes(kUVPlane, NV12, width) previously underestimated the size for odd widths, e.g. RowBytes(kUVPlane, NV12, 1) would return 1 instead of 2, because it would return width and not RoundUp(width, 2). It is fixed now. https://codereview.chromium.org/604743005/diff/280001/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/604743005/diff/280001/media/base/video_frame.... media/base/video_frame.cc:122: const gfx::Size new_coded_size = AdjustCodedSize(format, coded_size); On 2014/10/22 23:01:49, scherkus wrote: > general tip for the future: try to avoid making these unrelated changed in code > reviews -- it can make things a bit more confusing when reviewing Yeah, sorry about that. I will keep that in mind in the future. https://codereview.chromium.org/604743005/diff/280001/media/base/video_frame.... media/base/video_frame.cc:564: DCHECK(width % subsample.width() == 0 && height % subsample.height() == 0); On 2014/10/22 23:01:49, scherkus wrote: > if this fails we won't know which condition failed > > you should either split into two consecutive DCHECKs or you can emit a string > containing the information > > e.g., DCHECK(...) << "blah blah blah" << width; Done. https://codereview.chromium.org/604743005/diff/280001/media/base/video_frame.... media/base/video_frame.cc:725: offset.y() % subsample.height() == 0); On 2014/10/22 23:01:50, scherkus wrote: > ditto for DCHECK failure Done.
still lgtm (in case you haven't ran into it before, a "lgtm with nits" is basically saying go ahead and commit after making some last minor changes -- you don't need to wait for another round trip, which is nice considering we're many time zones apart!)
The CQ bit was checked by magjed@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/604743005/300001
Message was sent while issue was closed.
Committed patchset #6 (id:300001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/4087adc5677bb8dd7f414c0afafc8f4215ce0077 Cr-Commit-Position: refs/heads/master@{#300959} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
