Break tight coupling of VideoCaptureDeviceClient to renderer_host

content/browser/renderer_host/media/video_capture_controller.cc

Index: content/browser/renderer_host/media/video_capture_controller.cc
diff --git a/content/browser/renderer_host/media/video_capture_controller.cc b/content/browser/renderer_host/media/video_capture_controller.cc
index d72cb3dea7174a2c64f229300efd8ebc70817c54..df225b24e75d7f25ffb2fb8e6d195488660d73d0 100644
--- a/content/browser/renderer_host/media/video_capture_controller.cc
+++ b/content/browser/renderer_host/media/video_capture_controller.cc
@@ -21,6 +21,7 @@
 #include "content/browser/renderer_host/media/media_stream_manager.h"
 #include "content/browser/renderer_host/media/video_capture_buffer_pool.h"
 #include "content/browser/renderer_host/media/video_capture_device_client.h"
+#include "content/browser/renderer_host/media/video_capture_gpu_jpeg_decoder.h"
 #include "content/browser/renderer_host/media/video_capture_manager.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/common/content_switches.h"
@@ -79,6 +80,50 @@ void ReturnVideoFrame(const scoped_refptr<VideoFrame>& video_frame,
 #endif
 }
 
+std::unique_ptr<VideoCaptureJpegDecoder> CreateGpuJpegDecoder(
+    const VideoCaptureJpegDecoder::DecodeDoneCB& decode_done_cb) {
+  return base::MakeUnique<VideoCaptureGpuJpegDecoder>(decode_done_cb);
+}
+
+// Decorator for VideoFrameReceiver that forwards all incoming calls
+// to the Browser IO thread.
+class PostOnIOThreadVideoFrameReceiver : public VideoFrameReceiver {
+ public:
+  PostOnIOThreadVideoFrameReceiver(

[miu, 2016/09/13 00:33:29]

Sorry, didn't see this the last time around: Please add 'explicit' keyword for
one-arg ctors to avoid implicit construction.

[chfremer, 2016/09/14 16:26:01]

Done.

+      const base::WeakPtr<VideoFrameReceiver>& receiver)
+      : receiver_(receiver) {}
+
+  void OnIncomingCapturedVideoFrame(

[miu, 2016/09/12 20:16:46]

Please use base::ThreadChecker to ensure all these methods are being called by
the same thread. This ensures the sequence of method calls is preserved when
tasks are run on the IO thread.

[chfremer, 2016/09/12 22:23:33]

Hmm, I am not sure I agree that this would be an improvement.

If clients make calls to this class from multiple threads, they already cannot
expect the calls to be processed in-order. And if clients want to call from
multiple threads while also making sure that no overlapping calls happen, they
should be allowed to and they are already guaranteed to have the order
preserved.

Am I missing something here?

[miu, 2016/09/13 00:33:29]

Generally, in Chromium code, we try to adhere to the "one thread <--> one class"
design guideline as much as possible. (Granted, there are lots of situations
where it is not possible and/or not desirable.)

There's actually no problem with the use of this class, in this change, they way
it is. The problem stems from how this code will evolve over the years: For
example, it could be that, several years from now, another developer comes along
without all the context, takes a quick look, and makes a change that breaks some
assumption about the multi-threaded access to this interface and/or which
sequences of method calls were legal. They introduce a change, it is reviewed by
another developer who also doesn't have all the context, and it introduces a
subtle bug that only surfaces once released into the wild to millions of users.

Often, it's a good idea to restrict the semantics of an interface/impl as much
as possible, only to relax those restrictions later when a need arises. In this
case, using a ThreadChecker restricts the potential for new bugs to those that
would only exist for single-threaded method calling. Such bugs are much easier
to find during development and/or via unit testing.

Again, most code review comments are suggestions. So, take it or leave it. :)

[chfremer, 2016/09/14 16:26:01]

Thanks. This insight is super useful for me learning the ways of Chromium.
Since I am still new to the project, I am trying to find a good set of practices
that can be applied in most situations, so please allow me to dig into this a
little more.

I agree that it is probably a good idea to restrict classes to be used from a
single thread by default and only relax this restriction where needed, for the
benefit of being able to add sanity checks that verify the single-thread usage
and protect us from subtle and hard-to-find bugs.

Since I am looking for practices that can/should be applied in most situations,
I am wondering if using a ThreadChecker to enforce single-threaded usage should
then be something I apply to _all_ classes that do not explicitly require
multi-threaded (not necessarily concurrent) operation. I do not see any
particular reason why this practice would apply to this class more than to any
other, so consistency would guide me towards applying this practice everywhere.
Would that be okay from your perspective?

In the particular case of this class here, there is one concern that I have
about doing it as part of this CL: It would introduce a change of behavior.
Before this CL, there is no enforcement of these calls to arrive from a single
thread. Calls are initiated from the platform-specific implementations of
media::VideoCaptureDevice (and are passing through VideoCaptureDeviceClient).
Without checking every implementation carefully, I feel it is very well possible
that for some of those implementations calls are issued from different threads. 

If we want to make this change in behavior, I would prefer to do this in a
separate step.

[miu, 2016/09/16 21:10:25]

There is variance in styles/attitudes, depending on which part of the code base
you're in. AFAICT, the most general consensus is:

1. If a code module clearly never makes any references to threads, task runners,
etc., it should be assumed to be non-thread-safe.

2. In situations where a code module is not thread safe, but seems to be used by
a lot of code that cares about the threading strategy, people will often either:
a) use base::ThreadChecker anyway; or b) use base::NonThreadSafe.
Probably. Unfortunately, I'm struggling to explain more concretely. Umm...let
experience and intuition be your guide? ;-)

