Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1423843003:
Make content::BufferQueue use scoped ptrs for AllocatedSurfaces (Closed)
|
Created:
5 years, 1 month ago by ccameron Modified:
5 years ago CC:
chromium-reviews, danakj+watch_chromium.org, darin-cc_chromium.org, jam, jbauman+watch_chromium.org, kalyank, piman+watch_chromium.org, sievers+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@surfaceless_only Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake content::BufferQueue use scoped ptrs for AllocatedSurfaces
BUG=546795
Committed: https://crrev.com/3a36c0065435bf22a9fda905d6e7a47924a336d8
Cr-Commit-Position: refs/heads/master@{#364833}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Use vector<scoped_ptr<T>> #
Total comments: 22
Patch Set 3 : Incorporate review foodback #
Total comments: 12
Patch Set 4 : Moar tests #Patch Set 5 : And the rest of the feedback #
Total comments: 8
Patch Set 6 : More feedback #
Messages
Total messages: 21 (5 generated)
ccameron@chromium.org changed reviewers: + danakj@chromium.org, reveman@chromium.org
Question for yous folks: This violates DEPS cause ScopedPtrVector and ScopedPtrDeq are cc:: only, with a comment saying "put it in base before using". (probably should have noticed that before writing the whole patch) I could do that. Or, I could ignore it, and wait until enough C++14 support is in to allow a std::deque<scoped_ptr<T>>. I'm inclined to do the latter, but I wanted a quick opinion poll on the topic. The reason why I want to do the latter is that ScopedPtrVector and ScopedPtrDeque would be better off dead (example: "take_back" means different things for the two of them, example: there's no swap for entire ScopedPtrDeques, etc).
https://codereview.chromium.org/1423843003/diff/1/content/browser/compositor/... File content/browser/compositor/buffer_queue.cc (right): https://codereview.chromium.org/1423843003/diff/1/content/browser/compositor/... content/browser/compositor/buffer_queue.cc:209: void BufferQueue::FreeSurface(scoped_ptr<AllocatedSurface> surface) { can we move all this into the AllocatedSurface dtor instead? that way we don't have to worry about failing to call this and leaking the resources.. https://codereview.chromium.org/1423843003/diff/1/content/browser/compositor/... content/browser/compositor/buffer_queue.cc:226: available_surfaces_.take_back().Pass(); hm, is .Pass() really needed here? https://codereview.chromium.org/1423843003/diff/1/content/browser/compositor/... content/browser/compositor/buffer_queue.cc:235: return scoped_ptr<AllocatedSurface>(); nullptr https://codereview.chromium.org/1423843003/diff/1/content/browser/compositor/... content/browser/compositor/buffer_queue.cc:248: return scoped_ptr<AllocatedSurface>(); nullptr https://codereview.chromium.org/1423843003/diff/1/content/browser/compositor/... content/browser/compositor/buffer_queue.cc:258: return scoped_ptr<AllocatedSurface>(); nullptr https://codereview.chromium.org/1423843003/diff/1/content/browser/compositor/... content/browser/compositor/buffer_queue.cc:260: allocated_count_++; is this member needed? is this just not the sum of the size of the vectors? https://codereview.chromium.org/1423843003/diff/1/content/browser/compositor/... content/browser/compositor/buffer_queue.cc:263: return scoped_ptr<AllocatedSurface>( make_scoped_ptr(new All...)? https://codereview.chromium.org/1423843003/diff/1/content/browser/compositor/... File content/browser/compositor/buffer_queue.h (right): https://codereview.chromium.org/1423843003/diff/1/content/browser/compositor/... content/browser/compositor/buffer_queue.h:104: cc::ScopedPtrVector<AllocatedSurface> available_surfaces_; could we use base::ScopedVector instead?
On 2015/10/27 17:03:44, reveman wrote: > https://codereview.chromium.org/1423843003/diff/1/content/browser/compositor/... > File content/browser/compositor/buffer_queue.cc (right): > > https://codereview.chromium.org/1423843003/diff/1/content/browser/compositor/... > content/browser/compositor/buffer_queue.cc:209: void > BufferQueue::FreeSurface(scoped_ptr<AllocatedSurface> surface) { > can we move all this into the AllocatedSurface dtor instead? that way we don't > have to worry about failing to call this and leaking the resources.. > > https://codereview.chromium.org/1423843003/diff/1/content/browser/compositor/... > content/browser/compositor/buffer_queue.cc:226: > available_surfaces_.take_back().Pass(); > hm, is .Pass() really needed here? > > https://codereview.chromium.org/1423843003/diff/1/content/browser/compositor/... > content/browser/compositor/buffer_queue.cc:235: return > scoped_ptr<AllocatedSurface>(); > nullptr > > https://codereview.chromium.org/1423843003/diff/1/content/browser/compositor/... > content/browser/compositor/buffer_queue.cc:248: return > scoped_ptr<AllocatedSurface>(); > nullptr > > https://codereview.chromium.org/1423843003/diff/1/content/browser/compositor/... > content/browser/compositor/buffer_queue.cc:258: return > scoped_ptr<AllocatedSurface>(); > nullptr > > https://codereview.chromium.org/1423843003/diff/1/content/browser/compositor/... > content/browser/compositor/buffer_queue.cc:260: allocated_count_++; > is this member needed? is this just not the sum of the size of the vectors? > > https://codereview.chromium.org/1423843003/diff/1/content/browser/compositor/... > content/browser/compositor/buffer_queue.cc:263: return > scoped_ptr<AllocatedSurface>( > make_scoped_ptr(new All...)? > > https://codereview.chromium.org/1423843003/diff/1/content/browser/compositor/... > File content/browser/compositor/buffer_queue.h (right): > > https://codereview.chromium.org/1423843003/diff/1/content/browser/compositor/... > content/browser/compositor/buffer_queue.h:104: > cc::ScopedPtrVector<AllocatedSurface> available_surfaces_; > could we use base::ScopedVector instead? Oh -- what are your thoughts on having me shelve this until we have C++14 support for std::vector<scoped_ptr<T>> and deque? (as-is, this violates deps).
On 2015/10/27 at 17:06:13, ccameron wrote: > On 2015/10/27 17:03:44, reveman wrote: > > https://codereview.chromium.org/1423843003/diff/1/content/browser/compositor/... > > File content/browser/compositor/buffer_queue.cc (right): > > > > https://codereview.chromium.org/1423843003/diff/1/content/browser/compositor/... > > content/browser/compositor/buffer_queue.cc:209: void > > BufferQueue::FreeSurface(scoped_ptr<AllocatedSurface> surface) { > > can we move all this into the AllocatedSurface dtor instead? that way we don't > > have to worry about failing to call this and leaking the resources.. > > > > https://codereview.chromium.org/1423843003/diff/1/content/browser/compositor/... > > content/browser/compositor/buffer_queue.cc:226: > > available_surfaces_.take_back().Pass(); > > hm, is .Pass() really needed here? > > > > https://codereview.chromium.org/1423843003/diff/1/content/browser/compositor/... > > content/browser/compositor/buffer_queue.cc:235: return > > scoped_ptr<AllocatedSurface>(); > > nullptr > > > > https://codereview.chromium.org/1423843003/diff/1/content/browser/compositor/... > > content/browser/compositor/buffer_queue.cc:248: return > > scoped_ptr<AllocatedSurface>(); > > nullptr > > > > https://codereview.chromium.org/1423843003/diff/1/content/browser/compositor/... > > content/browser/compositor/buffer_queue.cc:258: return > > scoped_ptr<AllocatedSurface>(); > > nullptr > > > > https://codereview.chromium.org/1423843003/diff/1/content/browser/compositor/... > > content/browser/compositor/buffer_queue.cc:260: allocated_count_++; > > is this member needed? is this just not the sum of the size of the vectors? > > > > https://codereview.chromium.org/1423843003/diff/1/content/browser/compositor/... > > content/browser/compositor/buffer_queue.cc:263: return > > scoped_ptr<AllocatedSurface>( > > make_scoped_ptr(new All...)? > > > > https://codereview.chromium.org/1423843003/diff/1/content/browser/compositor/... > > File content/browser/compositor/buffer_queue.h (right): > > > > https://codereview.chromium.org/1423843003/diff/1/content/browser/compositor/... > > content/browser/compositor/buffer_queue.h:104: > > cc::ScopedPtrVector<AllocatedSurface> available_surfaces_; > > could we use base::ScopedVector instead? > > Oh -- what are your thoughts on having me shelve this until we have C++14 support for std::vector<scoped_ptr<T>> and deque? (as-is, this violates deps). Can't we just use base::ScopedVector for both containers in the meantime? I think that would still be a good improvement to the existing code.
If you wait by the river long enough, the bodies of your linked_ptr<>s will come bobbing on by...
(updated)
https://codereview.chromium.org/1423843003/diff/20001/content/browser/composi... File content/browser/compositor/buffer_queue.cc (right): https://codereview.chromium.org/1423843003/diff/20001/content/browser/composi... content/browser/compositor/buffer_queue.cc:95: in_flight_surfaces_.push_back(current_surface_.Pass()); std::move() https://codereview.chromium.org/1423843003/diff/20001/content/browser/composi... content/browser/compositor/buffer_queue.cc:130: surface = RecreateBuffer(surface.Pass()); std::move() for all passing https://codereview.chromium.org/1423843003/diff/20001/content/browser/composi... content/browser/compositor/buffer_queue.cc:135: if (current_surface_ && current_surface_->texture) { you could just make RecreateBuffer return null if the texture would be 0? doesn't it already? https://codereview.chromium.org/1423843003/diff/20001/content/browser/composi... content/browser/compositor/buffer_queue.cc:147: return scoped_ptr<AllocatedSurface>(); return nullptr https://codereview.chromium.org/1423843003/diff/20001/content/browser/composi... content/browser/compositor/buffer_queue.cc:155: return new_surface.Pass(); don't std::move() on return unless you're upcasting. clang should yell at you for this if you use move() https://codereview.chromium.org/1423843003/diff/20001/content/browser/composi... content/browser/compositor/buffer_queue.cc:160: if (displayed_surface_ && displayed_surface_->texture) can you be non-null but 0 texture? https://codereview.chromium.org/1423843003/diff/20001/content/browser/composi... content/browser/compositor/buffer_queue.cc:196: return surface.Pass(); should be move() but on return shouldn't be anything https://codereview.chromium.org/1423843003/diff/20001/content/browser/composi... content/browser/compositor/buffer_queue.cc:203: return scoped_ptr<AllocatedSurface>(new AllocatedSurface); make_scoped_ptr(new All...) but why not return nullptr in these errors? or, why are you nullchecking in other places? IOW it'd be nice to have 1 way to error not two: either null or 0 texture, choose one? https://codereview.chromium.org/1423843003/diff/20001/content/browser/composi... content/browser/compositor/buffer_queue.cc:216: return scoped_ptr<AllocatedSurface>(new AllocatedSurface); make_scoped_ptr https://codereview.chromium.org/1423843003/diff/20001/content/browser/composi... content/browser/compositor/buffer_queue.cc:226: return scoped_ptr<AllocatedSurface>(new AllocatedSurface); make_scoped_ptr https://codereview.chromium.org/1423843003/diff/20001/content/browser/composi... content/browser/compositor/buffer_queue.cc:253: if (buffer_queue) it can be null?
Thanks, updated (bob bob bob) https://codereview.chromium.org/1423843003/diff/20001/content/browser/composi... File content/browser/compositor/buffer_queue.cc (right): https://codereview.chromium.org/1423843003/diff/20001/content/browser/composi... content/browser/compositor/buffer_queue.cc:95: in_flight_surfaces_.push_back(current_surface_.Pass()); On 2015/12/09 23:25:16, danakj (behind on reviews) wrote: > std::move() Done. https://codereview.chromium.org/1423843003/diff/20001/content/browser/composi... content/browser/compositor/buffer_queue.cc:130: surface = RecreateBuffer(surface.Pass()); On 2015/12/09 23:25:15, danakj (behind on reviews) wrote: > std::move() for all passing Done. https://codereview.chromium.org/1423843003/diff/20001/content/browser/composi... content/browser/compositor/buffer_queue.cc:135: if (current_surface_ && current_surface_->texture) { On 2015/12/09 23:25:15, danakj (behind on reviews) wrote: > you could just make RecreateBuffer return null if the texture would be 0? > doesn't it already? This is the "remnant" mentioned below. https://codereview.chromium.org/1423843003/diff/20001/content/browser/composi... content/browser/compositor/buffer_queue.cc:147: return scoped_ptr<AllocatedSurface>(); On 2015/12/09 23:25:15, danakj (behind on reviews) wrote: > return nullptr Done. https://codereview.chromium.org/1423843003/diff/20001/content/browser/composi... content/browser/compositor/buffer_queue.cc:155: return new_surface.Pass(); On 2015/12/09 23:25:15, danakj (behind on reviews) wrote: > don't std::move() on return unless you're upcasting. clang should yell at you > for this if you use move() It does :) https://codereview.chromium.org/1423843003/diff/20001/content/browser/composi... content/browser/compositor/buffer_queue.cc:160: if (displayed_surface_ && displayed_surface_->texture) On 2015/12/09 23:25:15, danakj (behind on reviews) wrote: > can you be non-null but 0 texture? Yes (... see discussion later). https://codereview.chromium.org/1423843003/diff/20001/content/browser/composi... content/browser/compositor/buffer_queue.cc:196: return surface.Pass(); On 2015/12/09 23:25:15, danakj (behind on reviews) wrote: > should be move() but on return shouldn't be anything Done. https://codereview.chromium.org/1423843003/diff/20001/content/browser/composi... content/browser/compositor/buffer_queue.cc:203: return scoped_ptr<AllocatedSurface>(new AllocatedSurface); On 2015/12/09 23:25:15, danakj (behind on reviews) wrote: > make_scoped_ptr(new All...) > > but why not return nullptr in these errors? or, why are you nullchecking in > other places? > > IOW it'd be nice to have 1 way to error not two: either null or 0 texture, > choose one? Myself as of a few hours agreed with you (actually, there are some remanants of that above). Myself as of sending this out figured that was too big a change to merge into this. But ... I gave that another try, and it's not too hard. The idea before was that nullptr is never returned on error. If a surface is requested, you always get a pointer, but it may wrap a zero-texture -- so nullptr represents something just not being there, not an error (the current surface is nullptr when it is not bound yet, the displayed surface is nullptr before it is swapped in). But, now there is just nullptr. https://codereview.chromium.org/1423843003/diff/20001/content/browser/composi... content/browser/compositor/buffer_queue.cc:216: return scoped_ptr<AllocatedSurface>(new AllocatedSurface); On 2015/12/09 23:25:15, danakj (behind on reviews) wrote: > make_scoped_ptr Done. https://codereview.chromium.org/1423843003/diff/20001/content/browser/composi... content/browser/compositor/buffer_queue.cc:226: return scoped_ptr<AllocatedSurface>(new AllocatedSurface); On 2015/12/09 23:25:15, danakj (behind on reviews) wrote: > make_scoped_ptr Done. https://codereview.chromium.org/1423843003/diff/20001/content/browser/composi... content/browser/compositor/buffer_queue.cc:253: if (buffer_queue) On 2015/12/09 23:25:15, danakj (behind on reviews) wrote: > it can be null? Yeah, if the default ctor was used.
https://codereview.chromium.org/1423843003/diff/40001/content/browser/composi... File content/browser/compositor/buffer_queue.cc (left): https://codereview.chromium.org/1423843003/diff/40001/content/browser/composi... content/browser/compositor/buffer_queue.cc:86: DCHECK(!in_flight_surfaces_.empty() || displayed_surface_.texture); no more dcheck? https://codereview.chromium.org/1423843003/diff/40001/content/browser/composi... File content/browser/compositor/buffer_queue.cc (right): https://codereview.chromium.org/1423843003/diff/40001/content/browser/composi... content/browser/compositor/buffer_queue.cc:58: current_surface_ ? current_surface_->texture : 0, 0); is there a point to doing FramebufferTexture2D(..., 0, 0) here if GetNextSurface() failed? We don't in RecreateBuffers, should we there? https://codereview.chromium.org/1423843003/diff/40001/content/browser/composi... content/browser/compositor/buffer_queue.cc:87: if (!in_flight_surfaces_.empty() && in_flight_surfaces_.back()) This logic is a bit different. Before if there was an inflight surface it would use that id, even 0. Now if it's 0 it'll try use displayed_surface_. Maybe it's missing a test for this code. https://codereview.chromium.org/1423843003/diff/40001/content/browser/composi... content/browser/compositor/buffer_queue.cc:91: if (texture_id) { nit: can you put a whitespace line above this to separate it from the if/elseif stuff above? https://codereview.chromium.org/1423843003/diff/40001/content/browser/composi... content/browser/compositor/buffer_queue.cc:253: if (buffer_queue) i don't see calls to default constructor anymore. never null? maybe remove default constructor? https://codereview.chromium.org/1423843003/diff/40001/content/browser/composi... File content/browser/compositor/buffer_queue.h (right): https://codereview.chromium.org/1423843003/diff/40001/content/browser/composi... content/browser/compositor/buffer_queue.h:53: unsigned int current_texture_id() const { return current_surface_->texture; } do callers need to worry about null here? can they avoid a null deref? or is it invalid to call this when current surface is null? can you document when this should/shouldn't be called then?
Patchset #4 (id:60001) has been deleted
Patchset #4 (id:80001) has been deleted
Updated, with moar tests! https://codereview.chromium.org/1423843003/diff/40001/content/browser/composi... File content/browser/compositor/buffer_queue.cc (left): https://codereview.chromium.org/1423843003/diff/40001/content/browser/composi... content/browser/compositor/buffer_queue.cc:86: DCHECK(!in_flight_surfaces_.empty() || displayed_surface_.texture); On 2015/12/10 01:36:02, danakj (behind on reviews) wrote: > no more dcheck? That was wrong-by-inspection. It was entirely possible for |displayed_surface_.texture| to be zero if we fail to allocate the buffer. https://codereview.chromium.org/1423843003/diff/40001/content/browser/composi... File content/browser/compositor/buffer_queue.cc (right): https://codereview.chromium.org/1423843003/diff/40001/content/browser/composi... content/browser/compositor/buffer_queue.cc:58: current_surface_ ? current_surface_->texture : 0, 0); On 2015/12/10 01:36:02, danakj (behind on reviews) wrote: > is there a point to doing FramebufferTexture2D(..., 0, 0) here if > GetNextSurface() failed? We don't in RecreateBuffers, should we there? The spec says that the texture is automatically un-bound, so, no point. Removed. https://codereview.chromium.org/1423843003/diff/40001/content/browser/composi... content/browser/compositor/buffer_queue.cc:87: if (!in_flight_surfaces_.empty() && in_flight_surfaces_.back()) On 2015/12/10 01:36:02, danakj (behind on reviews) wrote: > This logic is a bit different. > > Before if there was an inflight surface it would use that id, even 0. Now if > it's 0 it'll try use displayed_surface_. Yeah. Actually, to be more thorough, we should do the in-flight surfaces back-to-front, then hit up the displayed surface. > Maybe it's missing a test for this code. Moar tests added!! https://codereview.chromium.org/1423843003/diff/40001/content/browser/composi... content/browser/compositor/buffer_queue.cc:91: if (texture_id) { On 2015/12/10 01:36:02, danakj (behind on reviews) wrote: > nit: can you put a whitespace line above this to separate it from the if/elseif > stuff above? Done. https://codereview.chromium.org/1423843003/diff/40001/content/browser/composi... content/browser/compositor/buffer_queue.cc:253: if (buffer_queue) On 2015/12/10 01:36:02, danakj (behind on reviews) wrote: > i don't see calls to default constructor anymore. never null? maybe remove > default constructor? Good call -- I removed the default ctor, so this can be killed now. https://codereview.chromium.org/1423843003/diff/40001/content/browser/composi... File content/browser/compositor/buffer_queue.h (right): https://codereview.chromium.org/1423843003/diff/40001/content/browser/composi... content/browser/compositor/buffer_queue.h:53: unsigned int current_texture_id() const { return current_surface_->texture; } On 2015/12/10 01:36:02, danakj (behind on reviews) wrote: > do callers need to worry about null here? can they avoid a null deref? or is it > invalid to call this when current surface is null? can you document when this > should/shouldn't be called then? Callers should worry. I'll preserve the old behavior by returning 0.
Thanks for more tests, few more comments https://codereview.chromium.org/1423843003/diff/110001/content/browser/compos... File content/browser/compositor/buffer_queue.cc (right): https://codereview.chromium.org/1423843003/diff/110001/content/browser/compos... content/browser/compositor/buffer_queue.cc:91: for (auto it = in_flight_surfaces_.rbegin(); you could use for (auto& surface : base::Reversed(in_flight_surfaces)) {} https://codereview.chromium.org/1423843003/diff/110001/content/browser/compos... File content/browser/compositor/buffer_queue_unittest.cc (right): https://codereview.chromium.org/1423843003/diff/110001/content/browser/compos... content/browser/compositor/buffer_queue_unittest.cc:53: bool allocate_succeeds_; nit: make private and add a set_allocate_succeeds() https://codereview.chromium.org/1423843003/diff/110001/content/browser/compos... content/browser/compositor/buffer_queue_unittest.cc:515: auto current = current_frame(); should these all be auto*? https://codereview.chromium.org/1423843003/diff/110001/content/browser/compos... content/browser/compositor/buffer_queue_unittest.cc:593: // Shoot the just-created buffer, and try another swap. The copy should s/Shoot/Destroy/
Updated! https://codereview.chromium.org/1423843003/diff/110001/content/browser/compos... File content/browser/compositor/buffer_queue.cc (right): https://codereview.chromium.org/1423843003/diff/110001/content/browser/compos... content/browser/compositor/buffer_queue.cc:91: for (auto it = in_flight_surfaces_.rbegin(); On 2015/12/10 at 19:46:42, danakj (behind on reviews) wrote: > you could use > > for (auto& surface : base::Reversed(in_flight_surfaces)) {} done https://codereview.chromium.org/1423843003/diff/110001/content/browser/compos... File content/browser/compositor/buffer_queue_unittest.cc (right): https://codereview.chromium.org/1423843003/diff/110001/content/browser/compos... content/browser/compositor/buffer_queue_unittest.cc:53: bool allocate_succeeds_; On 2015/12/10 at 19:46:42, danakj (behind on reviews) wrote: > nit: make private and add a set_allocate_succeeds() Done. https://codereview.chromium.org/1423843003/diff/110001/content/browser/compos... content/browser/compositor/buffer_queue_unittest.cc:515: auto current = current_frame(); On 2015/12/10 at 19:46:42, danakj (behind on reviews) wrote: > should these all be auto*? That works! Done. https://codereview.chromium.org/1423843003/diff/110001/content/browser/compos... content/browser/compositor/buffer_queue_unittest.cc:593: // Shoot the just-created buffer, and try another swap. The copy should On 2015/12/10 at 19:46:42, danakj (behind on reviews) wrote: > s/Shoot/Destroy/ Done.
LGTM
The CQ bit was checked by danakj@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1423843003/130001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1423843003/130001
Message was sent while issue was closed.
Committed patchset #6 (id:130001)
Message was sent while issue was closed.
Description was changed from ========== Make content::BufferQueue use scoped ptrs for AllocatedSurfaces BUG=546795 ========== to ========== Make content::BufferQueue use scoped ptrs for AllocatedSurfaces BUG=546795 Committed: https://crrev.com/3a36c0065435bf22a9fda905d6e7a47924a336d8 Cr-Commit-Position: refs/heads/master@{#364833} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/3a36c0065435bf22a9fda905d6e7a47924a336d8 Cr-Commit-Position: refs/heads/master@{#364833} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
