Extended proxy abstraction, to call certain methods to the worker thread.

Extended proxy abstraction, to call certain methods to the worker thread.

Extracted from cl https://codereview.webrtc.org/1766653002/, where
AddOrUpdateSink results in a deadlock.

BUG=webrtc:5426
Committed: https://crrev.com/5b68ab50bbd6ada3725aa1aeac3e87242986b6be
Cr-Commit-Position: refs/heads/master@{#12281}

[nisse-webrtc, 2016-04-05 10:13:45]

nisse@webrtc.org changed reviewers:
+ perkj@webrtc.org, pthatcher@webrtc.org

[nisse-webrtc, 2016-04-05 10:13:46]

https://codereview.webrtc.org/1861633002/diff/1/webrtc/api/videotrack.cc
File webrtc/api/videotrack.cc (right):

https://codereview.webrtc.org/1861633002/diff/1/webrtc/api/videotrack.cc#newc...
webrtc/api/videotrack.cc:39: //
RTC_DCHECK(thread_checker_.CalledOnValidThread());
Should we have another rtc::ThreadChecker, bound to the worker thread?

https://codereview.webrtc.org/1861633002/diff/1/webrtc/api/videotracksource.cc
File webrtc/api/videotracksource.cc (right):

https://codereview.webrtc.org/1861633002/diff/1/webrtc/api/videotracksource.c...
webrtc/api/videotracksource.cc:40: RTC_CHECK(worker_thread_->IsCurrent());
These RTC_CHECKs are now the only use of |worker_thread_|. Delete
worker_thread_, and replace by an rtc::ThreadChecker?

[perkj_webrtc, 2016-04-05 11:02:37]

https://codereview.webrtc.org/1861633002/diff/1/webrtc/api/proxy.h
File webrtc/api/proxy.h (right):

https://codereview.webrtc.org/1861633002/diff/1/webrtc/api/proxy.h#newcode303
webrtc/api/proxy.h:303: : signal_thread_(signal_thread), c_(c) {}               
           \
name signaling_thread_

https://codereview.webrtc.org/1861633002/diff/1/webrtc/api/videotrack.cc
File webrtc/api/videotrack.cc (right):

https://codereview.webrtc.org/1861633002/diff/1/webrtc/api/videotrack.cc#newc...
webrtc/api/videotrack.cc:39: //
RTC_DCHECK(thread_checker_.CalledOnValidThread());
On 2016/04/05 10:13:46, nisse-webrtc wrote:
> Should we have another rtc::ThreadChecker, bound to the worker thread?

yes please.

https://codereview.webrtc.org/1861633002/diff/1/webrtc/api/videotrack.cc#newc...
webrtc/api/videotrack.cc:59: // worker thread.
Can you verify if this is true for remote video sources?

https://codereview.webrtc.org/1861633002/diff/1/webrtc/api/videotracksource.cc
File webrtc/api/videotracksource.cc (right):

https://codereview.webrtc.org/1861633002/diff/1/webrtc/api/videotracksource.c...
webrtc/api/videotracksource.cc:40: RTC_CHECK(worker_thread_->IsCurrent());
On 2016/04/05 10:13:46, nisse-webrtc wrote:
> These RTC_CHECKs are now the only use of |worker_thread_|. Delete
> worker_thread_, and replace by an rtc::ThreadChecker?

yes please and name accordingly.

[nisse-webrtc, 2016-04-05 11:43:55]

https://codereview.webrtc.org/1861633002/diff/1/webrtc/api/proxy.h
File webrtc/api/proxy.h (right):

https://codereview.webrtc.org/1861633002/diff/1/webrtc/api/proxy.h#newcode303
webrtc/api/proxy.h:303: : signal_thread_(signal_thread), c_(c) {}               
           \
On 2016/04/05 11:02:37, perkj_webrtc wrote:
> name signaling_thread_

Done.

https://codereview.webrtc.org/1861633002/diff/1/webrtc/api/videotrack.cc
File webrtc/api/videotrack.cc (right):

https://codereview.webrtc.org/1861633002/diff/1/webrtc/api/videotrack.cc#newc...
webrtc/api/videotrack.cc:39: //
RTC_DCHECK(thread_checker_.CalledOnValidThread());
On 2016/04/05 10:13:46, nisse-webrtc wrote:
> Should we have another rtc::ThreadChecker, bound to the worker thread?

Done.

https://codereview.webrtc.org/1861633002/diff/1/webrtc/api/videotrack.cc#newc...
webrtc/api/videotrack.cc:59: // worker thread.
On 2016/04/05 11:02:37, perkj_webrtc wrote:
> Can you verify if this is true for remote video sources? 

I'm looking at the VideoRtpReceiver constructor. It creates a proxy for the
VideoTrack, but not for the VideoTrackSource. I guess I should add another proxy
there, since it will be returned to the application if it calls GetSource?

https://codereview.webrtc.org/1861633002/diff/1/webrtc/api/videotracksource.cc
File webrtc/api/videotracksource.cc (right):

https://codereview.webrtc.org/1861633002/diff/1/webrtc/api/videotracksource.c...
webrtc/api/videotracksource.cc:40: RTC_CHECK(worker_thread_->IsCurrent());
On 2016/04/05 11:02:37, perkj_webrtc wrote:
> On 2016/04/05 10:13:46, nisse-webrtc wrote:
> > These RTC_CHECKs are now the only use of |worker_thread_|. Delete
> > worker_thread_, and replace by an rtc::ThreadChecker?
> 
> yes please and name accordingly.

I'm deleting the constructor argument as well (there's no obvious way to use it
to initialize the thread checker). Have to add a worker_thread_ variable in the
VideoCapturerTrackSource instead.