I lean towards safety, but also practicality. So, I wouldn't add a whole bunch
of code to do thread DCHECKing if I feel its fairly obvious to other developers
that the code I wrote wasn't intended to be thread-safe. After all, the reason
to do all this is to avoid unpleasantly surprising yourself and other developers
down the road. The ROI here is roughly: investment="additional effort of writing
checks and in sometimes being wrong and having to fix them", reward="eliminate
time spent resolving bugs + peace of mind" So, your own judgement is needed to
quantify these terms.
Yeah, it's possible they might be called from different threads. With this
interface, FWICT, there *might* be a sequence relationship between
OnIncomingCapturedVideoFrame() and OnBufferDestroyed(), but as for the other two
methods, call sequence shouldn't really matter. One possibility here is to use
ThreadChecker only on those two methods (the ones where sequence matters).
However, if you think it's unlikely there's a sequence relationship, maybe no
ThreadCheck'ing is appropriate here.

As for checking all the platform-specific implementations: Yeah, that can be
tedious. One suggestion: Add the checks and then do a `git cl try` to do a CQ
dry-run, which will runs tests on all platforms. If things are green, go ahead
and commit the change. After that, there are plenty of other things to catch
problems (e.g., more build configurations, and the ASAN/TSAN builds to test
thread-safety).
That sounds reasonable to me.

[chfremer, 2016/09/17 00:09:21]

Thanks for this explanation, which I find helpful.
It really seems that a case-by-case judgement call is needed.
Excellent suggestion. I added this as planned CL1.6.1.3 to the design doc.

+      std::unique_ptr<media::VideoCaptureDevice::Client::Buffer> buffer,
+      const scoped_refptr<media::VideoFrame>& frame) override {
+    BrowserThread::PostTask(
+        BrowserThread::IO, FROM_HERE,
+        base::Bind(&VideoFrameReceiver::OnIncomingCapturedVideoFrame, receiver_,
+                   base::Passed(&buffer), frame));
+  }
+
+  void OnError() override {
+    BrowserThread::PostTask(
+        BrowserThread::IO, FROM_HERE,
+        base::Bind(&VideoFrameReceiver::OnError, receiver_));
+  }
+
+  void OnLog(const std::string& message) override {
+    BrowserThread::PostTask(
+        BrowserThread::IO, FROM_HERE,
+        base::Bind(&VideoFrameReceiver::OnLog, receiver_, message));
+  }
+
+  void OnBufferDestroyed(int buffer_id_to_drop) override {
+    BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,

[miu, 2016/09/12 20:16:46]

This makes me wonder whether receiver_ needs to be a scoped_refptr<> instead.
The buffer must not be destroyed until the client is done using it, but what you
have here with WeakPtr seems to imply the buffer can be destroyed out from
underneath a client.

Or, are you depending on the operating system to keep the shared memory around
in the render process for as long as it's needed, and we shouldn't be worried
about this? If so, will this scheme work for GPU memory buffers and other types?
We won't be leaking memory in the GPU or GPU process?

[chfremer, 2016/09/12 22:23:33]

Thanks. This is an interesting question.
I haven't thought about this, because I am attempting to do a refactoring that I
believe does not change any of the existing behavior.

Please let me read up a bit more on how this mechanism is supposed to work
currently, and then I will respond back.

[chfremer, 2016/09/14 16:26:01]

After thinking about it more carefully, I don't think I fully understand what
you mean. 
I understand that a buffer must not be destroyed while someone is still using
it.
What I don't understand is how you infer a concern based on the fact that
|receiver_| is a WeakPtr<> (as opposed to a scoped_refptr<>).

After following the calls in the code, my current understanding is as follows.
1.) VideoCaptureDeviceClient calls VideoCaptureBufferPool::ReserveForProducer()
on behalf of a "Producer", e.g. a capture device.
2.) The VideoCaptureBufferPool may decide that in order to fulfill the request,
it needs to drop an existing buffer. If that is the case it returns the id of
the buffer it has dropped back to the VideoCaptureDeviceClient.
3.) The VideoCaptureDeviceClient calls OnBufferDestroyed() to notify the
"Consumer" that the buffer with the id is no longer valid.

