MediaStreamVideo*/VideoTrackAdapter and RTCVideoRenderer (small) cleanup

MediaStreamVideo*/VideoTrackAdapter and RTCVideoRenderer (small) cleanup

Some more cleanup. In this CL:

- VideoCapturerDelegate::OnStateUpdateOnRenderThread
 does not need OnRenderThread suffix since it lives on
 Render Thread exclusively. Removed the suffix.

- cosmetic changes (removed unnecessary {}, updated comments).

- media_stream_video_source.cc struct SourceVideoResolution
 does not need to be named, defined anonymously together
 with the array it's used for, |kVideoResolutions|.

- Some for loops are substituted with range-based versions.

- Some member vars are made const.

- Some methods are reordered to follow class declarations.

- MediaStreamVideoTrack::thread_checker_ is unnecessary and
 |main_render_thread_checker_| from parent class can be used
 instead. Similar story for WebRtcLocalAudioTrack.

- MediaStreamVideoTrack::FrameDeliverer uses void* to identify
 a MediaStreamSink, changed to typedef MediaStreamSink* VideoSinkId
and used in this class (IMHO this is more readable).

All these happened to be noticeable as I was re-reading
the code, some kind of state-of-the-pipeline review :)

[1] https://code.google.com/p/chromium/codesearch#chromium/src/remoting/host/audio_pump.cc&l=135

BUG=440843
TBR=avi@chromium.org
(Avi, I'm TBRing you since it's just removing an
unused 2-class forward declaration @
content/public/renderer/media_stream_video_sink.h)

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

[mcasas, 2015-03-05 03:16:44]

Patchset #1 (id:1) has been deleted

[mcasas, 2015-03-05 15:32:23]

Patchset #1 (id:20001) has been deleted

[mcasas, 2015-03-06 02:48:57]

Patchset #1 (id:40001) has been deleted

[mcasas, 2015-03-06 02:49:04]

Patchset #2 (id:80001) has been deleted

[mcasas, 2015-03-06 02:54:52]

Patchset #2 (id:100001) has been deleted

[mcasas, 2015-03-06 02:55:17]

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

[mcasas, 2015-03-06 02:55:18]

emircan@ PTAL

[mcasas, 2015-03-06 07:10:29]

Patchset #2 (id:120001) has been deleted

[mcasas, 2015-03-06 07:10:48]

Patchset #2 (id:140001) has been deleted

[mcasas, 2015-03-06 18:59:05]

Patchset #2 (id:160001) has been deleted

[mcasas, 2015-03-09 14:29:18]

Patchset #1 (id:60001) has been deleted

[mcasas, 2015-03-09 14:29:26]

Patchset #1 (id:180001) has been deleted

[mcasas, 2015-03-09 14:30:30]

Patchset #1 (id:200001) has been deleted

[mcasas, 2015-03-09 15:40:43]

Patchset #1 (id:220001) has been deleted

[mcasas, 2015-03-09 15:40:49]

Patchset #1 (id:240001) has been deleted

[mcasas, 2015-03-09 16:55:30]

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

[mcasas, 2015-03-09 16:55:30]

emircan@ ping

wolenetz@ RS OWNERS/PTAL/Fwd please.

[emircan, 2015-03-10 01:40:06]

lgtm % the points below.

https://codereview.chromium.org/976233002/diff/260001/content/renderer/media/...
File content/renderer/media/media_stream_video_capturer_source.cc (right):

https://codereview.chromium.org/976233002/diff/260001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:27: {320, 180}};
I would prefer the earlier implementation as the struct is defined with a type.
In case params need to be passed into a function or declared, type would be
required.