[nisse-webrtc, 2016-04-05 12:23:27]

Wrap the VideoTrackSource for a remote track in a proxy object, before it is
passed to VideoTrack::Create.

[perkj_webrtc, 2016-04-05 12:36:56]

lgtm with a nit

https://codereview.webrtc.org/1861633002/diff/40001/webrtc/api/proxy.h
File webrtc/api/proxy.h (right):

https://codereview.webrtc.org/1861633002/diff/40001/webrtc/api/proxy.h#newcod...
webrtc/api/proxy.h:310: static rtc::scoped_refptr<C> Create(                    
           \
nit: formatting of line length.

[nisse-webrtc, 2016-04-05 12:43:34]

https://codereview.webrtc.org/1861633002/diff/40001/webrtc/api/proxy.h
File webrtc/api/proxy.h (right):

https://codereview.webrtc.org/1861633002/diff/40001/webrtc/api/proxy.h#newcod...
webrtc/api/proxy.h:310: static rtc::scoped_refptr<C> Create(                    
           \
On 2016/04/05 12:36:55, perkj_webrtc wrote:
> nit: formatting of line length.

Not sure exactly what you're suggesting, but I changed it back to a single line.

[nisse-webrtc, 2016-04-06 08:23:39]

The CQ bit was checked by nisse@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-06 08:23:50]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1861633002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1861633002/60001

[commit-bot: I haz the power, 2016-04-06 10:37:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-06 10:37:51]

Dry run: This issue passed the CQ dry run.

[nisse-webrtc, 2016-04-07 09:27:25]

nisse@webrtc.org changed reviewers:
+ tommi@webrtc.org

[nisse-webrtc, 2016-04-07 09:27:26]

+tommi

[tommi, 2016-04-07 11:50:03]

lgtm % the below minor comments

https://codereview.webrtc.org/1861633002/diff/60001/webrtc/api/mediastreamtra...
File webrtc/api/mediastreamtrackproxy.h (right):

https://codereview.webrtc.org/1861633002/diff/60001/webrtc/api/mediastreamtra...
webrtc/api/mediastreamtrackproxy.h:38: BEGIN_WORKER_PROXY_MAP(VideoTrack)
Down the  line, I'd like to rather rename the other macro, since that one will
likely be going away.
Can you put down a TODO to rename this one to [BEGIN|END]_PROXY_MAP and the
other one to e.g. [BEGIN|END]_SIGNALING_PROXY_MAP?

https://codereview.webrtc.org/1861633002/diff/60001/webrtc/api/proxy.h
File webrtc/api/proxy.h (right):

https://codereview.webrtc.org/1861633002/diff/60001/webrtc/api/proxy.h#newcod...
webrtc/api/proxy.h:299: class c##Proxy : public c##Interface {                  
               \
nit: Could change this to:
class c##SignalingProxy : public c##Interface { 

The only thing it does is make the symbols a bit clearer, but that can be useful
sometimes.

https://codereview.webrtc.org/1861633002/diff/60001/webrtc/api/proxy.h#newcod...
webrtc/api/proxy.h:319: : signaling_thread_(signaling_thread),                  
         \
indent looks off... does "git cl format" ignore this since it's a macro?

https://codereview.webrtc.org/1861633002/diff/60001/webrtc/api/videosourcepro...
File webrtc/api/videosourceproxy.h (right):

https://codereview.webrtc.org/1861633002/diff/60001/webrtc/api/videosourcepro...
webrtc/api/videosourceproxy.h:24: PROXY_CONSTMETHOD0(SourceState, state)
nit: can you indent this as is done in the other file?

[nisse-webrtc, 2016-04-07 12:50:33]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2016-04-07 12:50:33]