Without having analyzed exhaustively, I assume that the VideoCaptureBufferPool
only decides to drop buffers that are known to no longer be in use by both
Producers and Consumers. The "hold" and "relinquish hold" methods are probably
offered to allow those clients to tell the VideoCaptureBufferPool when they are
done with using a buffer. 

This CL does not change anything about how this works.

[miu, 2016/09/16 21:10:25]

What I was thinking (I now realize is not quite right): I was assuming the
memory backing the buffer had to remain valid until OnBufferDestroyed() was
called. In fact, the naming suggests I already was wrong about that. ;-) So, I
thought that VideoCaptureController (the impl of VideoFrameReceiver), which owns
the VideoCaptureBufferPool, could destroy the buffer pool while the buffer
memory was still in use.
I took another look, and agree. The thing I didn't notice originally was that
VCDeviceClient held a WeakPtr to VCController. With your change, this becomes:
VCDeviceClient holds a strong ptr to VideoFrameReceiver, and this impl of
VideoFrameReceiver then holds the WeakPtr to VCController. So, the ownership of
objects and pointer validity should be exactly the same.

[chfremer, 2016/09/17 00:09:21]

Yes, that is exactly right. Adding the decorator between the original two
instances made it a bit hard to see at first.

+                            base::Bind(&VideoFrameReceiver::OnBufferDestroyed,
+                                       receiver_, buffer_id_to_drop));
+  }
+
+ private:
+  base::WeakPtr<VideoFrameReceiver> receiver_;
+};
+
 }  // anonymous namespace
 
 struct VideoCaptureController::ControllerClient {
@@ -148,7 +193,12 @@ std::unique_ptr<media::VideoCaptureDevice::Client>
 VideoCaptureController::NewDeviceClient() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   return base::MakeUnique<VideoCaptureDeviceClient>(
-      this->GetWeakPtrForIOThread(), buffer_pool_);
+      base::MakeUnique<PostOnIOThreadVideoFrameReceiver>(
+          this->GetWeakPtrForIOThread()),
+      buffer_pool_,
+      base::Bind(&CreateGpuJpegDecoder,
+                 base::Bind(&VideoFrameReceiver::OnIncomingCapturedVideoFrame,
+                            this->GetWeakPtrForIOThread())));
 }
 
 void VideoCaptureController::AddClient(
@@ -354,7 +404,7 @@ VideoCaptureController::~VideoCaptureController() {
                                    controller_clients_.end());
 }
 
-void VideoCaptureController::DoIncomingCapturedVideoFrameOnIOThread(
+void VideoCaptureController::OnIncomingCapturedVideoFrame(
     std::unique_ptr<media::VideoCaptureDevice::Client::Buffer> buffer,
     const scoped_refptr<VideoFrame>& frame) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
@@ -425,7 +475,7 @@ void VideoCaptureController::DoIncomingCapturedVideoFrameOnIOThread(
   buffer_pool_->HoldForConsumers(buffer_id, count);
 }
 
-void VideoCaptureController::DoErrorOnIOThread() {
+void VideoCaptureController::OnError() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   state_ = VIDEO_CAPTURE_STATE_ERROR;
 
@@ -436,13 +486,12 @@ void VideoCaptureController::DoErrorOnIOThread() {
   }
 }
 
-void VideoCaptureController::DoLogOnIOThread(const std::string& message) {
+void VideoCaptureController::OnLog(const std::string& message) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   MediaStreamManager::SendMessageToNativeLog("Video capture: " + message);
 }
 
-void VideoCaptureController::DoBufferDestroyedOnIOThread(
-    int buffer_id_to_drop) {
+void VideoCaptureController::OnBufferDestroyed(int buffer_id_to_drop) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   for (auto* client : controller_clients_) {