https://codereview.chromium.org/976233002/diff/260001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:207: for (const
auto& frame_rate : kVideoFrameRates) {
frame_rate is an int so copying by value would be faster than keeping a pointer
and dereferencing.

[mcasas, 2015-03-10 21:42:30]

wolenetz@ PTAL

Notwithstanding ermican@ comments, which I'll take
in before landing, of course.

[wolenetz, 2015-03-11 23:37:37]

rslgtm % nits I really think you need an RTC OWNER to CR at least some of these
changes (like thread membership and comments clarifications).

https://codereview.chromium.org/976233002/diff/260001/content/public/renderer...
File content/public/renderer/media_stream_video_sink.h (right):

https://codereview.chromium.org/976233002/diff/260001/content/public/renderer...
content/public/renderer/media_stream_video_sink.h:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
Currently, no OWNER of this file is attached to this CR.

https://codereview.chromium.org/976233002/diff/260001/content/renderer/media/...
File content/renderer/media/media_stream_video_capture_source_unittest.cc
(right):

https://codereview.chromium.org/976233002/diff/260001/content/renderer/media/...
content/renderer/media/media_stream_video_capture_source_unittest.cc:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
aside: why is this file called *_capture_* vs *_capturer_*?

https://codereview.chromium.org/976233002/diff/260001/content/renderer/media/...
File content/renderer/media/media_stream_video_track.cc (right):

https://codereview.chromium.org/976233002/diff/260001/content/renderer/media/...
content/renderer/media/media_stream_video_track.cc:29: typedef void*
VideoSinkId;
aside: is void* truly necessary, or can we have some more specific type?!

https://codereview.chromium.org/976233002/diff/260001/content/renderer/media/...
File content/renderer/media/media_stream_video_track.h (right):

https://codereview.chromium.org/976233002/diff/260001/content/renderer/media/...
content/renderer/media/media_stream_video_track.h:81: base::ThreadChecker
render_thread_checker_;
I'm confused: which thread_checker_ was previously referred to prior to this CL?
There is also a protected thread_checker_ in MediaStreamTrack.

https://codereview.chromium.org/976233002/diff/260001/content/renderer/media/...
File content/renderer/media/rtc_video_renderer.h (right):

https://codereview.chromium.org/976233002/diff/260001/content/renderer/media/...
content/renderer/media/rtc_video_renderer.h:25: // order to render video frames
provided from a MediaStreamVideoTrack, to  which
nit: s/  / /

[wolenetz, 2015-03-11 23:38:59]

On 2015/03/11 23:37:37, wolenetz wrote:
> rslgtm % nits I really think you need an RTC OWNER to CR at least some of
these
> changes (like thread membership and comments clarifications).
> 
>
https://codereview.chromium.org/976233002/diff/260001/content/public/renderer...
> File content/public/renderer/media_stream_video_sink.h (right):
> 
>
https://codereview.chromium.org/976233002/diff/260001/content/public/renderer...
> content/public/renderer/media_stream_video_sink.h:1: // Copyright 2013 The
> Chromium Authors. All rights reserved.
> Currently, no OWNER of this file is attached to this CR.
> 
>
https://codereview.chromium.org/976233002/diff/260001/content/renderer/media/...
> File content/renderer/media/media_stream_video_capture_source_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/976233002/diff/260001/content/renderer/media/...
> content/renderer/media/media_stream_video_capture_source_unittest.cc:1: //
> Copyright 2014 The Chromium Authors. All rights reserved.
> aside: why is this file called *_capture_* vs *_capturer_*?
> 
>
https://codereview.chromium.org/976233002/diff/260001/content/renderer/media/...
> File content/renderer/media/media_stream_video_track.cc (right):
> 
>
https://codereview.chromium.org/976233002/diff/260001/content/renderer/media/...
> content/renderer/media/media_stream_video_track.cc:29: typedef void*
> VideoSinkId;
> aside: is void* truly necessary, or can we have some more specific type?!
> 
>
https://codereview.chromium.org/976233002/diff/260001/content/renderer/media/...
> File content/renderer/media/media_stream_video_track.h (right):
> 
>
https://codereview.chromium.org/976233002/diff/260001/content/renderer/media/...
> content/renderer/media/media_stream_video_track.h:81: base::ThreadChecker
> render_thread_checker_;
> I'm confused: which thread_checker_ was previously referred to prior to this
CL?
> There is also a protected thread_checker_ in MediaStreamTrack.
> 
>
https://codereview.chromium.org/976233002/diff/260001/content/renderer/media/...
> File content/renderer/media/rtc_video_renderer.h (right):
> 
>
https://codereview.chromium.org/976233002/diff/260001/content/renderer/media/...
> content/renderer/media/rtc_video_renderer.h:25: // order to render video
frames
> provided from a MediaStreamVideoTrack, to  which
> nit: s/  / /

restating for proper l*tm detection by rietveld: "rs lgtm % nits I really think
you need an RTC OWNER to CR at least some of these changes (like thread
membership and comments clarifications)."

[mcasas, 2015-03-12 16:31:54]

Patchset #2 (id:280001) has been deleted

[mcasas, 2015-03-13 01:09:34]

tommi@, PTAL at WebRTC related parts (forwarded by wolenetz@)

https://codereview.chromium.org/976233002/diff/260001/content/public/renderer...
File content/public/renderer/media_stream_video_sink.h (right):

https://codereview.chromium.org/976233002/diff/260001/content/public/renderer...
content/public/renderer/media_stream_video_sink.h:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
On 2015/03/11 23:37:37, wolenetz wrote:
> Currently, no OWNER of this file is attached to this CR.

I know. I'm using Chromite Butler and is marked in red.

[1]
https://chrome.google.com/webstore/detail/chromite-butler/bhcnanendmgjjeghama...

https://codereview.chromium.org/976233002/diff/260001/content/renderer/media/...
File content/renderer/media/media_stream_video_capture_source_unittest.cc
(right):

https://codereview.chromium.org/976233002/diff/260001/content/renderer/media/...
content/renderer/media/media_stream_video_capture_source_unittest.cc:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
On 2015/03/11 23:37:37, wolenetz wrote:
> aside: why is this file called *_capture_* vs *_capturer_*?

My guess is that "capture" relates to all activities
related to video capture while "capturer" refers to 
the actual representation of a video capturer device.
Or it might just have been inherited from WebRTC code
base ;)