The patchset sent to the CQ was uploaded after l-g-t-m from perkj@webrtc.org,
tommi@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1861633002/#ps100001 (title:
"Rebase.")

[commit-bot: I haz the power, 2016-04-07 12:50:37]

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

[nisse-webrtc, 2016-04-07 12:51:29]

Didn't do any renaming now, but I added a TODO about it.

https://codereview.webrtc.org/1861633002/diff/60001/webrtc/api/proxy.h
File webrtc/api/proxy.h (right):

https://codereview.webrtc.org/1861633002/diff/60001/webrtc/api/proxy.h#newcod...
webrtc/api/proxy.h:299: class c##Proxy : public c##Interface {                  
               \
On 2016/04/07 11:50:03, tommi-webrtc wrote:
> nit: Could change this to:
> class c##SignalingProxy : public c##Interface { 
> 
> The only thing it does is make the symbols a bit clearer, but that can be
useful
> sometimes.

It changes the name of the constructor. I think I'd prefer to postpone that to
renaming, to limit changes to code using the unchanged variant of the proxy.

https://codereview.webrtc.org/1861633002/diff/60001/webrtc/api/proxy.h#newcod...
webrtc/api/proxy.h:319: : signaling_thread_(signaling_thread),                  
         \
On 2016/04/07 11:50:03, tommi-webrtc wrote:
> indent looks off... does "git cl format" ignore this since it's a macro?

git cl format wants to change indentation in this file completely. I didn't do
that, to avoid changes to the parts I haven't touch. Could do a reformat in a
the same followup as the renaming.

https://codereview.webrtc.org/1861633002/diff/60001/webrtc/api/videosourcepro...
File webrtc/api/videosourceproxy.h (right):

https://codereview.webrtc.org/1861633002/diff/60001/webrtc/api/videosourcepro...
webrtc/api/videosourceproxy.h:24: PROXY_CONSTMETHOD0(SourceState, state)
On 2016/04/07 11:50:03, tommi-webrtc wrote:
> nit: can you indent this as is done in the other file?

Sure. (git cl format would prefer the opposite change, though).

[commit-bot: I haz the power, 2016-04-07 14:45:58]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-04-07 14:46:08]

Description was changed from

==========
Extended proxy abstraction, to call certain methods to the worker thread.

Extracted from cl https://codereview.webrtc.org/1766653002/, where
AddOrUpdateSink results in a deadlock.

BUG=webrtc:5426
==========

to

==========
Extended proxy abstraction, to call certain methods to the worker thread.

Extracted from cl https://codereview.webrtc.org/1766653002/, where
AddOrUpdateSink results in a deadlock.

BUG=webrtc:5426

Committed: https://crrev.com/5b68ab50bbd6ada3725aa1aeac3e87242986b6be
Cr-Commit-Position: refs/heads/master@{#12281}
==========

[commit-bot: I haz the power, 2016-04-07 14:46:09]

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