WebmMuxer-MediaRecorderHandler: thread hopping and data ownership

WebmMuxer-MediaRecorderHandler: thread hopping and data ownership

This CL addresses the global thread model between
VideoTrackRecorder - MediaRecorderHandler - WebmMuxer.
The constraints: all of them are created on Main Renderer
thread, whereas VTR frames come via Renderer IO thread.
The final result goes to Blink on Main Renderer Thread.

VideoTrackRecorder (IO) sends frames to WebmMuxer,
that is destroyed on Main Render thread. This causes a
race between WebmMuxer dtor and (the last) frame in
flight. This race was found during integration tests.

To address this issue, this CL moves WebmMuxer
to Main render thread completely, and the frames coming
from VTR on IO thread are trampolined on
MediaRecorderHandler thanks to BindToCurrentLoop.

To clarify data ownership across all these thread hops,
the encoded/contained data is managed as a
scoped_ptr<std::string>. This is OK since that data is
not shared, but passed from stage to stage
(and StringPiece only works in a single thread
environment since the data is not copied but managed
as unowned pointer).

Unittests, docs and headers adapted.

BUG=262211
TESTS=All appropriate UTs passing and also the whole
shebang test in https://crrev.com/1354863002/

Committed: https://crrev.com/a46a0503424df3f5f14c9b6422cdf97a26b9d45b
Cr-Commit-Position: refs/heads/master@{#350234}

[mcasas, 2015-09-17 00:49:59]

Patchset #1 (id:1) has been deleted

[mcasas, 2015-09-17 00:53:59]

mcasas@chromium.org changed reviewers:
+ ajose@chromium.org, emircan@chromium.org

[mcasas, 2015-09-17 00:53:59]

ajose@, ermican@ PTAL

[mcasas, 2015-09-17 14:57:55]

Patchset #1 (id:20001) has been deleted

[ajose, 2015-09-17 21:35:27]

lgtm

https://codereview.chromium.org/1351473006/diff/40001/media/capture/webm_muxer.h
File media/capture/webm_muxer.h (right):

https://codereview.chromium.org/1351473006/diff/40001/media/capture/webm_muxe...
media/capture/webm_muxer.h:48: static void
FinalizeSegment(scoped_ptr<mkvmuxer::Segment> segment);
Can this be below the ctr? 

https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...

[mcasas, 2015-09-18 02:08:04]

Patchset #2 (id:60001) has been deleted

[mcasas, 2015-09-18 02:08:27]

mcasas@chromium.org changed reviewers:
+ miu@chromium.org

[mcasas, 2015-09-18 02:08:28]

miu@ PTAL

emircan@ ping

[miu, 2015-09-21 19:39:43]

not lgtm:

https://codereview.chromium.org/1351473006/diff/80001/content/renderer/media/...
File content/renderer/media/media_recorder_handler.cc (right):

https://codereview.chromium.org/1351473006/diff/80001/content/renderer/media/...
content/renderer/media/media_recorder_handler.cc:90:
base::Bind(&media::WebmMuxer::OnEncodedVideo, webm_muxer_);
This does not call for ref-counting WebmMuxer.  Why not:

  media::BindToCurrentLoop(
      base::Bind(&MediaRecorderHandler::OnEncodedVideo,
                 weak_factory_.GetWeakPtr()))

...and then...

void MediaRecorderHandler::OnEncodedVideo(...) {
  DCHECK(main_render_thread_checker_.CalledOnValidThread());
  if (webm_muxer_)
    webm_muxer_->OnEncodedVideo(...);
}

[mcasas, 2015-09-21 20:22:18]

https://codereview.chromium.org/1351473006/diff/80001/content/renderer/media/...
File content/renderer/media/media_recorder_handler.cc (right):

https://codereview.chromium.org/1351473006/diff/80001/content/renderer/media/...
content/renderer/media/media_recorder_handler.cc:90:
base::Bind(&media::WebmMuxer::OnEncodedVideo, webm_muxer_);
On 2015/09/21 19:39:43, miu wrote:
> This does not call for ref-counting WebmMuxer.  Why not:
> 
>   media::BindToCurrentLoop(
>       base::Bind(&MediaRecorderHandler::OnEncodedVideo,
>                  weak_factory_.GetWeakPtr()))
> 
> ...and then...
> 
> void MediaRecorderHandler::OnEncodedVideo(...) {
>   DCHECK(main_render_thread_checker_.CalledOnValidThread());
>   if (webm_muxer_)
>     webm_muxer_->OnEncodedVideo(...);
> }

Yes, that's another possibility, essentially bouncing on 
MediaRecorderHandler to keep WebmMuxer not ref-counted,
and I considered this indeed.

But then we'd be using MediaRecorderHandler::weak_factory_
for PostTasks being used from Render IO thread, right?
Note that VideoTrackRecorder lives on and sends frames from 
the Renderer IO thread, and thus 
base::Bind(&MediaRecorderHandler::OnEncodedVideo,
                 weak_factory_.GetWeakPtr())
would be executed on IO thread, whereas |weak_factory_|
can only be invalidated on Main Render Thread. And we're
back to the situation before

(Note: |weak_factory_| is there for doing the jump between
WebmMuxer and its callback MediaRecorderHandler::WriteData(),
all of those on Main Render thread.

IOW, since we have
- creation of stuff on thread A
- bindings for frame callbacks on thread B
- a final hoop from thread B to thread A
someone has to be RefCountedThreadSafe so that frames
in flight are correctly processed while destruction is happening,
and that happens to be the place where the jump from A->B is 
done, currently WebmMuxer. Or do you propose to have a larger 
change, move WebmMuxer to Main Render thread, and make 
MediaRecorderHandler fully ref counted?

[miu, 2015-09-21 20:57:12]

https://codereview.chromium.org/1351473006/diff/80001/content/renderer/media/...
File content/renderer/media/media_recorder_handler.cc (right):

https://codereview.chromium.org/1351473006/diff/80001/content/renderer/media/...
content/renderer/media/media_recorder_handler.cc:90:
base::Bind(&media::WebmMuxer::OnEncodedVideo, webm_muxer_);
On 2015/09/21 20:22:18, mcasas wrote:
> Yes, that's another possibility, essentially bouncing on 
> MediaRecorderHandler to keep WebmMuxer not ref-counted,
> and I considered this indeed.
> 
> But then we'd be using MediaRecorderHandler::weak_factory_
> for PostTasks being used from Render IO thread, right?

That's why, in my example, I wrapped the base::Bind() in a
media::BindToCurrentLoop().  This means:

1. The render IO thread runs the callback provided by BindToCurrentLoop().  This
causes a task with the inner callback to be posted to the main thread.

2. The main thread checks the weak pointer is valid and, if so, calls
MediaRecorderHander::OnEncodedVideo().

3. Profit.

[mcasas, 2015-09-21 22:00:19]

Patchset #3 (id:100001) has been deleted

[mcasas, 2015-09-21 22:10:42]

Patchset #3 (id:120001) has been deleted

[mcasas, 2015-09-21 22:15:16]

Patchset #3 (id:140001) has been deleted

[mcasas, 2015-09-21 22:20:46]

Patchset #3 (id:160001) has been deleted

[mcasas, 2015-09-21 22:25:47]

miu@ PTAL

big changes - I suggest checking the whole diff, not the latest delta.

Also updated the description.

[miu, 2015-09-22 04:11:08]

Much better.  I should say that I'm not against ref-counting for no good reason.
 ;-)  Ref-counting complicates ownership semantics, makes it much harder to
reason how long an object will live, and can complicate tear-down code.  It can
often be eliminated with simple design changes.  There's also a TON of history
in Chrome Media code where the use of ref-counting got way out of hand and
caused a huge mess that scherkus@ and others spent over a year untangling.

This CL is a great example: By getting rid of ref-counting, you've made all
future maintenance on the WebmMuxer class much simpler.  That's because none of
its code needs to be thread-safe, and this is appropriate given that it is a
utility class for emitting structured data.

Just minor things left:

https://codereview.chromium.org/1351473006/diff/170001/content/renderer/media...
File content/renderer/media/media_recorder_handler.cc (right):

https://codereview.chromium.org/1351473006/diff/170001/content/renderer/media...
content/renderer/media/media_recorder_handler.cc:10: #include
"base/strings/stringprintf.h"
Is this needed?  Doesn't seem to be used.

https://codereview.chromium.org/1351473006/diff/170001/content/renderer/media...
content/renderer/media/media_recorder_handler.cc:105: webm_muxer_.reset(NULL);
nit: no arg needed to reset()

https://codereview.chromium.org/1351473006/diff/170001/content/renderer/media...
File content/renderer/media/media_recorder_handler.h (right):

https://codereview.chromium.org/1351473006/diff/170001/content/renderer/media...
content/renderer/media/media_recorder_handler.h:64: void
WriteData(scoped_ptr<std::string> data);
FWICT, there was no need to change the WriteData() method.  See comment in
webm_muxer.cc.

https://codereview.chromium.org/1351473006/diff/170001/media/capture/webm_mux...
File media/capture/webm_muxer.cc (right):

https://codereview.chromium.org/1351473006/diff/170001/media/capture/webm_mux...
media/capture/webm_muxer.cc:102:
write_data_callback_.Run(scoped_ptr<std::string>(
The extra copy here is unnecessary since the call is synchronous and not a task
posted across threads.  You should just go back to passing the StringPiece.

https://codereview.chromium.org/1351473006/diff/170001/media/capture/webm_mux...
File media/capture/webm_muxer.h (right):

https://codereview.chromium.org/1351473006/diff/170001/media/capture/webm_mux...
media/capture/webm_muxer.h:33: // Main Render thread; notably, it receives
OnEncodedVideo()s on that thread.
Up to you, but the comment should either:

1. Just say, "WebmMuxer is designed for single-threaded use throughout." or
"WebmMuxer is not thread-safe."

...or...

2. Also mention that the |write_data_callback| is invoked by the same thread
calling OnEncodedVideo().

IMO, #1 is clearer and simpler.

[mcasas, 2015-09-22 16:58:41]

miu@ PTAL

https://codereview.chromium.org/1351473006/diff/170001/content/renderer/media...
File content/renderer/media/media_recorder_handler.cc (right):

https://codereview.chromium.org/1351473006/diff/170001/content/renderer/media...
content/renderer/media/media_recorder_handler.cc:10: #include
"base/strings/stringprintf.h"
On 2015/09/22 04:11:08, miu wrote:
> Is this needed?  Doesn't seem to be used.

Done.
(Argh! I've tried hard to remove unused includes but
missed this one).

https://codereview.chromium.org/1351473006/diff/170001/content/renderer/media...
content/renderer/media/media_recorder_handler.cc:105: webm_muxer_.reset(NULL);
On 2015/09/22 04:11:08, miu wrote:
> nit: no arg needed to reset()

Done.

https://codereview.chromium.org/1351473006/diff/170001/content/renderer/media...
File content/renderer/media/media_recorder_handler.h (right):

https://codereview.chromium.org/1351473006/diff/170001/content/renderer/media...
content/renderer/media/media_recorder_handler.h:64: void
WriteData(scoped_ptr<std::string> data);
On 2015/09/22 04:11:08, miu wrote:
> FWICT, there was no need to change the WriteData() method.  See comment in
> webm_muxer.cc.

Acknowledged.

https://codereview.chromium.org/1351473006/diff/170001/media/capture/webm_mux...
File media/capture/webm_muxer.cc (right):

https://codereview.chromium.org/1351473006/diff/170001/media/capture/webm_mux...
media/capture/webm_muxer.cc:102:
write_data_callback_.Run(scoped_ptr<std::string>(
On 2015/09/22 04:11:08, miu wrote:
> The extra copy here is unnecessary since the call is synchronous and not a
task
> posted across threads.  You should just go back to passing the StringPiece.

Done.

FWIW I find StringPiece semantics a bit confusing
since they relate to String, but StringPiece does not
copy contents :?

https://codereview.chromium.org/1351473006/diff/170001/media/capture/webm_mux...
File media/capture/webm_muxer.h (right):

https://codereview.chromium.org/1351473006/diff/170001/media/capture/webm_mux...
media/capture/webm_muxer.h:33: // Main Render thread; notably, it receives
OnEncodedVideo()s on that thread.
On 2015/09/22 04:11:08, miu wrote:
> Up to you, but the comment should either:
> 
> 1. Just say, "WebmMuxer is designed for single-threaded use throughout." or
> "WebmMuxer is not thread-safe."
> 
> ...or...
> 
> 2. Also mention that the |write_data_callback| is invoked by the same thread
> calling OnEncodedVideo().
> 
> IMO, #1 is clearer and simpler.

#1 it is !

[miu, 2015-09-22 19:10:07]

lgtm

https://codereview.chromium.org/1351473006/diff/170001/media/capture/webm_mux...
File media/capture/webm_muxer.cc (right):

https://codereview.chromium.org/1351473006/diff/170001/media/capture/webm_mux...
media/capture/webm_muxer.cc:102:
write_data_callback_.Run(scoped_ptr<std::string>(
On 2015/09/22 16:58:41, mcasas wrote:
> On 2015/09/22 04:11:08, miu wrote:
> > The extra copy here is unnecessary since the call is synchronous and not a
> task
> > posted across threads.  You should just go back to passing the StringPiece.
> 
> Done.
> 
> FWIW I find StringPiece semantics a bit confusing
> since they relate to String, but StringPiece does not
> copy contents :?

The purpose of StringPiece is to reference substrings instead of making and
passing a copy.

Docs:
https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/strin...

https://codereview.chromium.org/1351473006/diff/190001/media/capture/webm_mux...
File media/capture/webm_muxer.h (right):

https://codereview.chromium.org/1351473006/diff/190001/media/capture/webm_mux...
media/capture/webm_muxer.h:41: using WriteDataCB = base::Callback<void(const
base::StringPiece&)>;
BTW--Now that I looked up the docs for StringPiece (see other comment), I
noticed they suggest passing StringPiece by value rather than by const-ref. 
Consider making that change before committing.

[mcasas, 2015-09-22 20:20:03]

mcasas@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[mcasas, 2015-09-22 20:20:04]

DaleCurtis: media/capture/webm_muxer* Owners RS/ PTAL

https://codereview.chromium.org/1351473006/diff/170001/media/capture/webm_mux...
File media/capture/webm_muxer.cc (right):

https://codereview.chromium.org/1351473006/diff/170001/media/capture/webm_mux...
media/capture/webm_muxer.cc:102:
write_data_callback_.Run(scoped_ptr<std::string>(
On 2015/09/22 19:10:06, miu wrote:
> On 2015/09/22 16:58:41, mcasas wrote:
> > On 2015/09/22 04:11:08, miu wrote:
> > > The extra copy here is unnecessary since the call is synchronous and not a
> > task
> > > posted across threads.  You should just go back to passing the
StringPiece.
> > 
> > Done.
> > 
> > FWIW I find StringPiece semantics a bit confusing
> > since they relate to String, but StringPiece does not
> > copy contents :?
> 
> The purpose of StringPiece is to reference substrings instead of making and
> passing a copy.
> 
> Docs:
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/strin...

Heh thanks for the reference.

What I meant is that base::StringPiece and std::string are similar,
since they share e.g. the constructor with a char* and a length, 
but base::StringPiece does not copy the contents pointed by
the said first argument. Perhaps I'd like to see it called, e.g.
StringPieceRef or similar, to indicate that StringPiece is not a
string itself, but a reference to one :) 
IOW the documentation is nice, but having a name that reveals
intent is much better.

https://codereview.chromium.org/1351473006/diff/190001/media/capture/webm_mux...
File media/capture/webm_muxer.h (right):

https://codereview.chromium.org/1351473006/diff/190001/media/capture/webm_mux...
media/capture/webm_muxer.h:41: using WriteDataCB = base::Callback<void(const
base::StringPiece&)>;
On 2015/09/22 19:10:06, miu wrote:
> BTW--Now that I looked up the docs for StringPiece (see other comment), I
> noticed they suggest passing StringPiece by value rather than by const-ref. 
> Consider making that change before committing.

Done.

[DaleCurtis, 2015-09-22 20:28:39]

miu@, mcasas@ you two should have a top-level per-file OWNERS for these.

rs lgtm based on miu@'s approval and that statement :)

[mcasas, 2015-09-22 20:31:48]

The CQ bit was checked by mcasas@chromium.org

[mcasas, 2015-09-22 20:31:49]

The patchset sent to the CQ was uploaded after l-g-t-m from ajose@chromium.org,
miu@chromium.org
Link to the patchset: https://codereview.chromium.org/1351473006/#ps210001
(title: "StringPiece passed by value")

[commit-bot: I haz the power, 2015-09-22 20:33:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1351473006/210001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1351473006/210001

[commit-bot: I haz the power, 2015-09-22 21:21:41]

Committed patchset #5 (id:210001)

[commit-bot: I haz the power, 2015-09-22 21:53:01]

Patchset 5 (id:??) landed as
https://crrev.com/a46a0503424df3f5f14c9b6422cdf97a26b9d45b
Cr-Commit-Position: refs/heads/master@{#350234}