https://codereview.chromium.org/976233002/diff/260001/content/renderer/media/...
File content/renderer/media/media_stream_video_capturer_source.cc (right):

https://codereview.chromium.org/976233002/diff/260001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:27: {320, 180}};
On 2015/03/10 01:40:06, emircan wrote:
> I would prefer the earlier implementation as the struct is defined with a
type.
> In case params need to be passed into a function or declared, type would be
> required.

Totally agree, but until such circumstance arises,
an anonymous struct is better.

https://codereview.chromium.org/976233002/diff/260001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:207: for (const
auto& frame_rate : kVideoFrameRates) {
On 2015/03/10 01:40:06, emircan wrote:
> frame_rate is an int so copying by value would be faster than keeping a
pointer
> and dereferencing.

Done.

https://codereview.chromium.org/976233002/diff/260001/content/renderer/media/...
File content/renderer/media/media_stream_video_track.cc (right):

https://codereview.chromium.org/976233002/diff/260001/content/renderer/media/...
content/renderer/media/media_stream_video_track.cc:29: typedef void*
VideoSinkId;
On 2015/03/11 23:37:37, wolenetz wrote:
> aside: is void* truly necessary, or can we have some more specific type?!

Good suggestion, it can be MediaStreamVideoSink*
so I adjusted the typedef - I think is still good
to have an alias so it's clear that is a pointer
used as an id.

https://codereview.chromium.org/976233002/diff/260001/content/renderer/media/...
File content/renderer/media/media_stream_video_track.h (right):

https://codereview.chromium.org/976233002/diff/260001/content/renderer/media/...
content/renderer/media/media_stream_video_track.h:81: base::ThreadChecker
render_thread_checker_;
On 2015/03/11 23:37:37, wolenetz wrote:
> I'm confused: which thread_checker_ was previously referred to prior to this
CL?
> There is also a protected thread_checker_ in MediaStreamTrack.

I consolidated the ThreadCheckers in the different classes
of the hierarchy. It seems that this class' |thread_checker_|
was actually hiding the inherited one, and the compiler did
not show any warning about it. Weird.

https://codereview.chromium.org/976233002/diff/260001/content/renderer/media/...
File content/renderer/media/rtc_video_renderer.h (right):

https://codereview.chromium.org/976233002/diff/260001/content/renderer/media/...
content/renderer/media/rtc_video_renderer.h:25: // order to render video frames
provided from a MediaStreamVideoTrack, to  which
On 2015/03/11 23:37:37, wolenetz wrote:
> nit: s/  / /

Done.

[mcasas, 2015-03-13 01:10:38]

mcasas@chromium.org changed reviewers:
+ avi@chromium.org, tommi@chromium.org

[mcasas, 2015-03-13 01:10:39]

tommi@: PTAL at WebRTC related parts (forwarded by wolenetz@).

avi@: content/public/renderer/media_stream_video_sink.h

[tommi (sloooow) - chröme, 2015-03-13 22:04:46]

lgtm

https://codereview.chromium.org/976233002/diff/300001/content/renderer/media/...
File content/renderer/media/media_stream_video_capturer_source.cc (right):

https://codereview.chromium.org/976233002/diff/300001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:21: }
kVideoResolutions[] = {{1920, 1080},
instead of applying the 'const' to the struct, do you mind applying it to the
variable?  As is, it's not clear from the variable declaration - and the
variable might actually not be const now that I think about it... or...

https://codereview.chromium.org/976233002/diff/300001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:130:
media::BindToCurrentLoop(base::Bind(&VideoCapturerDelegate::OnStateUpdate,
this looks kind of weird in the diff at least

https://codereview.chromium.org/976233002/diff/300001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:210:
media::PIXEL_FORMAT_I420));
indent

[mcasas, 2015-03-13 23:03:40]

Patchset #3 (id:320001) has been deleted

[mcasas, 2015-03-14 03:37:54]

The CQ bit was checked by mcasas@chromium.org

[mcasas, 2015-03-14 03:37:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
emircan@chromium.org, wolenetz@chromium.org, tommi@chromium.org
Link to the patchset: https://codereview.chromium.org/976233002/#ps340001
(title: "tommi@ comments")

[commit-bot: I haz the power, 2015-03-14 03:37:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/976233002/340001

[mcasas, 2015-03-14 03:38:49]

https://codereview.chromium.org/976233002/diff/300001/content/renderer/media/...
File content/renderer/media/media_stream_video_capturer_source.cc (right):

https://codereview.chromium.org/976233002/diff/300001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:21: }
kVideoResolutions[] = {{1920, 1080},
On 2015/03/13 22:04:46, tommi wrote:
> instead of applying the 'const' to the struct, do you mind applying it to the
> variable?  As is, it's not clear from the variable declaration - and the
> variable might actually not be const now that I think about it... or...

The classical constfusion!
Done.

https://codereview.chromium.org/976233002/diff/300001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:130:
media::BindToCurrentLoop(base::Bind(&VideoCapturerDelegate::OnStateUpdate,
On 2015/03/13 22:04:46, tommi wrote:
> this looks kind of weird in the diff at least

Done.

https://codereview.chromium.org/976233002/diff/300001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:210:
media::PIXEL_FORMAT_I420));
On 2015/03/13 22:04:46, tommi wrote:
> indent

Done, I hope.

[commit-bot: I haz the power, 2015-03-14 04:18:34]

Committed patchset #3 (id:340001)

[commit-bot: I haz the power, 2015-03-14 04:19:18]

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