VideoTrackAdapter: Add passing frames monitor, notify MSVCS -> MSVTrack(s)

VideoTrackAdapter: Add passing frames monitor, notify MSVCS -> MSVTrack(s)

MediaStreamVideoSource owns a VideoTrackAdapter, that sees the
frames passing by (possibly adapting frame rate and resolution).
This CL extends this VTA to monitor passing frames. Every time the
monitoring wakes up, it notifies the MediaStreamVideoSource of
the muted state via SetMutedState. This Class has no state, simply
updates all registered MediaStreamVideoTracks, who have a 
muted state.

Later CLs will further connect the MSVT::SetMutedState() to
ping WebMediaStreamTrack etc.

BUG=389159
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283285

[mcasas, 2014-07-08 09:17:23]

grunell@ PTAL.

[mcasas, 2014-07-08 09:22:09]

The CQ bit was checked by mcasas@chromium.org

[mcasas, 2014-07-08 09:22:10]

The CQ bit was unchecked by mcasas@chromium.org

[Henrik Grunell, 2014-07-08 11:39:54]

https://codereview.chromium.org/366243003/diff/20001/content/renderer/media/m...
File content/renderer/media/media_stream_track.h (right):

https://codereview.chromium.org/366243003/diff/20001/content/renderer/media/m...
content/renderer/media/media_stream_track.h:50: // The source can put this
attribute to true if it's not delivering frames.
I think it should say "This will be set to true if the source is not delivering
frames." Or which source class are you referring to? Maybe that should be in the
comment?

https://codereview.chromium.org/366243003/diff/20001/content/renderer/media/m...
File content/renderer/media/media_stream_video_source.cc (right):

https://codereview.chromium.org/366243003/diff/20001/content/renderer/media/m...
content/renderer/media/media_stream_video_source.cc:558: // QUESTION(mcasas): is
Unretained OK here?
Does this MediaStreamVideoSource object outlive the thread
MediaStreamVideoSource::SetMutedState is called on? If so, yes.

Does this MediaStreamVideoSource object outlive |track_adapter_|? If so, yes.

Otherwise, afaict, no.

https://codereview.chromium.org/366243003/diff/20001/content/renderer/media/v...
File content/renderer/media/video_track_adapter.cc (right):

https://codereview.chromium.org/366243003/diff/20001/content/renderer/media/v...
content/renderer/media/video_track_adapter.cc:20: static const float
kFirstFrameTimeoutInFrameIntervals = 100.0f;
Remove static.
Comment on these.

