Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2607203002:
[Mojo Video Capture] Retire buffers when Android Chromium goes to the background (Closed)
|
Created:
3 years, 11 months ago by chfremer Modified:
3 years, 10 months ago CC:
chromium-reviews, qsr+mojo_chromium.org, Aaron Boodman, posciak+watch_chromium.org, viettrungluu+watch_chromium.org, jam, yzshen+watch_chromium.org, abarth-chromium, feature-media-reviews_chromium.org, darin-cc_chromium.org, mcasas+watch+vc_chromium.org, xjz+watch_chromium.org, darin (slow to review), miu+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Mojo Video Capture] Retire buffers when Android Chromium goes to the background
Functional change:
Before this CL, when Android Chromium went to the background while video was
capturing, it would release both the VCDevice and VCDeviceClient while keeping
the VCBufferPool and the buffers already shared with Renderers alive. After this
CL, when going to the background, we also release the VCBufferPool and the
buffers.
Motivation:
For the Mojofication, we want to move the VCDevice, VCDeviceClient, and
VCBufferPool behind an abstraction (let's refer to it as AbstractDevice) and run
them in a separate process. With that, shutting down VCDevice, VCDeviceClient
while keeping VCBufferPool and shared buffers alive would require exposing this
as some new form of "suspended" mode for AbstractDevice (which is different from
the existing suspend/resume mechanism).
Instead of this, we opt for the less complex alternative solution, which is to
allow a complete shutdown of AbstractDevice, including the buffer pool, while
the app is in the background.
Execution:
In interface media::VideoFrameReceiver, replace OnBufferDestroyed(), which could
only be called while a buffer was not in use by the VideoFrameReceiver with
OnBufferRetired(), which may now be called while a buffer is still in use.
When AbstractDevice shuts down, it tells the VideoFrameReceiver that it is
"retiring" the buffers. The VideoFrameReceiver may keep consuming them as long
as it needs, since it holds shared ownership while it is consuming.
This CL is part of the Mojo Video Capture work. For the bigger picture,
see [1] CL11pre.
BUG=584797
TEST=
content_unittests --gtest_filter="*Video*",
Apprtc loopback on Debug,
Desktop Capture Example extension on Release
On Android content_shell, run video capture sample from [2], put to
background, then restore from background.
[1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxsxWslJ_X-zpOhijvI8/edit?usp=sharing
[2] https://webrtc.github.io/samples/src/content/getusermedia/gum/
Review-Url: https://codereview.chromium.org/2607203002
Cr-Commit-Position: refs/heads/master@{#448376}
Committed: https://chromium.googlesource.com/chromium/src/+/10e14c24b244e7f58074ae74b0181cb3cb60551e
Patch Set 1 #
Total comments: 21
Patch Set 2 : Added tests and fixed a bug uncovered by the tests. #
Total comments: 4
Patch Set 3 : change std::set to std::vector #Patch Set 4 : rebase #Patch Set 5 : Support reused buffer ids in Controller #
Total comments: 16
Patch Set 6 : mcasas comments #Messages
Total messages: 42 (28 generated)
Description was changed from
==========
Apply patch to 11pre
BUG=
==========
to
==========
[Mojo Video Capture] Retire buffers when Android Chromium goes to the background
Functional change:
Before this CL, when Android Chromium went to the background while video was
capturing, it would release both the VCDevice and VCDeviceClient while keeping
the VCBufferPool and the buffers already shared with Renderers alive. After this
CL, when going to the background, we also release the VCBufferPool and the
buffers.
Motivation:
For the Mojofication, we want to move the VCDevice, VCDeviceClient, and
VCBufferPool behind an abstraction (let's refer to it as AbstractDevice) and run
them in a separate process. With that, shutting down VCDevice, VCDeviceClient
while keeping VCBufferPool and shared buffers alive would require exposing this
as some new form of "suspended" mode for AbstractDevice (which is different from
the existing suspend/resume mechanism).
Instead of this, we opt for the less complex alternative solution, which is to
allow a complete shutdown of AbstractDevice, including the buffer pool, while
the app is in the background.
Execution:
In interface media::VideoFrameReceiver, replace OnBufferDestroyed(), which could
only be called while a buffer was not in use by the VideoFrameReceiver with
OnBufferRetired(), which may now be called while a buffer is still in use.
When AbstractDevice shuts down, it tells the VideoFrameReceiver that it is
"retiring" the buffers. The VideoFrameReceiver may keep consuming them as long
as it needs, since it holds shared ownership while it is consuming.
This CL is part of the Mojo Video Capture work. For the bigger picture,
see [1] CL11pre.
BUG=584797
TEST=
content_unittests --gtest_filter="*Video*",
Apprtc loopback on Debug,
Desktop Capture Example extension on Release
On Android content_shell, run video capture sample from [2], put to
background, then restore from background.
[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
[2] https://webrtc.github.io/samples/src/content/getusermedia/gum/
==========
The CQ bit was checked by chfremer@chromium.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.chromium.or...
Patchset #1 (id:1) has been deleted
chfremer@chromium.org changed reviewers: + braveyao@chromium.org, mcasas@chromium.org, miu@chromium.org
mcasas@: PTAL miu@: FYI braveyao@: Are there any concerns from your side regarding the functional change of releasing buffers while in background? Or is this actually a welcome change, considering that it occupies less memory while in the background?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
In the spirit of fighting codebase complexity, can you add more unittests for these particular behaviour? https://codereview.chromium.org/2607203002/diff/20001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_controller.cc (right): https://codereview.chromium.org/2607203002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller.cc:503: } nit: no {} for one-line bodies, even for two one-line bodies. https://codereview.chromium.org/2607203002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller.cc:536: DCHECK(buffer_state_iter != buffer_states_.end()); These lines are == to l.489-494 (and also a bit like l.382-286) plz factor them out somehow. https://codereview.chromium.org/2607203002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller.cc:552: auto known_buffers_entry_iter = std::find(std::begin(client->known_buffers), I wouldn't use the suffix _iter because it looks like encoding the variable type in the name, which we don't like in Chromium; instead, just call it by what it is, e.g. s/known_buffers_entry_iter/known_buffer/ Here and elsewhere in this file. https://codereview.chromium.org/2607203002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller.cc:562: } More testing of all this dancing of buffers...? https://codereview.chromium.org/2607203002/diff/20001/media/capture/video/vid... File media/capture/video/video_capture_device_client.cc (right): https://codereview.chromium.org/2607203002/diff/20001/media/capture/video/vid... media/capture/video/video_capture_device_client.cc:101: } No {} for one-line bodies. https://codereview.chromium.org/2607203002/diff/20001/media/capture/video/vid... media/capture/video/video_capture_device_client.cc:310: // Buffer pool has decided to releasea buffer. Notify receiver in case s/releasea/release a/ s/Buffer pool/|buffer_pool_|/ ? https://codereview.chromium.org/2607203002/diff/20001/media/capture/video/vid... media/capture/video/video_capture_device_client.cc:315: receiver_->OnBufferRetired(buffer_id_to_drop); if (base::ContainsValue(buffer_ids_known_by_receiver, buffer_id_to_drop)) { buffer_ids_known_by_receiver.erase(buffer_id_to_drop); receiver_->OnBufferRetired(buffer_id_to_drop); } https://cs.chromium.org/chromium/src/base/stl_util.h?dr=CSs&q=containskey&sq=... https://codereview.chromium.org/2607203002/diff/20001/media/capture/video/vid... media/capture/video/video_capture_device_client.cc:346: } if (base::ContainsValue(buffer_ids_known_by_receiver, buffer.id())) buffer_ids_known_by_receiver.insert(buffer.id()); https://cs.chromium.org/chromium/src/base/stl_util.h?dr=CSs&q=containskey&sq=... https://codereview.chromium.org/2607203002/diff/20001/media/capture/video/vid... File media/capture/video/video_capture_device_client.h (right): https://codereview.chromium.org/2607203002/diff/20001/media/capture/video/vid... media/capture/video/video_capture_device_client.h:132: DFAKE_MUTEX(call_from_producer_); Ooooh this is brilliant! https://codereview.chromium.org/2607203002/diff/20001/media/capture/video/vid... File media/capture/video/video_frame_receiver.h (right): https://codereview.chromium.org/2607203002/diff/20001/media/capture/video/vid... media/capture/video/video_frame_receiver.h:31: // The producer guarantees that it does not reuse a retired buffer_id before s/buffer_id/|buffer_id|/
PTAL Thanks for the suggestion to add tests. This actually uncovered a bug that I would otherwise have introduced here (new instances of VideoCaptureBufferPoolImpl would start handing out buffers with |buffer_id| = 0, violating the new contract for VideoFrameReceiver that ids of retired buffers are not reused). I added the new test cases to video_capture_controller_unittest.cc, which tests an already quite complex unit involving several classes. With some of these classes now being disentangled, we could in theory start adding tests on smaller units as well, for example tests exercising VideoCaptureController or VideoCaptureDeviceClient in isolation. However, I would not be sure how to name them, since video_capture_controller_unittest.cc and video_capture_device_client_unittest.cc are already taken by other tests. https://codereview.chromium.org/2607203002/diff/20001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_controller.cc (right): https://codereview.chromium.org/2607203002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller.cc:503: } On 2017/01/20 22:41:32, mcasas wrote: > nit: no {} for one-line bodies, even for two one-line bodies. Done. https://codereview.chromium.org/2607203002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller.cc:536: DCHECK(buffer_state_iter != buffer_states_.end()); On 2017/01/20 22:41:32, mcasas wrote: > These lines are == to l.489-494 (and also a bit like l.382-286) > plz factor them out somehow. Done. https://codereview.chromium.org/2607203002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller.cc:552: auto known_buffers_entry_iter = std::find(std::begin(client->known_buffers), On 2017/01/20 22:41:32, mcasas wrote: > I wouldn't use the suffix _iter because it looks like > encoding the variable type in the name, which we > don't like in Chromium; instead, just call it by what > it is, e.g. > s/known_buffers_entry_iter/known_buffer/ > > Here and elsewhere in this file. Hmm. I specifically chose to add the "_iter" suffix in order to call it by what it is (or more precisely, what API a reader should expect it to expose). The suffix in the name is needed to avoid confusing it with a reference (or copy) of an actual entry of the container. Without the suffix, readers have to navigate to the declaration in order to find out what it is, which is an unnecessary burden on the reader. https://codereview.chromium.org/2607203002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller.cc:562: } On 2017/01/20 22:41:32, mcasas wrote: > More testing of all this dancing of buffers...? Yes. Will do. https://codereview.chromium.org/2607203002/diff/20001/media/capture/video/vid... File media/capture/video/video_capture_device_client.cc (right): https://codereview.chromium.org/2607203002/diff/20001/media/capture/video/vid... media/capture/video/video_capture_device_client.cc:101: } On 2017/01/20 22:41:32, mcasas wrote: > No {} for one-line bodies. Done. https://codereview.chromium.org/2607203002/diff/20001/media/capture/video/vid... media/capture/video/video_capture_device_client.cc:310: // Buffer pool has decided to releasea buffer. Notify receiver in case On 2017/01/20 22:41:32, mcasas wrote: > s/releasea/release a/ > > s/Buffer pool/|buffer_pool_|/ ? Done. https://codereview.chromium.org/2607203002/diff/20001/media/capture/video/vid... media/capture/video/video_capture_device_client.cc:315: receiver_->OnBufferRetired(buffer_id_to_drop); On 2017/01/20 22:41:32, mcasas wrote: > if (base::ContainsValue(buffer_ids_known_by_receiver, buffer_id_to_drop)) { > buffer_ids_known_by_receiver.erase(buffer_id_to_drop); > receiver_->OnBufferRetired(buffer_id_to_drop); > } > > https://cs.chromium.org/chromium/src/base/stl_util.h?dr=CSs&q=containskey&sq=... Done. Thanks. That is much nicer indeed. https://codereview.chromium.org/2607203002/diff/20001/media/capture/video/vid... media/capture/video/video_capture_device_client.cc:346: } On 2017/01/20 22:41:32, mcasas wrote: > if (base::ContainsValue(buffer_ids_known_by_receiver, buffer.id())) > buffer_ids_known_by_receiver.insert(buffer.id()); > > https://cs.chromium.org/chromium/src/base/stl_util.h?dr=CSs&q=containskey&sq=... Done. https://codereview.chromium.org/2607203002/diff/20001/media/capture/video/vid... File media/capture/video/video_capture_device_client.h (right): https://codereview.chromium.org/2607203002/diff/20001/media/capture/video/vid... media/capture/video/video_capture_device_client.h:132: DFAKE_MUTEX(call_from_producer_); On 2017/01/20 22:41:32, mcasas wrote: > Ooooh this is brilliant! Acknowledged. https://codereview.chromium.org/2607203002/diff/20001/media/capture/video/vid... File media/capture/video/video_frame_receiver.h (right): https://codereview.chromium.org/2607203002/diff/20001/media/capture/video/vid... media/capture/video/video_frame_receiver.h:31: // The producer guarantees that it does not reuse a retired buffer_id before On 2017/01/20 22:41:32, mcasas wrote: > s/buffer_id/|buffer_id|/ Done.
The CQ bit was checked by chfremer@chromium.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.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/01/25 23:45:24, chfremer wrote: > PTAL > > Thanks for the suggestion to add tests. This actually uncovered a bug that I > would otherwise have introduced here (new instances of > VideoCaptureBufferPoolImpl would start handing out buffers with |buffer_id| = 0, > violating the new contract for VideoFrameReceiver that ids of retired buffers > are not reused). > > I added the new test cases to video_capture_controller_unittest.cc, which tests > an already quite complex unit involving several classes. With some of these > classes now being disentangled, we could in theory start adding tests on smaller > units as well, for example tests exercising VideoCaptureController or > VideoCaptureDeviceClient in isolation. However, I would not be sure how to name > them, since video_capture_controller_unittest.cc and > video_capture_device_client_unittest.cc are already taken by other tests. > > https://codereview.chromium.org/2607203002/diff/20001/content/browser/rendere... > File content/browser/renderer_host/media/video_capture_controller.cc (right): > > https://codereview.chromium.org/2607203002/diff/20001/content/browser/rendere... > content/browser/renderer_host/media/video_capture_controller.cc:503: } > On 2017/01/20 22:41:32, mcasas wrote: > > nit: no {} for one-line bodies, even for two one-line bodies. > > Done. > > https://codereview.chromium.org/2607203002/diff/20001/content/browser/rendere... > content/browser/renderer_host/media/video_capture_controller.cc:536: > DCHECK(buffer_state_iter != buffer_states_.end()); > On 2017/01/20 22:41:32, mcasas wrote: > > These lines are == to l.489-494 (and also a bit like l.382-286) > > plz factor them out somehow. > > Done. > > https://codereview.chromium.org/2607203002/diff/20001/content/browser/rendere... > content/browser/renderer_host/media/video_capture_controller.cc:552: auto > known_buffers_entry_iter = std::find(std::begin(client->known_buffers), > On 2017/01/20 22:41:32, mcasas wrote: > > I wouldn't use the suffix _iter because it looks like > > encoding the variable type in the name, which we > > don't like in Chromium; instead, just call it by what > > it is, e.g. > > s/known_buffers_entry_iter/known_buffer/ > > > > Here and elsewhere in this file. > > Hmm. I specifically chose to add the "_iter" suffix in order to call it by what > it is (or more precisely, what API a reader should expect it to expose). The > suffix in the name is needed to avoid confusing it with a reference (or copy) of > an actual entry of the container. Without the suffix, readers have to navigate > to the declaration in order to find out what it is, which is an unnecessary > burden on the reader. > > https://codereview.chromium.org/2607203002/diff/20001/content/browser/rendere... > content/browser/renderer_host/media/video_capture_controller.cc:562: } > On 2017/01/20 22:41:32, mcasas wrote: > > More testing of all this dancing of buffers...? > > Yes. Will do. > > https://codereview.chromium.org/2607203002/diff/20001/media/capture/video/vid... > File media/capture/video/video_capture_device_client.cc (right): > > https://codereview.chromium.org/2607203002/diff/20001/media/capture/video/vid... > media/capture/video/video_capture_device_client.cc:101: } > On 2017/01/20 22:41:32, mcasas wrote: > > No {} for one-line bodies. > > Done. > > https://codereview.chromium.org/2607203002/diff/20001/media/capture/video/vid... > media/capture/video/video_capture_device_client.cc:310: // Buffer pool has > decided to releasea buffer. Notify receiver in case > On 2017/01/20 22:41:32, mcasas wrote: > > s/releasea/release a/ > > > > s/Buffer pool/|buffer_pool_|/ ? > > Done. > > https://codereview.chromium.org/2607203002/diff/20001/media/capture/video/vid... > media/capture/video/video_capture_device_client.cc:315: > receiver_->OnBufferRetired(buffer_id_to_drop); > On 2017/01/20 22:41:32, mcasas wrote: > > if (base::ContainsValue(buffer_ids_known_by_receiver, buffer_id_to_drop)) > { > > buffer_ids_known_by_receiver.erase(buffer_id_to_drop); > > receiver_->OnBufferRetired(buffer_id_to_drop); > > } > > > > > https://cs.chromium.org/chromium/src/base/stl_util.h?dr=CSs&q=containskey&sq=... > > Done. > Thanks. That is much nicer indeed. > > https://codereview.chromium.org/2607203002/diff/20001/media/capture/video/vid... > media/capture/video/video_capture_device_client.cc:346: } > On 2017/01/20 22:41:32, mcasas wrote: > > if (base::ContainsValue(buffer_ids_known_by_receiver, buffer.id())) > > buffer_ids_known_by_receiver.insert(buffer.id()); > > > > > https://cs.chromium.org/chromium/src/base/stl_util.h?dr=CSs&q=containskey&sq=... > > Done. > > https://codereview.chromium.org/2607203002/diff/20001/media/capture/video/vid... > File media/capture/video/video_capture_device_client.h (right): > > https://codereview.chromium.org/2607203002/diff/20001/media/capture/video/vid... > media/capture/video/video_capture_device_client.h:132: > DFAKE_MUTEX(call_from_producer_); > On 2017/01/20 22:41:32, mcasas wrote: > > Ooooh this is brilliant! > > Acknowledged. > > https://codereview.chromium.org/2607203002/diff/20001/media/capture/video/vid... > File media/capture/video/video_frame_receiver.h (right): > > https://codereview.chromium.org/2607203002/diff/20001/media/capture/video/vid... > media/capture/video/video_frame_receiver.h:31: // The producer guarantees that > it does not reuse a retired buffer_id before > On 2017/01/20 22:41:32, mcasas wrote: > > s/buffer_id/|buffer_id|/ > > Done. ping
lgtm % nits: https://codereview.chromium.org/2607203002/diff/40001/media/capture/video/vid... File media/capture/video/video_capture_device_client.h (right): https://codereview.chromium.org/2607203002/diff/40001/media/capture/video/vid... media/capture/video/video_capture_device_client.h:103: std::set<int> buffer_ids_known_by_receiver_; nit: std::vector<int> instead, for better CPU/memory performance? (Assuming this will always contain less than 100-ish elements.) https://codereview.chromium.org/2607203002/diff/40001/media/capture/video/vid... File media/capture/video/video_frame_receiver.h (right): https://codereview.chromium.org/2607203002/diff/40001/media/capture/video/vid... media/capture/video/video_frame_receiver.h:30: // at its earliest convenience. nit: Does "at its earliest convenience" really mean "once there are no consumers referencing the frame?"
lgtm On the naming, I feel mostly like this answer: http://stackoverflow.com/questions/3592378/is-naming-variables-after-their-ty... https://codereview.chromium.org/2607203002/diff/20001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_controller.cc (right): https://codereview.chromium.org/2607203002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller.cc:552: auto known_buffers_entry_iter = std::find(std::begin(client->known_buffers), On 2017/01/25 23:45:20, chfremer wrote: > On 2017/01/20 22:41:32, mcasas wrote: > > I wouldn't use the suffix _iter because it looks like > > encoding the variable type in the name, which we > > don't like in Chromium; instead, just call it by what > > it is, e.g. > > s/known_buffers_entry_iter/known_buffer/ > > > > Here and elsewhere in this file. > > Hmm. I specifically chose to add the "_iter" suffix in order to call it by what > it is (or more precisely, what API a reader should expect it to expose). The > suffix in the name is needed to avoid confusing it with a reference (or copy) of > an actual entry of the container. Without the suffix, readers have to navigate > to the declaration in order to find out what it is, which is an unnecessary > burden on the reader. I disagree, variable names should have a semantical meaning, not a syntactical, and the fact that a variable is an iterator or a class or a reference should be taken care by the definition of the operators etc. That said, it seems like _iter in variables is somehow popular in media/ and content/ (grep -rn "_iter =" media content | wc -l ==> 125) so I guess it's not too bad for temporary variables (but not for constness etc).
On 2017/01/31 01:39:39, mcasas wrote: > lgtm > > > On the naming, I feel mostly like this answer: > http://stackoverflow.com/questions/3592378/is-naming-variables-after-their-ty... > > https://codereview.chromium.org/2607203002/diff/20001/content/browser/rendere... > File content/browser/renderer_host/media/video_capture_controller.cc (right): > > https://codereview.chromium.org/2607203002/diff/20001/content/browser/rendere... > content/browser/renderer_host/media/video_capture_controller.cc:552: auto > known_buffers_entry_iter = std::find(std::begin(client->known_buffers), > On 2017/01/25 23:45:20, chfremer wrote: > > On 2017/01/20 22:41:32, mcasas wrote: > > > I wouldn't use the suffix _iter because it looks like > > > encoding the variable type in the name, which we > > > don't like in Chromium; instead, just call it by what > > > it is, e.g. > > > s/known_buffers_entry_iter/known_buffer/ > > > > > > Here and elsewhere in this file. > > > > Hmm. I specifically chose to add the "_iter" suffix in order to call it by > what > > it is (or more precisely, what API a reader should expect it to expose). The > > suffix in the name is needed to avoid confusing it with a reference (or copy) > of > > an actual entry of the container. Without the suffix, readers have to navigate > > to the declaration in order to find out what it is, which is an unnecessary > > burden on the reader. > > I disagree, variable names should have a semantical > meaning, not a syntactical, and the fact that a variable > is an iterator or a class or a reference should be taken > care by the definition of the operators etc. That said, > it seems like _iter in variables is somehow popular in > media/ and content/ > (grep -rn "_iter =" media content | wc -l ==> 125) > so I guess it's not too bad for temporary variables > (but not for constness etc). I 100% agree with what you are saying about naming variables. I guess the interesting question is whether or not the difference between an element inside a collection and an iterator pointing to the same element is significant enough to be deemed "semantic". I see how opinions on that may diverge. Personally, I would answer with a clear yes, which is why I prefer to use the _iter suffix. What does it for me is that an iterator can be used in std::vector::erase(iter), while a regular element (or pointer or reference) cannot.
Thanks for the review! Added a patch set to change the std::set to a std::vector. https://codereview.chromium.org/2607203002/diff/40001/media/capture/video/vid... File media/capture/video/video_capture_device_client.h (right): https://codereview.chromium.org/2607203002/diff/40001/media/capture/video/vid... media/capture/video/video_capture_device_client.h:103: std::set<int> buffer_ids_known_by_receiver_; On 2017/01/31 01:18:51, miu wrote: > nit: std::vector<int> instead, for better CPU/memory performance? (Assuming this > will always contain less than 100-ish elements.) Done. https://codereview.chromium.org/2607203002/diff/40001/media/capture/video/vid... File media/capture/video/video_frame_receiver.h (right): https://codereview.chromium.org/2607203002/diff/40001/media/capture/video/vid... media/capture/video/video_frame_receiver.h:30: // at its earliest convenience. On 2017/01/31 01:18:51, miu wrote: > nit: Does "at its earliest convenience" really mean "once there are no consumers > referencing the frame?" In case of the concrete implementation VideoCaptureController this is indeed how we do it. However, this information belongs with the concrete class VideoCaptureController and not with this interface VideoFrameReceiver. That is the reason why I formulated it in such loose terms. The code operating the VideoFrameReceiver should not need to know what the implementation is, and it should not care how long that implementation holds on to retired buffers.
The CQ bit was checked by chfremer@chromium.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.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by chfremer@chromium.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.chromium.or...
PTAL I realized that introducing a static counter for generating buffer_ids in Patch Set 2 in VideoCaptureBufferPoolImpl was a bad idea. There are several problems with it. 1. Running content_unittests --gtest_filter="*Video*" locally produces test failures, because the counter is not reset between test runs. 2. Eventually, the buffer pool will live inside a separate service process. This means that when temporarily tearing down the VCDevice + VCDeviceClient + VCBufferPoolImpl instances (for Android background), this will reset the static counter as well, which makes it ineffective in guaranteeing that buffer ids are not reused. A potential solution would have been to have a "BufferIdGenerator" object live in VideoCaptureManager and pass it to newly created instances of VideoCaptureBufferPoolImpl. But this would get quite ugly, requiring communication between the VideoCaptureBufferPoolImpl and the VideoCaptureManager each time a new id is generated. I found that the cleanst solution seems to be to change the contract to allow buffer_ids being reused immediately after they have been retired (and while they may still be in use by clients of VideoCaptureController). To realize this, I made the following changes: 1. Rename BufferState to BufferContext (for improved clarity). As before, a BufferContext is created whenever a producer shares a new buffer with VideoCaptureController. Different from before, a new buffer can now reuse the same buffer_id as a previously retired buffer (that may still be in use by clients). 2. For communication between VideoCaptureController and its clients, do not use buffer_ids directly. Instead, use buffer_context_ids, which are unique even when the same buffer_id is in use more than once.
PS5 lgtm too.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Couple of minor comments otherwise still lgtm https://codereview.chromium.org/2607203002/diff/100001/content/browser/render... File content/browser/renderer_host/media/video_capture_controller.cc (right): https://codereview.chromium.org/2607203002/diff/100001/content/browser/render... content/browser/renderer_host/media/video_capture_controller.cc:377: const int buffer_context_id = buffer_context_iter->buffer_context_id(); Move to l. 401? Only needed inside the for loop l.402 https://codereview.chromium.org/2607203002/diff/100001/content/browser/render... content/browser/renderer_host/media/video_capture_controller.cc:516: return buffer_context_iter; return directly the std::find() statement? Also in l.527 https://codereview.chromium.org/2607203002/diff/100001/content/browser/render... File content/browser/renderer_host/media/video_capture_controller.h (right): https://codereview.chromium.org/2607203002/diff/100001/content/browser/render... content/browser/renderer_host/media/video_capture_controller.h:145: explicit BufferContext( nit: Doesn't need to be explicit since it has > 1 argument. https://codereview.chromium.org/2607203002/diff/100001/content/browser/render... content/browser/renderer_host/media/video_capture_controller.h:171: int buffer_context_id_; Can be const https://codereview.chromium.org/2607203002/diff/100001/content/browser/render... content/browser/renderer_host/media/video_capture_controller.h:178: int consumer_hold_count_ = 0; In general try to initialize only once, and by preference in the constructor, so all the default values are together. https://codereview.chromium.org/2607203002/diff/100001/content/browser/render... content/browser/renderer_host/media/video_capture_controller.h:199: const std::vector<BufferContext>::iterator& buffer_state_iter); Perhaps we could make a public using BuferContexts = std::vector<BufferContext>; https://codereview.chromium.org/2607203002/diff/100001/content/browser/render... File content/browser/renderer_host/media/video_capture_controller_unittest.cc (right): https://codereview.chromium.org/2607203002/diff/100001/content/browser/render... content/browser/renderer_host/media/video_capture_controller_unittest.cc:730: .Times(1); .Times(1) is superfluous since it's the default value. https://codereview.chromium.org/2607203002/diff/100001/content/browser/render... content/browser/renderer_host/media/video_capture_controller_unittest.cc:747: int buffer_id_reported_to_client = -1; Should we s/-1/media::VideoCaptureBufferPool::kInvalidId/ ?
Patchset #6 (id:120001) has been deleted
The CQ bit was checked by chfremer@chromium.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.chromium.or...
Thanks for the review! Addressed mcasas@ comments. https://codereview.chromium.org/2607203002/diff/100001/content/browser/render... File content/browser/renderer_host/media/video_capture_controller.cc (right): https://codereview.chromium.org/2607203002/diff/100001/content/browser/render... content/browser/renderer_host/media/video_capture_controller.cc:377: const int buffer_context_id = buffer_context_iter->buffer_context_id(); On 2017/02/03 23:24:21, mcasas wrote: > Move to l. 401? Only needed inside the for loop l.402 Done. https://codereview.chromium.org/2607203002/diff/100001/content/browser/render... content/browser/renderer_host/media/video_capture_controller.cc:516: return buffer_context_iter; On 2017/02/03 23:24:21, mcasas wrote: > return directly the std::find() statement? > > Also in l.527 Done. https://codereview.chromium.org/2607203002/diff/100001/content/browser/render... File content/browser/renderer_host/media/video_capture_controller.h (right): https://codereview.chromium.org/2607203002/diff/100001/content/browser/render... content/browser/renderer_host/media/video_capture_controller.h:145: explicit BufferContext( On 2017/02/03 23:24:22, mcasas wrote: > nit: Doesn't need to be explicit since it has > 1 argument. Done. https://codereview.chromium.org/2607203002/diff/100001/content/browser/render... content/browser/renderer_host/media/video_capture_controller.h:171: int buffer_context_id_; On 2017/02/03 23:24:22, mcasas wrote: > Can be const Assignment operator is needed for usage in std::vector<> here, which prevents the use of const. https://codereview.chromium.org/2607203002/diff/100001/content/browser/render... content/browser/renderer_host/media/video_capture_controller.h:178: int consumer_hold_count_ = 0; On 2017/02/03 23:24:22, mcasas wrote: > In general try to initialize only once, and by > preference in the constructor, so all the default > values are together. Done. https://codereview.chromium.org/2607203002/diff/100001/content/browser/render... content/browser/renderer_host/media/video_capture_controller.h:199: const std::vector<BufferContext>::iterator& buffer_state_iter); On 2017/02/03 23:24:22, mcasas wrote: > Perhaps we could make a public > > using BuferContexts = std::vector<BufferContext>; To be honest, I am not currently a fan of the "usings" aliases for collection types. The reason is that I always need to know the type (interface) of the collection being used in order to work with it. A type alias whose name is just the plural form of the element type does not contain this information, so I have to go hunt down the alias declaration every time. https://codereview.chromium.org/2607203002/diff/100001/content/browser/render... File content/browser/renderer_host/media/video_capture_controller_unittest.cc (right): https://codereview.chromium.org/2607203002/diff/100001/content/browser/render... content/browser/renderer_host/media/video_capture_controller_unittest.cc:730: .Times(1); On 2017/02/03 23:24:22, mcasas wrote: > .Times(1) is superfluous since it's the default value. Done. https://codereview.chromium.org/2607203002/diff/100001/content/browser/render... content/browser/renderer_host/media/video_capture_controller_unittest.cc:747: int buffer_id_reported_to_client = -1; On 2017/02/03 23:24:22, mcasas wrote: > Should we s/-1/media::VideoCaptureBufferPool::kInvalidId/ ? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by chfremer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mcasas@chromium.org, miu@chromium.org Link to the patchset: https://codereview.chromium.org/2607203002/#ps140001 (title: "mcasas comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1486412779547610,
"parent_rev": "22094236e8ef012d05e313c6e77c07a71addb37e", "commit_rev":
"10e14c24b244e7f58074ae74b0181cb3cb60551e"}
Message was sent while issue was closed.
Description was changed from
==========
[Mojo Video Capture] Retire buffers when Android Chromium goes to the background
Functional change:
Before this CL, when Android Chromium went to the background while video was
capturing, it would release both the VCDevice and VCDeviceClient while keeping
the VCBufferPool and the buffers already shared with Renderers alive. After this
CL, when going to the background, we also release the VCBufferPool and the
buffers.
Motivation:
For the Mojofication, we want to move the VCDevice, VCDeviceClient, and
VCBufferPool behind an abstraction (let's refer to it as AbstractDevice) and run
them in a separate process. With that, shutting down VCDevice, VCDeviceClient
while keeping VCBufferPool and shared buffers alive would require exposing this
as some new form of "suspended" mode for AbstractDevice (which is different from
the existing suspend/resume mechanism).
Instead of this, we opt for the less complex alternative solution, which is to
allow a complete shutdown of AbstractDevice, including the buffer pool, while
the app is in the background.
Execution:
In interface media::VideoFrameReceiver, replace OnBufferDestroyed(), which could
only be called while a buffer was not in use by the VideoFrameReceiver with
OnBufferRetired(), which may now be called while a buffer is still in use.
When AbstractDevice shuts down, it tells the VideoFrameReceiver that it is
"retiring" the buffers. The VideoFrameReceiver may keep consuming them as long
as it needs, since it holds shared ownership while it is consuming.
This CL is part of the Mojo Video Capture work. For the bigger picture,
see [1] CL11pre.
BUG=584797
TEST=
content_unittests --gtest_filter="*Video*",
Apprtc loopback on Debug,
Desktop Capture Example extension on Release
On Android content_shell, run video capture sample from [2], put to
background, then restore from background.
[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
[2] https://webrtc.github.io/samples/src/content/getusermedia/gum/
==========
to
==========
[Mojo Video Capture] Retire buffers when Android Chromium goes to the background
Functional change:
Before this CL, when Android Chromium went to the background while video was
capturing, it would release both the VCDevice and VCDeviceClient while keeping
the VCBufferPool and the buffers already shared with Renderers alive. After this
CL, when going to the background, we also release the VCBufferPool and the
buffers.
Motivation:
For the Mojofication, we want to move the VCDevice, VCDeviceClient, and
VCBufferPool behind an abstraction (let's refer to it as AbstractDevice) and run
them in a separate process. With that, shutting down VCDevice, VCDeviceClient
while keeping VCBufferPool and shared buffers alive would require exposing this
as some new form of "suspended" mode for AbstractDevice (which is different from
the existing suspend/resume mechanism).
Instead of this, we opt for the less complex alternative solution, which is to
allow a complete shutdown of AbstractDevice, including the buffer pool, while
the app is in the background.
Execution:
In interface media::VideoFrameReceiver, replace OnBufferDestroyed(), which could
only be called while a buffer was not in use by the VideoFrameReceiver with
OnBufferRetired(), which may now be called while a buffer is still in use.
When AbstractDevice shuts down, it tells the VideoFrameReceiver that it is
"retiring" the buffers. The VideoFrameReceiver may keep consuming them as long
as it needs, since it holds shared ownership while it is consuming.
This CL is part of the Mojo Video Capture work. For the bigger picture,
see [1] CL11pre.
BUG=584797
TEST=
content_unittests --gtest_filter="*Video*",
Apprtc loopback on Debug,
Desktop Capture Example extension on Release
On Android content_shell, run video capture sample from [2], put to
background, then restore from background.
[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
[2] https://webrtc.github.io/samples/src/content/getusermedia/gum/
Review-Url: https://codereview.chromium.org/2607203002
Cr-Commit-Position: refs/heads/master@{#448376}
Committed:
https://chromium.googlesource.com/chromium/src/+/10e14c24b244e7f58074ae74b018...
==========
Message was sent while issue was closed.
Committed patchset #6 (id:140001) as https://chromium.googlesource.com/chromium/src/+/10e14c24b244e7f58074ae74b018... |
