Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
964293002:
VideoCaptureImpl & relatives small cleanup. (Closed)
|
Created:
5 years, 9 months ago by mcasas 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. |
DescriptionVideoCaptureImpl & relatives small cleanup.
Including:
- use auto, const auto, range based for loops.
- loop variable renaming due to this change.
- gfx::Size() has a GetArea() method, use it.
- removal of unnnecessary {}
- argument formatting when it fits in one line
- VideoTrackAdapter::VideoFrameResolutionAdapter::DoDeliverFrame() is not virtual.
BUG=
Committed: https://crrev.com/e9e5cc288d491895cafcf28d699b4920e441f04e
Cr-Commit-Position: refs/heads/master@{#319111}
Patch Set 1 #
Total comments: 8
Patch Set 2 : s/it./it->/ that made bots sad #Patch Set 3 : emircan@s comments #
Total comments: 22
Patch Set 4 : wolenetz@ comments #Patch Set 5 : Rebase #Patch Set 6 : Reverted changes in VideoCaptureImpl::OnDelegateAdded() - client must be removed before StartCapure… #
Messages
Total messages: 33 (20 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:50003) has been deleted
Patchset #1 (id:70001) has been deleted
Patchset #1 (id:90001) has been deleted
Patchset #1 (id:110001) has been deleted
Patchset #1 (id:130001) has been deleted
Patchset #1 (id:150001) has been deleted
Patchset #1 (id:170001) has been deleted
emircan@chromium.org changed reviewers: + emircan@chromium.org
https://codereview.chromium.org/964293002/diff/190001/content/renderer/media/... File content/renderer/media/media_stream_video_source.cc (right): https://codereview.chromium.org/964293002/diff/190001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:306: [&](const media::VideoCaptureFormat& f1, Default lambda captures are not allowed in the styleguide https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Lambda_expre.... Consider changing to [&area] or [=area]. https://codereview.chromium.org/964293002/diff/190001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:310: }); Overall comparison complexity is O(N) for min_element. Unless there is a compiler optimization, the absolute diff for a format may be calculated more than once for comparisons sake. We can calculate the differences first, and use the default comparator as well. Not sure if it would be significant difference though. https://codereview.chromium.org/964293002/diff/190001/content/renderer/media/... File content/renderer/media/video_capture_impl.cc (right): https://codereview.chromium.org/964293002/diff/190001/content/renderer/media/... content/renderer/media/video_capture_impl.cc:362: const ClientInfo client_info = it.second; You can consider assigning by reference. https://codereview.chromium.org/964293002/diff/190001/content/renderer/media/... File content/renderer/media/video_capture_message_filter.cc (right): https://codereview.chromium.org/964293002/diff/190001/content/renderer/media/... content/renderer/media/video_capture_message_filter.cc:82: delegates_[it.first] = it.second; If map::insert can be used if the incoming pending_delegates_ keys are going to be unique.
lgtm % minor issues addressed above.
mcasas@chromium.org changed reviewers: + wolenetz@chromium.org
wolenetz@ OWNERS RS/PTAL. https://codereview.chromium.org/964293002/diff/190001/content/renderer/media/... File content/renderer/media/media_stream_video_source.cc (right): https://codereview.chromium.org/964293002/diff/190001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:306: [&](const media::VideoCaptureFormat& f1, On 2015/03/02 19:29:07, emircan wrote: > Default lambda captures are not allowed in the styleguide > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Lambda_expre.... > Consider changing to [&area] or [=area]. Done. https://codereview.chromium.org/964293002/diff/190001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:310: }); On 2015/03/02 19:29:07, emircan wrote: > Overall comparison complexity is O(N) for min_element. Unless there is a > compiler optimization, the absolute diff for a format may be calculated more > than once for comparisons sake. We can calculate the differences first, and use > the default comparator as well. Not sure if it would be significant difference > though. Hmm you're right. I was too centered in making the algorithm explicit. Creating an array of differences and then choosing the best would still mean walking the array twice, so still worse than the original solution. Reverting in part. https://codereview.chromium.org/964293002/diff/190001/content/renderer/media/... File content/renderer/media/video_capture_impl.cc (right): https://codereview.chromium.org/964293002/diff/190001/content/renderer/media/... content/renderer/media/video_capture_impl.cc:362: const ClientInfo client_info = it.second; On 2015/03/02 19:29:07, emircan wrote: > You can consider assigning by reference. The original code did make a deep copy before removing the iterator-pointed-item from |clients_pending_on_filter_| and only then calling StartCapture(). (I could not take a reference since it'd be removed before being used in StartCapture()) The whole idiom smells like a hidden race or at the very least some kind of hard to grok dependency, Let's try StartCapture() on each ClientInfo entry and then clients_pending_on_filter_.clear(), let's see what the bots say. https://codereview.chromium.org/964293002/diff/190001/content/renderer/media/... File content/renderer/media/video_capture_message_filter.cc (right): https://codereview.chromium.org/964293002/diff/190001/content/renderer/media/... content/renderer/media/video_capture_message_filter.cc:82: delegates_[it.first] = it.second; On 2015/03/02 19:29:07, emircan wrote: > If map::insert can be used if the incoming pending_delegates_ keys are going to > be unique. map::insert() never replaces while operator[] might do. I'm not sure of the semantics but the key should be unique or get the it.second replaced... I propose we leave it as is in this cleanup-only CL.
PS3 rsl-g-t-m % nits, fixing bot-compilation-failure (covered in my s/=/:/ comment, I think), and: please add an rtc OWNER such as perkj@ or tommi@ to this review, since more than just mechanical changes are included to rtc media code. https://codereview.chromium.org/964293002/diff/230001/content/renderer/media/... File content/renderer/media/media_stream_video_source.cc (right): https://codereview.chromium.org/964293002/diff/230001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:306: for (; it != formats.end(); ++it) { nit: ISTM this could be a candidate for a range-based for loop. https://codereview.chromium.org/964293002/diff/230001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:307: const int diff = abs(area - it->frame_size.GetArea()); nit: please update the CL description to note there is more than just auto/const auto/range-based-for stuff contained in the CL like this (and the removal of unnecessary {}, above). https://codereview.chromium.org/964293002/diff/230001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:348: for (const auto& constraint : kSupportedConstraints) { each of kSupportedConstraints is a char*. so, const auto* (or, just const char*)? Specifically, from https://chromium-cpp.appspot.com/, "For pointers, use for (auto* ...) to make clear that the copy of the loop variable is intended, and only a pointer is copied" https://codereview.chromium.org/964293002/diff/230001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:492: for (const auto& request_it : requested_constraints_) { nit: s/_it// https://codereview.chromium.org/964293002/diff/230001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:494: request_it.constraints; nit: s/_it// https://codereview.chromium.org/964293002/diff/230001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:546: for (const auto& it : callbacks) { nit: s/it/callback/ or s/it/requestedConstraint/ for readability? I don't know this code that well, but "it" seems to be not very explanatory, especially when "it" is an auto&. https://codereview.chromium.org/964293002/diff/230001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:592: for (const auto& it : tracks_) nit: s/it/something-that-describes-expected-type-of-it-like-maybe-track/ ? https://codereview.chromium.org/964293002/diff/230001/content/renderer/media/... File content/renderer/media/video_capture_impl.cc (right): https://codereview.chromium.org/964293002/diff/230001/content/renderer/media/... content/renderer/media/video_capture_impl.cc:256: for (const auto& it : clients_) nit: s/it/something-more-type-descriptive/ ? https://codereview.chromium.org/964293002/diff/230001/content/renderer/media/... content/renderer/media/video_capture_impl.cc:284: for (const auto& it : clients_) nit: ditto here and elsewhere in this CL. https://codereview.chromium.org/964293002/diff/230001/content/renderer/media/... content/renderer/media/video_capture_impl.cc:359: for (const auto& it = clients_pending_on_filter_) { s/=/:/ ? I don't know this code, this for loop now seems like a locally scoped variable definition *only*, and not a loop at all. https://codereview.chromium.org/964293002/diff/230001/content/renderer/media/... File content/renderer/media/video_track_adapter.cc (right): https://codereview.chromium.org/964293002/diff/230001/content/renderer/media/... content/renderer/media/video_track_adapter.cc:94: void DoDeliverFrame(const scoped_refptr<media::VideoFrame>& frame, nit: ditto update CL description to include this, which is outside of the otherwise mechanical C++11 range-based-for-loop/auto/etc currently in the description.
mcasas@chromium.org changed reviewers: + perkj@chromium.org
perkj@ OWNERS RS plz https://codereview.chromium.org/964293002/diff/230001/content/renderer/media/... File content/renderer/media/media_stream_video_source.cc (right): https://codereview.chromium.org/964293002/diff/230001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:306: for (; it != formats.end(); ++it) { On 2015/03/02 23:51:08, wolenetz wrote: > nit: ISTM this could be a candidate for a range-based for loop. It seems so but if you see previous Patchsets it just didn't bend to it... https://codereview.chromium.org/964293002/diff/230001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:307: const int diff = abs(area - it->frame_size.GetArea()); On 2015/03/02 23:51:09, wolenetz wrote: > nit: please update the CL description to note there is more than just auto/const > auto/range-based-for stuff contained in the CL like this (and the removal of > unnecessary {}, above). Done. https://codereview.chromium.org/964293002/diff/230001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:348: for (const auto& constraint : kSupportedConstraints) { On 2015/03/02 23:51:09, wolenetz wrote: > each of kSupportedConstraints is a char*. so, const auto* (or, just const > char*)? Specifically, from https://chromium-cpp.appspot.com/, "For pointers, use > for (auto* ...) to make clear that the copy of the loop variable is intended, > and only a pointer is copied" Done. https://codereview.chromium.org/964293002/diff/230001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:492: for (const auto& request_it : requested_constraints_) { On 2015/03/02 23:51:09, wolenetz wrote: > nit: s/_it// Done. https://codereview.chromium.org/964293002/diff/230001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:494: request_it.constraints; On 2015/03/02 23:51:09, wolenetz wrote: > nit: s/_it// Done. https://codereview.chromium.org/964293002/diff/230001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:546: for (const auto& it : callbacks) { On 2015/03/02 23:51:09, wolenetz wrote: > nit: s/it/callback/ or s/it/requestedConstraint/ for readability? I don't know > this code that well, but "it" seems to be not very explanatory, especially when > "it" is an auto&. I think |request| is good enough. I'll let the RTC owners decide otherwise. https://codereview.chromium.org/964293002/diff/230001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:592: for (const auto& it : tracks_) On 2015/03/02 23:51:09, wolenetz wrote: > nit: s/it/something-that-describes-expected-type-of-it-like-maybe-track/ ? Done. https://codereview.chromium.org/964293002/diff/230001/content/renderer/media/... File content/renderer/media/video_capture_impl.cc (right): https://codereview.chromium.org/964293002/diff/230001/content/renderer/media/... content/renderer/media/video_capture_impl.cc:256: for (const auto& it : clients_) On 2015/03/02 23:51:09, wolenetz wrote: > nit: s/it/something-more-type-descriptive/ ? Done. https://codereview.chromium.org/964293002/diff/230001/content/renderer/media/... content/renderer/media/video_capture_impl.cc:284: for (const auto& it : clients_) On 2015/03/02 23:51:09, wolenetz wrote: > nit: ditto here and elsewhere in this CL. Done. https://codereview.chromium.org/964293002/diff/230001/content/renderer/media/... content/renderer/media/video_capture_impl.cc:359: for (const auto& it = clients_pending_on_filter_) { On 2015/03/02 23:51:09, wolenetz wrote: > s/=/:/ ? I don't know this code, this for loop now seems like a locally scoped > variable definition *only*, and not a loop at all. Done. https://codereview.chromium.org/964293002/diff/230001/content/renderer/media/... File content/renderer/media/video_track_adapter.cc (right): https://codereview.chromium.org/964293002/diff/230001/content/renderer/media/... content/renderer/media/video_track_adapter.cc:94: void DoDeliverFrame(const scoped_refptr<media::VideoFrame>& frame, On 2015/03/02 23:51:09, wolenetz wrote: > nit: ditto update CL description to include this, which is outside of the > otherwise mechanical C++11 range-based-for-loop/auto/etc currently in the > description. Acknowledged.
On 2015/03/03 15:40:52, mcasas wrote: > perkj@ OWNERS RS plz > > https://codereview.chromium.org/964293002/diff/230001/content/renderer/media/... > File content/renderer/media/media_stream_video_source.cc (right): > > https://codereview.chromium.org/964293002/diff/230001/content/renderer/media/... > content/renderer/media/media_stream_video_source.cc:306: for (; it != > formats.end(); ++it) { > On 2015/03/02 23:51:08, wolenetz wrote: > > nit: ISTM this could be a candidate for a range-based for loop. > > It seems so but if you see previous Patchsets it > just didn't bend to it... > > https://codereview.chromium.org/964293002/diff/230001/content/renderer/media/... > content/renderer/media/media_stream_video_source.cc:307: const int diff = > abs(area - it->frame_size.GetArea()); > On 2015/03/02 23:51:09, wolenetz wrote: > > nit: please update the CL description to note there is more than just > auto/const > > auto/range-based-for stuff contained in the CL like this (and the removal of > > unnecessary {}, above). > > Done. > > https://codereview.chromium.org/964293002/diff/230001/content/renderer/media/... > content/renderer/media/media_stream_video_source.cc:348: for (const auto& > constraint : kSupportedConstraints) { > On 2015/03/02 23:51:09, wolenetz wrote: > > each of kSupportedConstraints is a char*. so, const auto* (or, just const > > char*)? Specifically, from https://chromium-cpp.appspot.com/, "For pointers, > use > > for (auto* ...) to make clear that the copy of the loop variable is intended, > > and only a pointer is copied" > > Done. > > https://codereview.chromium.org/964293002/diff/230001/content/renderer/media/... > content/renderer/media/media_stream_video_source.cc:492: for (const auto& > request_it : requested_constraints_) { > On 2015/03/02 23:51:09, wolenetz wrote: > > nit: s/_it// > > Done. > > https://codereview.chromium.org/964293002/diff/230001/content/renderer/media/... > content/renderer/media/media_stream_video_source.cc:494: request_it.constraints; > On 2015/03/02 23:51:09, wolenetz wrote: > > nit: s/_it// > > Done. > > https://codereview.chromium.org/964293002/diff/230001/content/renderer/media/... > content/renderer/media/media_stream_video_source.cc:546: for (const auto& it : > callbacks) { > On 2015/03/02 23:51:09, wolenetz wrote: > > nit: s/it/callback/ or s/it/requestedConstraint/ for readability? I don't know > > this code that well, but "it" seems to be not very explanatory, especially > when > > "it" is an auto&. > > I think |request| is good enough. > I'll let the RTC owners decide otherwise. > > https://codereview.chromium.org/964293002/diff/230001/content/renderer/media/... > content/renderer/media/media_stream_video_source.cc:592: for (const auto& it : > tracks_) > On 2015/03/02 23:51:09, wolenetz wrote: > > nit: s/it/something-that-describes-expected-type-of-it-like-maybe-track/ ? > > Done. > > https://codereview.chromium.org/964293002/diff/230001/content/renderer/media/... > File content/renderer/media/video_capture_impl.cc (right): > > https://codereview.chromium.org/964293002/diff/230001/content/renderer/media/... > content/renderer/media/video_capture_impl.cc:256: for (const auto& it : > clients_) > On 2015/03/02 23:51:09, wolenetz wrote: > > nit: s/it/something-more-type-descriptive/ ? > > Done. > > https://codereview.chromium.org/964293002/diff/230001/content/renderer/media/... > content/renderer/media/video_capture_impl.cc:284: for (const auto& it : > clients_) > On 2015/03/02 23:51:09, wolenetz wrote: > > nit: ditto here and elsewhere in this CL. > > Done. > > https://codereview.chromium.org/964293002/diff/230001/content/renderer/media/... > content/renderer/media/video_capture_impl.cc:359: for (const auto& it = > clients_pending_on_filter_) { > On 2015/03/02 23:51:09, wolenetz wrote: > > s/=/:/ ? I don't know this code, this for loop now seems like a locally scoped > > variable definition *only*, and not a loop at all. > > Done. > > https://codereview.chromium.org/964293002/diff/230001/content/renderer/media/... > File content/renderer/media/video_track_adapter.cc (right): > > https://codereview.chromium.org/964293002/diff/230001/content/renderer/media/... > content/renderer/media/video_track_adapter.cc:94: void DoDeliverFrame(const > scoped_refptr<media::VideoFrame>& frame, > On 2015/03/02 23:51:09, wolenetz wrote: > > nit: ditto update CL description to include this, which is outside of the > > otherwise mechanical C++11 range-based-for-loop/auto/etc currently in the > > description. > > Acknowledged. thanks lgtm
The CQ bit was checked by mcasas@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from emircan@chromium.org Link to the patchset: https://codereview.chromium.org/964293002/#ps250001 (title: "wolenetz@ comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/964293002/250001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by mcasas@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from perkj@chromium.org, emircan@chromium.org Link to the patchset: https://codereview.chromium.org/964293002/#ps290001 (title: "Reverted changes in VideoCaptureImpl::OnDelegateAdded() - client must be removed before StartCapure…")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/964293002/290001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
The CQ bit was checked by mcasas@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/964293002/290001
Message was sent while issue was closed.
Committed patchset #6 (id:290001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/e9e5cc288d491895cafcf28d699b4920e441f04e Cr-Commit-Position: refs/heads/master@{#319111} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