https://codereview.chromium.org/366243003/diff/20001/content/renderer/media/v...
content/renderer/media/video_track_adapter.cc:333: io_message_loop_->PostTask(
Could the monitoring possibly be racy initially? Should the monitoring task be
posted before adding the track?

https://codereview.chromium.org/366243003/diff/20001/content/renderer/media/v...
content/renderer/media/video_track_adapter.cc:379: frame_rate_ =
target_frame_rate;
Should frame_rate_ be called target_frame_rate_?

https://codereview.chromium.org/366243003/diff/20001/content/renderer/media/v...
content/renderer/media/video_track_adapter.cc:380: DCHECK_NE(frame_rate_, 0.0f);
Put this dcheck before setting |frame_rate_| and check |target_frame_rate|
instead.

https://codereview.chromium.org/366243003/diff/20001/content/renderer/media/v...
content/renderer/media/video_track_adapter.cc:417: uint64
frame_counter_snapshot) {
I think the "snapshot" part of the name is somewhat unclear. A comment on this
function in the header file explaining the (trivial) algorithm would be good.
(And maybe a change of the name.)

https://codereview.chromium.org/366243003/diff/20001/content/renderer/media/v...
File content/renderer/media/video_track_adapter.h (right):

https://codereview.chromium.org/366243003/diff/20001/content/renderer/media/v...
content/renderer/media/video_track_adapter.h:38: // be released on the main
render thread.
Comment on the new arguments.

https://codereview.chromium.org/366243003/diff/20001/content/renderer/media/v...
content/renderer/media/video_track_adapter.h:75: const OnMutedCallback&
set_muted_state_callback,
I think the name |set_muted_state_callback| should be say "on_muted_callback",
since "set" would make assumptions on what the observer does.

https://codereview.chromium.org/366243003/diff/20001/content/renderer/media/v...
content/renderer/media/video_track_adapter.h:78: void
CheckFramesReceivedOnIO(const OnMutedCallback& set_muted_state_callback,
Comment on what this does.

https://codereview.chromium.org/366243003/diff/20001/content/renderer/media/v...
content/renderer/media/video_track_adapter.h:98: uint64 frame_counter_;
Add comments on all new member variables. In particular which thread they are
accessed on.

[mcasas, 2014-07-08 16:12:01]

grunell@ PTAL

https://codereview.chromium.org/366243003/diff/20001/content/renderer/media/m...
File content/renderer/media/media_stream_track.h (right):

https://codereview.chromium.org/366243003/diff/20001/content/renderer/media/m...
content/renderer/media/media_stream_track.h:50: // The source can put this
attribute to true if it's not delivering frames.
On 2014/07/08 11:39:53, Henrik Grunell wrote:
> I think it should say "This will be set to true if the source is not
delivering
> frames." Or which source class are you referring to? Maybe that should be in
the
> comment?

source -> MediaStreamSource.
Rewritten.

https://codereview.chromium.org/366243003/diff/20001/content/renderer/media/m...
File content/renderer/media/media_stream_video_source.cc (right):

https://codereview.chromium.org/366243003/diff/20001/content/renderer/media/m...
content/renderer/media/media_stream_video_source.cc:558: // QUESTION(mcasas): is
Unretained OK here?
On 2014/07/08 11:39:53, Henrik Grunell wrote:
> Does this MediaStreamVideoSource object outlive the thread
> MediaStreamVideoSource::SetMutedState is called on? If so, yes.
> 
> Does this MediaStreamVideoSource object outlive |track_adapter_|? If so, yes.
> 
> Otherwise, afaict, no.

MSVS has a |weak_ptr_factory_|, never mind this.

https://codereview.chromium.org/366243003/diff/20001/content/renderer/media/v...
File content/renderer/media/video_track_adapter.cc (right):

https://codereview.chromium.org/366243003/diff/20001/content/renderer/media/v...
content/renderer/media/video_track_adapter.cc:20: static const float
kFirstFrameTimeoutInFrameIntervals = 100.0f;
On 2014/07/08 11:39:53, Henrik Grunell wrote:
> Remove static.
> Comment on these.

Done.

https://codereview.chromium.org/366243003/diff/20001/content/renderer/media/v...
content/renderer/media/video_track_adapter.cc:333: io_message_loop_->PostTask(
On 2014/07/08 11:39:53, Henrik Grunell wrote:
> Could the monitoring possibly be racy initially? Should the monitoring task be
> posted before adding the track?

The posting could be racy with respect to the frame 
action initialisation, however that would just mark the
source as "muted", which is anyway what we'd like to see.
If the source starts producing frames, then eventually
|muted==false| and everything would be fine. QED, the
race would be handled fine in this case.

https://codereview.chromium.org/366243003/diff/20001/content/renderer/media/v...
content/renderer/media/video_track_adapter.cc:379: frame_rate_ =
target_frame_rate;
On 2014/07/08 11:39:53, Henrik Grunell wrote:
> Should frame_rate_ be called target_frame_rate_?

Done.

https://codereview.chromium.org/366243003/diff/20001/content/renderer/media/v...
content/renderer/media/video_track_adapter.cc:380: DCHECK_NE(frame_rate_, 0.0f);
On 2014/07/08 11:39:53, Henrik Grunell wrote:
> Put this dcheck before setting |frame_rate_| and check |target_frame_rate|
> instead.

Done.

https://codereview.chromium.org/366243003/diff/20001/content/renderer/media/v...
content/renderer/media/video_track_adapter.cc:417: uint64
frame_counter_snapshot) {
On 2014/07/08 11:39:53, Henrik Grunell wrote:
> I think the "snapshot" part of the name is somewhat unclear. A comment on this
> function in the header file explaining the (trivial) algorithm would be good.
> (And maybe a change of the name.)

Done.

https://codereview.chromium.org/366243003/diff/20001/content/renderer/media/v...
File content/renderer/media/video_track_adapter.h (right):

https://codereview.chromium.org/366243003/diff/20001/content/renderer/media/v...
content/renderer/media/video_track_adapter.h:38: // be released on the main
render thread.
On 2014/07/08 11:39:53, Henrik Grunell wrote:
> Comment on the new arguments.

Done.

https://codereview.chromium.org/366243003/diff/20001/content/renderer/media/v...
content/renderer/media/video_track_adapter.h:75: const OnMutedCallback&
set_muted_state_callback,
On 2014/07/08 11:39:53, Henrik Grunell wrote:
> I think the name |set_muted_state_callback| should be say "on_muted_callback",
> since "set" would make assumptions on what the observer does.

Done.

https://codereview.chromium.org/366243003/diff/20001/content/renderer/media/v...
content/renderer/media/video_track_adapter.h:78: void
CheckFramesReceivedOnIO(const OnMutedCallback& set_muted_state_callback,
On 2014/07/08 11:39:53, Henrik Grunell wrote:
> Comment on what this does.

Done.

https://codereview.chromium.org/366243003/diff/20001/content/renderer/media/v...
content/renderer/media/video_track_adapter.h:98: uint64 frame_counter_;
On 2014/07/08 11:39:53, Henrik Grunell wrote:
> Add comments on all new member variables. In particular which thread they are
> accessed on.

Done.

[Henrik Grunell, 2014-07-09 08:55:53]

https://codereview.chromium.org/366243003/diff/20001/content/renderer/media/m...
File content/renderer/media/media_stream_video_source.cc (right):

https://codereview.chromium.org/366243003/diff/20001/content/renderer/media/m...
content/renderer/media/media_stream_video_source.cc:558: // QUESTION(mcasas): is
Unretained OK here?
On 2014/07/08 16:12:01, mcasas wrote:
> On 2014/07/08 11:39:53, Henrik Grunell wrote:
> > Does this MediaStreamVideoSource object outlive the thread
> > MediaStreamVideoSource::SetMutedState is called on? If so, yes.
> > 
> > Does this MediaStreamVideoSource object outlive |track_adapter_|? If so,
yes.
> > 
> > Otherwise, afaict, no.
> 
> MSVS has a |weak_ptr_factory_|, never mind this.

How/where is a weak ptr used to ensure we don't callback to a deleted object?

https://codereview.chromium.org/366243003/diff/20001/content/renderer/media/v...
File content/renderer/media/video_track_adapter.h (right):

https://codereview.chromium.org/366243003/diff/20001/content/renderer/media/v...
content/renderer/media/video_track_adapter.h:75: const OnMutedCallback&
set_muted_state_callback,
On 2014/07/08 16:12:01, mcasas wrote:
> On 2014/07/08 11:39:53, Henrik Grunell wrote:
> > I think the name |set_muted_state_callback| should be say
"on_muted_callback",
> > since "set" would make assumptions on what the observer does.
> 
> Done.

Nit: I'd prefer "on_muted_callback". (Without "state".)

https://codereview.chromium.org/366243003/diff/40001/content/renderer/media/v...
File content/renderer/media/video_track_adapter.cc (right):

https://codereview.chromium.org/366243003/diff/40001/content/renderer/media/v...
content/renderer/media/video_track_adapter.cc:21: // the first frame and on
normal conditions, respectively.
Nit: "under normal conditions"

Why a larger number the first frame? Explain in comment.

https://codereview.chromium.org/366243003/diff/40001/content/renderer/media/v...
File content/renderer/media/video_track_adapter.h (right):

https://codereview.chromium.org/366243003/diff/40001/content/renderer/media/v...
content/renderer/media/video_track_adapter.h:39: // |target_frame_rate| is used
to calculate a prudent interval to check for
I think the naming of |target_frame_rate| is not clear, at least not to me. :)
I'd interpret "target" as something this class would (try to) adapt to, but it's
actually a monitor interval. Would "monitor_interval_frame_rate" be a correct
description?

https://codereview.chromium.org/366243003/diff/40001/content/renderer/media/v...
content/renderer/media/video_track_adapter.h:40: // passing frames and inform of
the result via |on_muted_state_callback|.
It only informs via the callback if we consider it muted, right? Clarify that.

https://codereview.chromium.org/366243003/diff/40001/content/renderer/media/v...
content/renderer/media/video_track_adapter.h:81: // inform of the situation via
|set_muted_state_callback|.
Same as above. Clarify only if considered muted.

https://codereview.chromium.org/366243003/diff/40001/content/renderer/media/v...
content/renderer/media/video_track_adapter.h:102: // Running frame_counter,
accesed on IO-thread.
"Running frame counter, accessed on the IO-thread."

https://codereview.chromium.org/366243003/diff/40001/content/renderer/media/v...
content/renderer/media/video_track_adapter.h:104: // Frame rate configured on
the source, accesed on IO-thread.
Nit: add empty line before.

https://codereview.chromium.org/366243003/diff/40001/content/renderer/media/v...
content/renderer/media/video_track_adapter.h:104: // Frame rate configured on
the source, accesed on IO-thread.
I don't understand the "configured on the source" part.
Nit: "accessed on the"

https://codereview.chromium.org/366243003/diff/40001/content/renderer/media/v...
content/renderer/media/video_track_adapter.h:105: float target_frame_rate_;
Same as above, for me the "target" part of the name is unclear.

[mcasas, 2014-07-09 12:39:36]

grunell@ PTAL.

https://codereview.chromium.org/366243003/diff/20001/content/renderer/media/m...
File content/renderer/media/media_stream_video_source.cc (right):

https://codereview.chromium.org/366243003/diff/20001/content/renderer/media/m...
content/renderer/media/media_stream_video_source.cc:558: // QUESTION(mcasas): is
Unretained OK here?
On 2014/07/09 08:55:52, Henrik Grunell wrote:
> On 2014/07/08 16:12:01, mcasas wrote:
> > On 2014/07/08 11:39:53, Henrik Grunell wrote:
> > > Does this MediaStreamVideoSource object outlive the thread
> > > MediaStreamVideoSource::SetMutedState is called on? If so, yes.
> > > 
> > > Does this MediaStreamVideoSource object outlive |track_adapter_|? If so,
> yes.
> > > 
> > > Otherwise, afaict, no.
> > 
> > MSVS has a |weak_ptr_factory_|, never mind this.
> 
> How/where is a weak ptr used to ensure we don't callback to a deleted object?

"You can use a base::WeakPtr and base::WeakPtrFactory (in
base/memory/weak_ptr.h) to ensure that any invokes can not outlive the object
they are being invoked on, without using reference counting. The base::Bind
mechanism has special understanding for base::WeakPtr that will disable the
task's execution if the base::WeakPtr has been invalidated. " [1]

[1]
http://www.chromium.org/developers/design-documents/threading#base_WeakPtr_an...

https://codereview.chromium.org/366243003/diff/20001/content/renderer/media/v...
File content/renderer/media/video_track_adapter.h (right):

https://codereview.chromium.org/366243003/diff/20001/content/renderer/media/v...
content/renderer/media/video_track_adapter.h:75: const OnMutedCallback&
set_muted_state_callback,
On 2014/07/09 08:55:52, Henrik Grunell wrote:
> On 2014/07/08 16:12:01, mcasas wrote:
> > On 2014/07/08 11:39:53, Henrik Grunell wrote:
> > > I think the name |set_muted_state_callback| should be say
> "on_muted_callback",
> > > since "set" would make assumptions on what the observer does.
> > 
> > Done.
> 
> Nit: I'd prefer "on_muted_callback". (Without "state".)

I just wanted to make explicit that muted is an state,
as oppose to some kind of control.

https://codereview.chromium.org/366243003/diff/40001/content/renderer/media/v...
File content/renderer/media/video_track_adapter.cc (right):

https://codereview.chromium.org/366243003/diff/40001/content/renderer/media/v...
content/renderer/media/video_track_adapter.cc:21: // the first frame and on
normal conditions, respectively.
On 2014/07/09 08:55:52, Henrik Grunell wrote:
> Nit: "under normal conditions"
> 
> Why a larger number the first frame? Explain in comment.

Done.

https://codereview.chromium.org/366243003/diff/40001/content/renderer/media/v...
File content/renderer/media/video_track_adapter.h (right):

https://codereview.chromium.org/366243003/diff/40001/content/renderer/media/v...
content/renderer/media/video_track_adapter.h:40: // passing frames and inform of
the result via |on_muted_state_callback|.
On 2014/07/09 08:55:52, Henrik Grunell wrote:
> It only informs via the callback if we consider it muted, right? Clarify that.

It pings back every time it wakes up, with the 
appropriate state every time.

https://codereview.chromium.org/366243003/diff/40001/content/renderer/media/v...
content/renderer/media/video_track_adapter.h:81: // inform of the situation via
|set_muted_state_callback|.
On 2014/07/09 08:55:53, Henrik Grunell wrote:
> Same as above. Clarify only if considered muted.

Done.

https://codereview.chromium.org/366243003/diff/40001/content/renderer/media/v...
content/renderer/media/video_track_adapter.h:102: // Running frame_counter,
accesed on IO-thread.
On 2014/07/09 08:55:52, Henrik Grunell wrote:
> "Running frame counter, accessed on the IO-thread."

Done.

https://codereview.chromium.org/366243003/diff/40001/content/renderer/media/v...
content/renderer/media/video_track_adapter.h:104: // Frame rate configured on
the source, accesed on IO-thread.
On 2014/07/09 08:55:52, Henrik Grunell wrote:
> Nit: add empty line before.

Done.

https://codereview.chromium.org/366243003/diff/40001/content/renderer/media/v...
content/renderer/media/video_track_adapter.h:104: // Frame rate configured on
the source, accesed on IO-thread.
On 2014/07/09 08:55:52, Henrik Grunell wrote:
> Nit: add empty line before.

Done.

https://codereview.chromium.org/366243003/diff/40001/content/renderer/media/v...
content/renderer/media/video_track_adapter.h:105: float target_frame_rate_;
On 2014/07/09 08:55:53, Henrik Grunell wrote:
> Same as above, for me the "target" part of the name is unclear.

See above.

[Henrik Grunell, 2014-07-09 13:01:23]

https://codereview.chromium.org/366243003/diff/20001/content/renderer/media/m...
File content/renderer/media/media_stream_video_source.cc (right):

https://codereview.chromium.org/366243003/diff/20001/content/renderer/media/m...
content/renderer/media/media_stream_video_source.cc:558: // QUESTION(mcasas): is
Unretained OK here?
On 2014/07/09 12:39:35, mcasas wrote:
> On 2014/07/09 08:55:52, Henrik Grunell wrote:
> > On 2014/07/08 16:12:01, mcasas wrote:
> > > On 2014/07/08 11:39:53, Henrik Grunell wrote:
> > > > Does this MediaStreamVideoSource object outlive the thread
> > > > MediaStreamVideoSource::SetMutedState is called on? If so, yes.
> > > > 
> > > > Does this MediaStreamVideoSource object outlive |track_adapter_|? If so,
> > yes.
> > > > 
> > > > Otherwise, afaict, no.
> > > 
> > > MSVS has a |weak_ptr_factory_|, never mind this.
> > 
> > How/where is a weak ptr used to ensure we don't callback to a deleted
object?
> 
> "You can use a base::WeakPtr and base::WeakPtrFactory (in
> base/memory/weak_ptr.h) to ensure that any invokes can not outlive the object
> they are being invoked on, without using reference counting. The base::Bind
> mechanism has special understanding for base::WeakPtr that will disable the
> task's execution if the base::WeakPtr has been invalidated. " [1]
> 
> [1]
>
http://www.chromium.org/developers/design-documents/threading#base_WeakPtr_an...

Yes. But you need to change base::Unretained(this) to
weak_ptr_factory_.GetWeakPtr(). (Sorry if I wasn't explicit before.) I saw you
made that change now.

https://codereview.chromium.org/366243003/diff/20001/content/renderer/media/v...
File content/renderer/media/video_track_adapter.h (right):

https://codereview.chromium.org/366243003/diff/20001/content/renderer/media/v...
content/renderer/media/video_track_adapter.h:75: const OnMutedCallback&
set_muted_state_callback,
On 2014/07/09 12:39:35, mcasas wrote:
> On 2014/07/09 08:55:52, Henrik Grunell wrote:
> > On 2014/07/08 16:12:01, mcasas wrote:
> > > On 2014/07/08 11:39:53, Henrik Grunell wrote:
> > > > I think the name |set_muted_state_callback| should be say
> > "on_muted_callback",
> > > > since "set" would make assumptions on what the observer does.
> > > 
> > > Done.
> > 
> > Nit: I'd prefer "on_muted_callback". (Without "state".)
> 
> I just wanted to make explicit that muted is an state,
> as oppose to some kind of control.

OK, I'm fine with that.

https://codereview.chromium.org/366243003/diff/40001/content/renderer/media/v...
File content/renderer/media/video_track_adapter.h (right):

https://codereview.chromium.org/366243003/diff/40001/content/renderer/media/v...
content/renderer/media/video_track_adapter.h:39: // |target_frame_rate| is used
to calculate a prudent interval to check for
On 2014/07/09 08:55:53, Henrik Grunell wrote:
> I think the naming of |target_frame_rate| is not clear, at least not to me. :)
> I'd interpret "target" as something this class would (try to) adapt to, but
it's
> actually a monitor interval. Would "monitor_interval_frame_rate" be a correct
> description?

I guess you missed a reply on this, |source_frame_rate| is better, thanks.

https://codereview.chromium.org/366243003/diff/40001/content/renderer/media/v...
content/renderer/media/video_track_adapter.h:40: // passing frames and inform of
the result via |on_muted_state_callback|.
On 2014/07/09 12:39:35, mcasas wrote:
> On 2014/07/09 08:55:52, Henrik Grunell wrote:
> > It only informs via the callback if we consider it muted, right? Clarify
that.
> 
> It pings back every time it wakes up, with the 
> appropriate state every time.

Oh, right.

https://codereview.chromium.org/366243003/diff/60001/content/renderer/media/v...
File content/renderer/media/video_track_adapter.cc (right):

https://codereview.chromium.org/366243003/diff/60001/content/renderer/media/v...
content/renderer/media/video_track_adapter.cc:418: void
VideoTrackAdapter::CheckFramesReceivedOnIO(
It seems like this will go on forever, even after a track has been removed? The
monitoring should stop then.

https://codereview.chromium.org/366243003/diff/60001/content/renderer/media/v...
File content/renderer/media/video_track_adapter.h (right):

https://codereview.chromium.org/366243003/diff/60001/content/renderer/media/v...
content/renderer/media/video_track_adapter.h:102: // Running frame counter,
accesed on the IO-thread.
Nit: "accessed" (2 "s"). Same below.

[Henrik Grunell, 2014-07-09 14:00:43]

https://codereview.chromium.org/366243003/diff/60001/content/renderer/media/v...
File content/renderer/media/video_track_adapter.cc (right):

https://codereview.chromium.org/366243003/diff/60001/content/renderer/media/v...
content/renderer/media/video_track_adapter.cc:336: io_message_loop_->PostTask(
Looks like we start a new monitor loop every time a track is added, should only
start once.

Also stop monitoring when there are no more tracks connected to the source.

[mcasas, 2014-07-10 08:16:55]

grunell@ PTAL.

https://codereview.chromium.org/366243003/diff/60001/content/renderer/media/v...
File content/renderer/media/video_track_adapter.cc (right):

https://codereview.chromium.org/366243003/diff/60001/content/renderer/media/v...
content/renderer/media/video_track_adapter.cc:336: io_message_loop_->PostTask(
On 2014/07/09 14:00:42, Henrik Grunell wrote:
> Looks like we start a new monitor loop every time a track is added, should
only
> start once.
> 

Added. I do that via checking |adapters_.size()| on 
IO thread. Note the inversion in the PostTasks, first
I trigger the StartTrackMonitoring() and then the 
AddTrackOnIO(), and they will be scheduled in that
order.

> Also stop monitoring when there are no more tracks connected to the source.

In that case the source would go down anyway, since 
it cannot survive with no tracks connected to it, and
this will invalidate the weak ptr and stop the monitoring
anyway.

https://codereview.chromium.org/366243003/diff/60001/content/renderer/media/v...
content/renderer/media/video_track_adapter.cc:418: void
VideoTrackAdapter::CheckFramesReceivedOnIO(
On 2014/07/09 13:01:22, Henrik Grunell wrote:
> It seems like this will go on forever, even after a track has been removed?
The
> monitoring should stop then.

Closed as per comments above and offline discussion.

Note that there is 1 VTA per MSVSource. When all tracks
are removed, the source will go down anyway.

https://codereview.chromium.org/366243003/diff/60001/content/renderer/media/v...
File content/renderer/media/video_track_adapter.h (right):

https://codereview.chromium.org/366243003/diff/60001/content/renderer/media/v...
content/renderer/media/video_track_adapter.h:102: // Running frame counter,
accesed on the IO-thread.
On 2014/07/09 13:01:22, Henrik Grunell wrote:
> Nit: "accessed" (2 "s"). Same below.

Done.

[Henrik Grunell, 2014-07-10 08:41:35]

https://codereview.chromium.org/366243003/diff/80001/content/renderer/media/m...
File content/renderer/media/media_stream_track.cc (right):

https://codereview.chromium.org/366243003/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_track.cc:39: bool
MediaStreamTrack::GetMutedState(void) const {
Remove.

https://codereview.chromium.org/366243003/diff/80001/content/renderer/media/m...
File content/renderer/media/media_stream_track.h (right):

https://codereview.chromium.org/366243003/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_track.h:38: virtual bool GetMutedState(void)
const;
bool muted_state() const { return muted_state_; }

Or does it need to be virtual?
Does SetMutedState?

https://codereview.chromium.org/366243003/diff/80001/content/renderer/media/m...
File content/renderer/media/media_stream_video_source_unittest.cc (right):

https://codereview.chromium.org/366243003/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_video_source_unittest.cc:794: }
I would recommend to add a test where the state goes to muted then back to not
muted.

https://codereview.chromium.org/366243003/diff/80001/content/renderer/media/m...
File content/renderer/media/media_stream_video_track.cc (right):

https://codereview.chromium.org/366243003/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_video_track.cc:247: bool
MediaStreamVideoTrack::GetMutedState(void) const {
This doesn't need to be overridden, does the same as the base.

https://codereview.chromium.org/366243003/diff/80001/content/renderer/media/v...
File content/renderer/media/video_track_adapter.cc (right):

https://codereview.chromium.org/366243003/diff/80001/content/renderer/media/v...
content/renderer/media/video_track_adapter.cc:385: if (adapters_.size())
if (adapters_.size() > 0)

https://codereview.chromium.org/366243003/diff/80001/content/renderer/media/v...
content/renderer/media/video_track_adapter.cc:431:
io_message_loop_->PostDelayedTask(FROM_HERE,
Add a comment about that we'll keep monitoring until this object goes away,
since it does that when all tracks has been removed.

[mcasas, 2014-07-10 11:59:04]

grunell@ PTAL

https://codereview.chromium.org/366243003/diff/80001/content/renderer/media/m...
File content/renderer/media/media_stream_track.h (right):

https://codereview.chromium.org/366243003/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_track.h:38: virtual bool GetMutedState(void)
const;
On 2014/07/10 08:41:34, Henrik Grunell wrote:
> bool muted_state() const { return muted_state_; }
> 
> Or does it need to be virtual?
> Does SetMutedState?

Discussed offline, but just to get it logged:

MSTrack is a parent class of MSVideoTrack and
MSAudioTrack (actually WebRtcLocalAudioTrack).
WebRtcLocalAudioTrack knows not of |muted_state|
hence the local implementation here.

As per W3C doco, all tracks should implement 
the muted event, so there's a default 
implementation here, just not using the 
standard mutator and accessor naming because
the deriving classes might -and actually do- 
more stuff, in MSVT case, checks the thread.

https://codereview.chromium.org/366243003/diff/80001/content/renderer/media/m...
File content/renderer/media/media_stream_video_source_unittest.cc (right):

https://codereview.chromium.org/366243003/diff/80001/content/renderer/media/m...
content/renderer/media/media_stream_video_source_unittest.cc:794: }
On 2014/07/10 08:41:34, Henrik Grunell wrote:
> I would recommend to add a test where the state goes to muted then back to not
> muted.

Extended "MutedSource" to do that.

https://codereview.chromium.org/366243003/diff/80001/content/renderer/media/v...
File content/renderer/media/video_track_adapter.cc (right):

https://codereview.chromium.org/366243003/diff/80001/content/renderer/media/v...
content/renderer/media/video_track_adapter.cc:385: if (adapters_.size())
On 2014/07/10 08:41:34, Henrik Grunell wrote:
> if (adapters_.size() > 0)

Done.

https://codereview.chromium.org/366243003/diff/80001/content/renderer/media/v...
content/renderer/media/video_track_adapter.cc:431:
io_message_loop_->PostDelayedTask(FROM_HERE,
On 2014/07/10 08:41:34, Henrik Grunell wrote:
> Add a comment about that we'll keep monitoring until this object goes away,
> since it does that when all tracks has been removed.

Done.

[Henrik Grunell, 2014-07-10 13:49:30]

LGTM

https://codereview.chromium.org/366243003/diff/100001/content/renderer/media/...
File content/renderer/media/video_track_adapter.cc (right):

https://codereview.chromium.org/366243003/diff/100001/content/renderer/media/...
content/renderer/media/video_track_adapter.cc:385: if (adapters_.size() > 0)
Nit: It doesn't happen to have an empty() function? If so, use it.

[mcasas, 2014-07-10 13:51:20]

tommi@ PTAL

[tommi (sloooow) - chröme, 2014-07-14 16:55:58]

lgtm

https://codereview.chromium.org/366243003/diff/100001/content/renderer/media/...
File content/renderer/media/media_stream_track.cc (right):

https://codereview.chromium.org/366243003/diff/100001/content/renderer/media/...
content/renderer/media/media_stream_track.cc:36: muted_state_ = muted_state;
thread check?  In general the class looks like its single threaded, so dchecks
all around would be good.

https://codereview.chromium.org/366243003/diff/100001/content/renderer/media/...
File content/renderer/media/media_stream_video_source.cc (right):

https://codereview.chromium.org/366243003/diff/100001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:589:
DCHECK(CalledOnValidThread());
can you add this check to the other methods too?

https://codereview.chromium.org/366243003/diff/100001/content/renderer/media/...
File content/renderer/media/media_stream_video_source.h (right):

https://codereview.chromium.org/366243003/diff/100001/content/renderer/media/...
content/renderer/media/media_stream_video_source.h:38: // When the first track
is added to the source by calling AddTrack the
AddTrack, the

https://codereview.chromium.org/366243003/diff/100001/content/renderer/media/...
File content/renderer/media/video_track_adapter.cc (right):

https://codereview.chromium.org/366243003/diff/100001/content/renderer/media/...
content/renderer/media/video_track_adapter.cc:390: <<
(kFirstFrameTimeoutInFrameIntervals / source_frame_rate_) << "s";
nit: align <<

[mcasas, 2014-07-15 08:22:59]

The CQ bit was checked by mcasas@chromium.org

[commit-bot: I haz the power, 2014-07-15 08:25:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mcasas@chromium.org/366243003/120001

[mcasas, 2014-07-15 10:54:55]

The CQ bit was checked by mcasas@chromium.org

[commit-bot: I haz the power, 2014-07-15 10:56:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mcasas@chromium.org/366243003/140001

[mcasas, 2014-07-15 17:11:18]

The CQ bit was unchecked by mcasas@chromium.org

[mcasas, 2014-07-15 17:11:25]

The CQ bit was checked by mcasas@chromium.org

[commit-bot: I haz the power, 2014-07-15 17:13:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mcasas@chromium.org/366243003/140001

[commit-bot: I haz the power, 2014-07-16 00:02:08]

Change committed as 283285

[mcasas, 2014-07-18 07:25:06]

Submitting comments, please ignore.

https://codereview.chromium.org/366243003/diff/100001/content/renderer/media/...
File content/renderer/media/media_stream_track.cc (right):

https://codereview.chromium.org/366243003/diff/100001/content/renderer/media/...
content/renderer/media/media_stream_track.cc:36: muted_state_ = muted_state;
On 2014/07/14 16:55:57, tommi wrote:
> thread check?  In general the class looks like its single threaded, so dchecks
> all around would be good.

Done.

https://codereview.chromium.org/366243003/diff/100001/content/renderer/media/...
File content/renderer/media/media_stream_video_source.cc (right):

https://codereview.chromium.org/366243003/diff/100001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:589:
DCHECK(CalledOnValidThread());
On 2014/07/14 16:55:57, tommi wrote:
> can you add this check to the other methods too?

Done.

https://codereview.chromium.org/366243003/diff/100001/content/renderer/media/...
File content/renderer/media/media_stream_video_source.h (right):

https://codereview.chromium.org/366243003/diff/100001/content/renderer/media/...
content/renderer/media/media_stream_video_source.h:38: // When the first track
is added to the source by calling AddTrack the
On 2014/07/14 16:55:57, tommi wrote:
> AddTrack, the

Done.

https://codereview.chromium.org/366243003/diff/100001/content/renderer/media/...
File content/renderer/media/video_track_adapter.cc (right):

https://codereview.chromium.org/366243003/diff/100001/content/renderer/media/...
content/renderer/media/video_track_adapter.cc:385: if (adapters_.size() > 0)
On 2014/07/10 13:49:30, Henrik Grunell OOO back Aug 11 wrote:
> Nit: It doesn't happen to have an empty() function? If so, use it.

Done.

https://codereview.chromium.org/366243003/diff/100001/content/renderer/media/...
content/renderer/media/video_track_adapter.cc:390: <<
(kFirstFrameTimeoutInFrameIntervals / source_frame_rate_) << "s";
On 2014/07/14 16:55:58, tommi wrote:
> nit: align <<

I think it's aligned.