Change MediaStreamVideoSource to output different resolutions to different tracks depending on the …

Change MediaStreamVideoSource to output different resolutions do different tracks depending on the track constraints.

This cl introduce a new class VideoTrackAdapter. The adapter is responsible for forwarding frames on the IO-thread from a MediaStreamVideoSource to a the tracks connected to the source. It has the ability to wrap media::VideoFrames with new visible_rect and natural_size to match max width and height resolutions as well as min and max aspect ratio per track. 
Note that this does not yet work for textures.

It also changes the WebrtcVideoCaptureAdapter to use libyuv::Scale instead of libyuv::I420Copy. This is needed to allow the video frames sent on a PC to be both cropped and scaled.

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

[perkj_chrome, 2014-05-06 13:11:00]

Hej
I think this is ready for a first pass. 

Can you please review?

[perkj_chrome, 2014-05-07 12:32:15]

Miguel, can you please take a look?

[perkj_chrome, 2014-05-07 13:00:32]

And Patrik for the tests and as much code as you want.

[mcasas, 2014-05-07 14:10:43]

Looking fine, just minor comments and suggestions.

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

https://codereview.chromium.org/246433006/diff/60001/content/renderer/media/m...
content/renderer/media/media_stream_video_source.cc:273: if (*min_aspect_ratio
!= 0)
|min_aspect_ratio| is a double, so this comparison is always true.

https://codereview.chromium.org/246433006/diff/60001/content/renderer/media/m...
content/renderer/media/media_stream_video_source.cc:338: new
VideoTrackAdapter(io_message_loop)),
nit: Fits in one line.

https://codereview.chromium.org/246433006/diff/60001/content/renderer/media/m...
content/renderer/media/media_stream_video_source.cc:419: return
track_adapter_->io_message_loop();
I think nobody uses this getter anymore.

https://codereview.chromium.org/246433006/diff/60001/content/renderer/media/m...
content/renderer/media/media_stream_video_source.cc:529:
GetDesiredMaxWidthAndHeight(it->constraints, &max_width, &max_height);
nit: Inside GetDesiredMaxWidthAndHeight(), |max_width| and |max_height| are also
initialised to max() values.

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

https://codereview.chromium.org/246433006/diff/60001/content/renderer/media/v...
content/renderer/media/video_track_adapter.cc:31: // adaptation and delivers
frames to all registered tracks on the IO-thread.
This threading detail should go in the header file.

https://codereview.chromium.org/246433006/diff/60001/content/renderer/media/v...
content/renderer/media/video_track_adapter.cc:32: // All method calls except
creation, must be  on the IO-thread.
nit: double space after "must be"

https://codereview.chromium.org/246433006/diff/60001/content/renderer/media/v...
content/renderer/media/video_track_adapter.cc:79:
std::vector<VideoIdCallbackPair> callbacks_;
std::list? Only used for push_back() and iterator-based search.

https://codereview.chromium.org/246433006/diff/60001/content/renderer/media/v...
content/renderer/media/video_track_adapter.cc:116:
scoped_refptr<media::VideoFrame> video_frame(frame);
Move to l.152?

https://codereview.chromium.org/246433006/diff/60001/content/renderer/media/v...
content/renderer/media/video_track_adapter.cc:121: if
(frame->natural_size().width() > max_frame_size_.width() ||
Perhaps add comment here? Like: "If the input |frame|'s dimensions or aspect
ratio are out of the defined range then..."

https://codereview.chromium.org/246433006/diff/60001/content/renderer/media/v...
content/renderer/media/video_track_adapter.cc:125: int desired_width =
std::min(max_frame_size_.width(),
I'd love to see some human-parseable comment on the micro algorithm used here,
like: Choose output letterbox dimensions such that both the aspect ratio range
and the max dimensions are respected blabla.

https://codereview.chromium.org/246433006/diff/60001/content/renderer/media/v...
content/renderer/media/video_track_adapter.cc:137: if (requested_ratio <
min_aspect_ratio_)
else if? |requested_ratio_| should not be larger than max and smaller than min,
right?

Interesting to add somewhere:
DCHECK((max_aspect_ratio_ - min_aspect_ratio_) > 1E-1); ?

https://codereview.chromium.org/246433006/diff/60001/content/renderer/media/v...
content/renderer/media/video_track_adapter.cc:161: <<
video_frame->natural_size().width()
nit: video_frame->natural_size() has a ToString() method that prints something
like axb.

https://codereview.chromium.org/246433006/diff/60001/content/renderer/media/v...
content/renderer/media/video_track_adapter.cc:176: for
(std::vector<VideoIdCallbackPair>::iterator it = callbacks_.begin();
const_iterator?

https://codereview.chromium.org/246433006/diff/60001/content/renderer/media/v...
content/renderer/media/video_track_adapter.cc:221: void
VideoTrackAdapter::AddTrack(MediaStreamVideoTrack* track,
The naked pointer stands out. Could we at least qualify it |const| ?

https://codereview.chromium.org/246433006/diff/60001/content/renderer/media/v...
content/renderer/media/video_track_adapter.cc:243: for (FrameAdapters::iterator
it = adapters_.begin(); it != adapters_.end();
const_iterator?

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

https://codereview.chromium.org/246433006/diff/60001/content/renderer/media/v...
content/renderer/media/video_track_adapter.h:26: class VideoTrackAdapter
I seem to understand from VideoTrackAdapter structure that it can perform
different adaptations, but these class-docos only talk about
resolution/aspect-ratio/visible-region adjustments. 

IOW if VTA is generic, then mention the current use case as a particular
adaptation, otherwise surface VFRA and skip VTA altogether.

https://codereview.chromium.org/246433006/diff/60001/content/renderer/media/v...
content/renderer/media/video_track_adapter.h:70: typedef
std::vector<scoped_refptr<VideoFrameResolutionAdapter> >
std::list? Seems that you only used it for push_back() and iterator search
anyway.

https://codereview.chromium.org/246433006/diff/60001/content/renderer/media/w...
File content/renderer/media/webrtc/webrtc_video_capturer_adapter_unittest.cc
(right):

https://codereview.chromium.org/246433006/diff/60001/content/renderer/media/w...
content/renderer/media/webrtc/webrtc_video_capturer_adapter_unittest.cc:70: 
Variate these new tests for I420 and YV12?

i.e. add a param to TestSourceCropFrame with the pixel format,
make the Test class derive like

public testing::WithParamInterface<const char*>

Add somewhere the variations' array:

static const media::VideoFrame::Format kTestConfigFlags[] = { 
  media::VideoFrame::I420,
  media::VideoFrame::YV12
};

Inside the test you can get the variation's param with
GetParam(). Then after each test (change their names 
to TEST_P):

INSTANTIATE_TEST_CASE_P(WebRtcVideoCapturerAdapterTests,
                        WebRtcVideoCapturerAdapterTest,
                        testing::ValuesIn(kTestConfigFlags));

https://codereview.chromium.org/246433006/diff/60001/content/renderer/media/w...
content/renderer/media/webrtc/webrtc_video_capturer_adapter_unittest.cc:72: 
Too many empty lines.

https://codereview.chromium.org/246433006/diff/60001/content/test/data/media/...
File content/test/data/media/getusermedia.html (right):

https://codereview.chromium.org/246433006/diff/60001/content/test/data/media/...
content/test/data/media/getusermedia.html:192: function
twoGetUserMedia(constraints1, constraints2) {
s/twoGetUserMedia/getUserMediaTwice/  ? (Also l.193)

[phoglund_chromium, 2014-05-07 14:14:25]

lgtm for content_browsertests

[perkj_chrome, 2014-05-08 11:29:46]

PTAL

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

https://codereview.chromium.org/246433006/diff/60001/content/renderer/media/m...
content/renderer/media/media_stream_video_source.cc:273: if (*min_aspect_ratio
!= 0)
On 2014/05/07 14:10:43, mcasas wrote:
> |min_aspect_ratio| is a double, so this comparison is always true.

Why would it be true? I set it to 0 at line 248?

https://codereview.chromium.org/246433006/diff/60001/content/renderer/media/m...
content/renderer/media/media_stream_video_source.cc:338: new
VideoTrackAdapter(io_message_loop)),
On 2014/05/07 14:10:43, mcasas wrote:
> nit: Fits in one line.

Done.

https://codereview.chromium.org/246433006/diff/60001/content/renderer/media/m...
content/renderer/media/media_stream_video_source.cc:419: return
track_adapter_->io_message_loop();
On 2014/05/07 14:10:43, mcasas wrote:
> I think nobody uses this getter anymore.

no- its used by a few of the classes that inherits from MediaStreamVideoSource.

https://codereview.chromium.org/246433006/diff/60001/content/renderer/media/m...
content/renderer/media/media_stream_video_source.cc:529:
GetDesiredMaxWidthAndHeight(it->constraints, &max_width, &max_height);
On 2014/05/07 14:10:43, mcasas wrote:
> nit: Inside GetDesiredMaxWidthAndHeight(), |max_width| and |max_height| are
also
> initialised to max() values.
done

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

https://codereview.chromium.org/246433006/diff/60001/content/renderer/media/v...
content/renderer/media/video_track_adapter.cc:31: // adaptation and delivers
frames to all registered tracks on the IO-thread.
On 2014/05/07 14:10:43, mcasas wrote:
> This threading detail should go in the header file.

This class only exist as an internal class. So documentation related to this
class should be here. There is also documentation in the header file where I
forward declare this.

https://codereview.chromium.org/246433006/diff/60001/content/renderer/media/v...
content/renderer/media/video_track_adapter.cc:32: // All method calls except
creation, must be  on the IO-thread.
On 2014/05/07 14:10:43, mcasas wrote:
> nit: double space after "must be"

Done.

https://codereview.chromium.org/246433006/diff/60001/content/renderer/media/v...
content/renderer/media/video_track_adapter.cc:79:
std::vector<VideoIdCallbackPair> callbacks_;
On 2014/05/07 14:10:43, mcasas wrote:
> std::list? Only used for push_back() and iterator-based search.

dito.

https://codereview.chromium.org/246433006/diff/60001/content/renderer/media/v...
content/renderer/media/video_track_adapter.cc:116:
scoped_refptr<media::VideoFrame> video_frame(frame);
On 2014/05/07 14:10:43, mcasas wrote:
> Move to l.152?
line 169  DoDeliverFrame(video_frame, format) should be done regardless of the
if statement.

https://codereview.chromium.org/246433006/diff/60001/content/renderer/media/v...
content/renderer/media/video_track_adapter.cc:121: if
(frame->natural_size().width() > max_frame_size_.width() ||
On 2014/05/07 14:10:43, mcasas wrote:
> Perhaps add comment here? Like: "If the input |frame|'s dimensions or aspect
> ratio are out of the defined range then..."

Done.

https://codereview.chromium.org/246433006/diff/60001/content/renderer/media/v...
content/renderer/media/video_track_adapter.cc:125: int desired_width =
std::min(max_frame_size_.width(),
On 2014/05/07 14:10:43, mcasas wrote:
> I'd love to see some human-parseable comment on the micro algorithm used here,
> like: Choose output letterbox dimensions such that both the aspect ratio range
> and the max dimensions are respected blabla.

Done.

https://codereview.chromium.org/246433006/diff/60001/content/renderer/media/v...
content/renderer/media/video_track_adapter.cc:137: if (requested_ratio <
min_aspect_ratio_)
On 2014/05/07 14:10:43, mcasas wrote:
> else if? |requested_ratio_| should not be larger than max and smaller than
min,
> right?
> 
> Interesting to add somewhere:
> DCHECK((max_aspect_ratio_ - min_aspect_ratio_) > 1E-1); ?

done in ctor

https://codereview.chromium.org/246433006/diff/60001/content/renderer/media/v...
content/renderer/media/video_track_adapter.cc:161: <<
video_frame->natural_size().width()
On 2014/05/07 14:10:43, mcasas wrote:
> nit: video_frame->natural_size() has a ToString() method that prints something
> like axb.

Done.

https://codereview.chromium.org/246433006/diff/60001/content/renderer/media/v...
content/renderer/media/video_track_adapter.cc:176: for
(std::vector<VideoIdCallbackPair>::iterator it = callbacks_.begin();
On 2014/05/07 14:10:43, mcasas wrote:
> const_iterator?

Done.

https://codereview.chromium.org/246433006/diff/60001/content/renderer/media/v...
content/renderer/media/video_track_adapter.cc:221: void
VideoTrackAdapter::AddTrack(MediaStreamVideoTrack* track,
On 2014/05/07 14:10:43, mcasas wrote:
> The naked pointer stands out. Could we at least qualify it |const| ?

Done.

https://codereview.chromium.org/246433006/diff/60001/content/renderer/media/v...
content/renderer/media/video_track_adapter.cc:243: for (FrameAdapters::iterator
it = adapters_.begin(); it != adapters_.end();
On 2014/05/07 14:10:43, mcasas wrote:
> const_iterator?

Done.

https://codereview.chromium.org/246433006/diff/60001/content/renderer/media/v...
content/renderer/media/video_track_adapter.cc:243: for (FrameAdapters::iterator
it = adapters_.begin(); it != adapters_.end();
On 2014/05/07 14:10:43, mcasas wrote:
> const_iterator?

Done.

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

https://codereview.chromium.org/246433006/diff/60001/content/renderer/media/v...
content/renderer/media/video_track_adapter.h:26: class VideoTrackAdapter
On 2014/05/07 14:10:43, mcasas wrote:
> I seem to understand from VideoTrackAdapter structure that it can perform
> different adaptations, but these class-docos only talk about
> resolution/aspect-ratio/visible-region adjustments. 
> 
> IOW if VTA is generic, then mention the current use case as a particular
> adaptation, otherwise surface VFRA and skip VTA altogether.

Not sure what you mean. It can only change the resolution.

https://codereview.chromium.org/246433006/diff/60001/content/renderer/media/v...
content/renderer/media/video_track_adapter.h:70: typedef
std::vector<scoped_refptr<VideoFrameResolutionAdapter> >
On 2014/05/07 14:10:43, mcasas wrote:
> std::list? Seems that you only used it for push_back() and iterator search
> anyway.

vector seems to be prefered in Chrome. Its far more widely used than lists.
Performance vice I don't think it matters since there will only be one or very
few items in this list.

https://codereview.chromium.org/246433006/diff/60001/content/renderer/media/w...
File content/renderer/media/webrtc/webrtc_video_capturer_adapter_unittest.cc
(right):

https://codereview.chromium.org/246433006/diff/60001/content/renderer/media/w...
content/renderer/media/webrtc/webrtc_video_capturer_adapter_unittest.cc:70: 
I don't think that is necessary. 

On 2014/05/07 14:10:43, mcasas wrote:
> Variate these new tests for I420 and YV12?
> 
> i.e. add a param to TestSourceCropFrame with the pixel format,
> make the Test class derive like
> 
> public testing::WithParamInterface<const char*>
> 
> Add somewhere the variations' array:
> 
> static const media::VideoFrame::Format kTestConfigFlags[] = { 
>   media::VideoFrame::I420,
>   media::VideoFrame::YV12
> };
> 
> Inside the test you can get the variation's param with
> GetParam(). Then after each test (change their names 
> to TEST_P):
> 
> INSTANTIATE_TEST_CASE_P(WebRtcVideoCapturerAdapterTests,
>                         WebRtcVideoCapturerAdapterTest,
>                         testing::ValuesIn(kTestConfigFlags));

https://codereview.chromium.org/246433006/diff/60001/content/renderer/media/w...
content/renderer/media/webrtc/webrtc_video_capturer_adapter_unittest.cc:72: 
On 2014/05/07 14:10:43, mcasas wrote:
> Too many empty lines.

Done.

https://codereview.chromium.org/246433006/diff/60001/content/test/data/media/...
File content/test/data/media/getusermedia.html (right):

https://codereview.chromium.org/246433006/diff/60001/content/test/data/media/...
content/test/data/media/getusermedia.html:192: function
twoGetUserMedia(constraints1, constraints2) {
On 2014/05/07 14:10:43, mcasas wrote:
> s/twoGetUserMedia/getUserMediaTwice/  ? (Also l.193)

The method call above is called twoGetUserMediaAndStop. So this match that
convention. Are you ok with this or you want me to change both?

[tommi (sloooow) - chröme, 2014-05-08 11:57:57]

first pass - need to go to a meeting will take another look after

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

https://codereview.chromium.org/246433006/diff/120001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:58: bool
GetConstraintValue(const blink::WebMediaConstraints& constraints,
this feels like copy/paste of some other code. Can you use what's already
available? (I think Shijing added some utility functions that do this recently)

https://codereview.chromium.org/246433006/diff/120001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:66:
base::StringToInt(value_str.utf8(), value);
you need to handle this error

https://codereview.chromium.org/246433006/diff/120001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:78:
base::StringToDouble(value_str.utf8(), value);
here too

https://codereview.chromium.org/246433006/diff/120001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:83: bool
HasMandatoryConstraints(const blink::WebMediaConstraints& constraints) {
This feels like an expensive way to check if the mandatory constraints are
there.  Are we ever calling this method first and then fetching the mandatory
constraints again?  Could we do without this method?

https://codereview.chromium.org/246433006/diff/120001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:91: //
std::numeric_limits<int>::max();
also document the behavior if the width and height are set in separate parts of
the constraints (optional and mandatory)

https://codereview.chromium.org/246433006/diff/120001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:183: // These constraints
are handled by cropping if the camera output the wrong
s/output the/outputs a

https://codereview.chromium.org/246433006/diff/120001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:428: // it in that case.
remove them? (and not 'it')

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

https://codereview.chromium.org/246433006/diff/120001/content/renderer/media/...
content/renderer/media/video_track_adapter.cc:60: bool IsEmpty() const { return
callbacks_.empty(); }
would like to have a thread check here

https://codereview.chromium.org/246433006/diff/120001/content/renderer/media/...
content/renderer/media/video_track_adapter.cc:103: DCHECK(callbacks_.empty());
thread check

https://codereview.chromium.org/246433006/diff/120001/content/renderer/media/...
content/renderer/media/video_track_adapter.cc:144: requested_ratio) + 1 & ~1;
why & ~1?
can you add documentation?

https://codereview.chromium.org/246433006/diff/120001/content/renderer/media/...
content/renderer/media/video_track_adapter.cc:157:
media::ComputeLetterboxRegion(frame->visible_rect(), desired_size);
indent

https://codereview.chromium.org/246433006/diff/120001/content/renderer/media/...
content/renderer/media/video_track_adapter.cc:178: for
(std::vector<VideoIdCallbackPair>::const_iterator it = callbacks_.begin();
thread check here or at least a comment that explains where we are?

https://codereview.chromium.org/246433006/diff/120001/content/renderer/media/...
content/renderer/media/video_track_adapter.cc:208: return
max_frame_size_.width() == max_width &&
thread check

https://codereview.chromium.org/246433006/diff/120001/content/renderer/media/...
content/renderer/media/video_track_adapter.cc:219:
VideoTrackAdapter::~VideoTrackAdapter() {
thread check

[perkj_chrome, 2014-05-08 11:59:02]

self review

https://codereview.chromium.org/246433006/diff/120001/content/browser/media/w...
File content/browser/media/webrtc_getusermedia_browsertest.cc (right):

https://codereview.chromium.org/246433006/diff/120001/content/browser/media/w...
content/browser/media/webrtc_getusermedia_browsertest.cc:482: 
one to many lines

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

https://codereview.chromium.org/246433006/diff/120001/content/renderer/media/...
content/renderer/media/video_track_adapter.cc:46: // Removes |callback|
associated with |id| from receiving video frames if
|track|

https://codereview.chromium.org/246433006/diff/120001/content/renderer/media/...
content/renderer/media/video_track_adapter.cc:86: int max_width,
why separate arguments?

https://codereview.chromium.org/246433006/diff/120001/content/renderer/media/...
content/renderer/media/video_track_adapter.cc:223: void
VideoTrackAdapter::AddTrack(MediaStreamVideoTrack* track,
const * track

https://codereview.chromium.org/246433006/diff/120001/content/renderer/media/...
File content/renderer/media/video_track_adapter.h (right):

https://codereview.chromium.org/246433006/diff/120001/content/renderer/media/...
content/renderer/media/video_track_adapter.h:35: void
AddTrack(MediaStreamVideoTrack* track,
const pointer here and below for MediaStreamVideoTrack

[perkj_chrome, 2014-05-08 13:36:24]

PTAL

https://codereview.chromium.org/246433006/diff/120001/content/browser/media/w...
File content/browser/media/webrtc_getusermedia_browsertest.cc (right):

https://codereview.chromium.org/246433006/diff/120001/content/browser/media/w...
content/browser/media/webrtc_getusermedia_browsertest.cc:482: 
On 2014/05/08 11:59:02, perkj wrote:
> one to many lines

Done.

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

https://codereview.chromium.org/246433006/diff/120001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:58: bool
GetConstraintValue(const blink::WebMediaConstraints& constraints,
On 2014/05/08 11:57:58, tommi wrote:
> this feels like copy/paste of some other code. Can you use what's already
> available? (I think Shijing added some utility functions that do this
recently)

This is just a move from further down below. Shijing has not landed his cl yet.

https://codereview.chromium.org/246433006/diff/120001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:66:
base::StringToInt(value_str.utf8(), value);
On 2014/05/08 11:57:58, tommi wrote:
> you need to handle this error

See comment above +  according to base::StringToInt - value is set to best
effort that depends on the error.
This is not changed by this cl.

How about we consider a follow up cl next week where we parse the blink
constraints into a Chrome object that has normal std::vectors and then
convenience functions like this will be simple. Hopefully Shijings patch is then
also landed and we can consider how we should treat parsing errors. I guess, if
its a mandatory- we should fail the request - if its optional- maybe we should
just ignore the  constraint.

https://codereview.chromium.org/246433006/diff/120001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:78:
base::StringToDouble(value_str.utf8(), value);
On 2014/05/08 11:57:58, tommi wrote:
> here too

dito

https://codereview.chromium.org/246433006/diff/120001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:83: bool
HasMandatoryConstraints(const blink::WebMediaConstraints& constraints) {
On 2014/05/08 11:57:58, tommi wrote:
> This feels like an expensive way to check if the mandatory constraints are
> there.  Are we ever calling this method first and then fetching the mandatory
> constraints again?  Could we do without this method?

This is also just a move and not changed in this cl. 
How about we consider a follow up cl next week where we parse the blink
constraints into a Chrome object that has normal std::vectors and then
convenience functions like this will be simple. Hopefully Shijings patch is then
also landed.

https://codereview.chromium.org/246433006/diff/120001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:91: //
std::numeric_limits<int>::max();
On 2014/05/08 11:57:58, tommi wrote:
> also document the behavior if the width and height are set in separate parts
of
> the constraints (optional and mandatory)

This is just a move as well. 

Done for the documentation part.

https://codereview.chromium.org/246433006/diff/120001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:183: // These constraints
are handled by cropping if the camera output the wrong
On 2014/05/08 11:57:58, tommi wrote:
> s/output the/outputs a

Done.

https://codereview.chromium.org/246433006/diff/120001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:428: // it in that case.
On 2014/05/08 11:57:58, tommi wrote:
> remove them? (and not 'it')

Refrased

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

https://codereview.chromium.org/246433006/diff/120001/content/renderer/media/...
content/renderer/media/video_track_adapter.cc:46: // Removes |callback|
associated with |id| from receiving video frames if
On 2014/05/08 11:59:02, perkj wrote:
> |track|

Done.

https://codereview.chromium.org/246433006/diff/120001/content/renderer/media/...
content/renderer/media/video_track_adapter.cc:60: bool IsEmpty() const { return
callbacks_.empty(); }
On 2014/05/08 11:57:58, tommi wrote:
> would like to have a thread check here

Done.

https://codereview.chromium.org/246433006/diff/120001/content/renderer/media/...
content/renderer/media/video_track_adapter.cc:86: int max_width,
On 2014/05/08 11:59:02, perkj wrote:
> why separate arguments? They are separate in the MEdiaStereamVideoSource and
don't really belong togehter. ie one might be set to a valid value but not the
other.

https://codereview.chromium.org/246433006/diff/120001/content/renderer/media/...
content/renderer/media/video_track_adapter.cc:103: DCHECK(callbacks_.empty());
On 2014/05/08 11:57:58, tommi wrote:
> thread check

Done.

https://codereview.chromium.org/246433006/diff/120001/content/renderer/media/...
content/renderer/media/video_track_adapter.cc:144: requested_ratio) + 1 & ~1;
On 2014/05/08 11:57:58, tommi wrote:
> why & ~1?
> can you add documentation?
That is kind of a good question- I did it to avoid rounding errors - but I am
not entirely sure its needed.
Done.

https://codereview.chromium.org/246433006/diff/120001/content/renderer/media/...
content/renderer/media/video_track_adapter.cc:157:
media::ComputeLetterboxRegion(frame->visible_rect(), desired_size);
On 2014/05/08 11:57:58, tommi wrote:
> indent

Done.

https://codereview.chromium.org/246433006/diff/120001/content/renderer/media/...
content/renderer/media/video_track_adapter.cc:178: for
(std::vector<VideoIdCallbackPair>::const_iterator it = callbacks_.begin();
On 2014/05/08 11:57:58, tommi wrote:
> thread check here or at least a comment that explains where we are?

Done.

https://codereview.chromium.org/246433006/diff/120001/content/renderer/media/...
content/renderer/media/video_track_adapter.cc:208: return
max_frame_size_.width() == max_width &&
On 2014/05/08 11:57:58, tommi wrote:
> thread check

Done.

https://codereview.chromium.org/246433006/diff/120001/content/renderer/media/...
content/renderer/media/video_track_adapter.cc:219:
VideoTrackAdapter::~VideoTrackAdapter() {
On 2014/05/08 11:57:58, tommi wrote:
> thread check
Can I do that ? its thread safe recounted and bound both to the IO and the main
render thread.

https://codereview.chromium.org/246433006/diff/120001/content/renderer/media/...
content/renderer/media/video_track_adapter.cc:223: void
VideoTrackAdapter::AddTrack(MediaStreamVideoTrack* track,
On 2014/05/08 11:59:02, perkj wrote:
> const * track

Done.

https://codereview.chromium.org/246433006/diff/120001/content/renderer/media/...
File content/renderer/media/video_track_adapter.h (right):

https://codereview.chromium.org/246433006/diff/120001/content/renderer/media/...
content/renderer/media/video_track_adapter.h:35: void
AddTrack(MediaStreamVideoTrack* track,
On 2014/05/08 11:59:02, perkj wrote:
> const pointer here and below for MediaStreamVideoTrack

Done.

[mcasas, 2014-05-09 07:46:24]

Couple of smaller things.
Possible bug in webrtc_video_capturer_adapter.cc:144?

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

https://codereview.chromium.org/246433006/diff/60001/content/renderer/media/m...
content/renderer/media/media_stream_video_source.cc:273: if (*min_aspect_ratio
!= 0)
On 2014/05/08 11:29:47, perkj wrote:
> On 2014/05/07 14:10:43, mcasas wrote:
> > |min_aspect_ratio| is a double, so this comparison is always true.
> 
> Why would it be true? I set it to 0 at line 248?

A float cannot be 0.0 exactly, and the most likely thing is for the
compiler to upgrade your (int) 0 to (float) 0.0, hence hitting this case.

Float comparisons are usually like
if (abs(*min_aspect_ratio) < epsilon) ...

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

https://codereview.chromium.org/246433006/diff/60001/content/renderer/media/v...
content/renderer/media/video_track_adapter.cc:31: // adaptation and delivers
frames to all registered tracks on the IO-thread.
On 2014/05/08 11:29:47, perkj wrote:
> On 2014/05/07 14:10:43, mcasas wrote:
> > This threading detail should go in the header file.
> 
> This class only exist as an internal class. So documentation related to this
> class should be here. There is also documentation in the header file where I
> forward declare this.

After offline discussion, I buy the classes' division, but suggest
you could add a comment line on the multiplicity, i.e. "per MSVSource
there is at most one VideoTrackAdapter, and that contains up to
#MSVideoTracks objects of class VTA::VFResolutionAdapter's, depending
on the Constraints..." ?

https://codereview.chromium.org/246433006/diff/60001/content/test/data/media/...
File content/test/data/media/getusermedia.html (right):

https://codereview.chromium.org/246433006/diff/60001/content/test/data/media/...
content/test/data/media/getusermedia.html:192: function
twoGetUserMedia(constraints1, constraints2) {
On 2014/05/08 11:29:47, perkj wrote:
> On 2014/05/07 14:10:43, mcasas wrote:
> > s/twoGetUserMedia/getUserMediaTwice/  ? (Also l.193)
> 
> The method call above is called twoGetUserMediaAndStop. So this match that
> convention. Are you ok with this or you want me to change both?

Not a big deal if both methods sound alike.

https://codereview.chromium.org/246433006/diff/140001/content/browser/media/w...
File content/browser/media/webrtc_browsertest.cc (right):

https://codereview.chromium.org/246433006/diff/140001/content/browser/media/w...
content/browser/media/webrtc_browsertest.cc:57:
base::PlatformThread::Sleep(base::TimeDelta::FromSeconds(5));
nit: justify paragraph (to 80col).

https://codereview.chromium.org/246433006/diff/140001/content/browser/media/w...
content/browser/media/webrtc_browsertest.cc:121:
MakeTypicalPeerConnectionCall(javascript);
ultra-nit: Three decimals here and four for 16:9, intended?

https://codereview.chromium.org/246433006/diff/140001/content/browser/media/w...
File content/browser/media/webrtc_getusermedia_browsertest.cc (right):

https://codereview.chromium.org/246433006/diff/140001/content/browser/media/w...
content/browser/media/webrtc_getusermedia_browsertest.cc:481: }
These three tests share code. What about a method
with params (constraints1, constraints2, expected_result)?

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

https://codereview.chromium.org/246433006/diff/140001/content/renderer/media/...
content/renderer/media/media_stream_video_source.h:42: // start.
power-nit: "start." can go in the previous line.

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

https://codereview.chromium.org/246433006/diff/140001/content/renderer/media/...
content/renderer/media/video_track_adapter.cc:30: // render thread but operates
on the IO-thread. It does the resolution
I think VFRA is created in IO-thread, see l.266

https://codereview.chromium.org/246433006/diff/140001/content/renderer/media/...
File content/renderer/media/video_track_adapter.h (right):

https://codereview.chromium.org/246433006/diff/140001/content/renderer/media/...
content/renderer/media/video_track_adapter.h:24: // Adaptations is done by
wrapping the original media::VideoFrame in a new
"VTA is created in the render thread but operates in the IO thread".

When you say "render thread", do you mean "Render Thread", "UI Thread",
or just "a rendering thread" ?

https://codereview.chromium.org/246433006/diff/140001/content/renderer/media/...
File content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc (right):

https://codereview.chromium.org/246433006/diff/140001/content/renderer/media/...
content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:144:
visible_rect.x() / 2;
Should this have parentheses: (y * stride + x) /2 ? -->
I420 subsamples both in vertical and horizontal dimensions.

https://codereview.chromium.org/246433006/diff/140001/content/renderer/media/...
content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:147:
visible_rect.x() / 2;
Likewise.

[tommi (sloooow) - chröme, 2014-05-09 11:16:13]

due to having limited time today I defer to Miguel for the rest of the review.

https://codereview.chromium.org/246433006/diff/140001/content/browser/media/w...
File content/browser/media/webrtc_getusermedia_browsertest.cc (right):

https://codereview.chromium.org/246433006/diff/140001/content/browser/media/w...
content/browser/media/webrtc_getusermedia_browsertest.cc:437: std::string
constraints1 = "{video: true, audio: true}";
nit: why not just |constraints|?  (why do you need to string variables to hold
the same value?)

https://codereview.chromium.org/246433006/diff/140001/content/browser/media/w...
content/browser/media/webrtc_getusermedia_browsertest.cc:476: std::string
command = "twoGetUserMedia(" + constraints1 + ',' +
suggestion: make a method, std::string TwoGetUserMedia(const std::string&
constraints1, const std::string& constraints2);

Then have that method do the string concatenation and return
ExecuteJavascriptAndReturnResult(command).

That way you keep the construction of the javascript in one place and improve
readability of the tests.

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

https://codereview.chromium.org/246433006/diff/140001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:58: bool
GetConstraintValue(const blink::WebMediaConstraints& constraints,
do we want to address the error issue in this cl?

https://codereview.chromium.org/246433006/diff/140001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:66:
base::StringToInt(value_str.utf8(), value);
if not, then would prefer to handle errors from string conversion routines and
treat these constraints as if they were missing as discussed offline.

https://codereview.chromium.org/246433006/diff/140001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:139: return;
does this return statement add anything?

https://codereview.chromium.org/246433006/diff/140001/content/renderer/media/...
File content/renderer/media/mock_media_stream_video_source.cc (right):

https://codereview.chromium.org/246433006/diff/140001/content/renderer/media/...
content/renderer/media/mock_media_stream_video_source.cc:10: #include
"base/message_loop/message_loop.h"
is this include necessary?

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

https://codereview.chromium.org/246433006/diff/140001/content/renderer/media/...
content/renderer/media/video_track_adapter.cc:30: // render thread but operates
on the IO-thread. It does the resolution
On 2014/05/09 07:46:24, mcasas wrote:
> I think VFRA is created in IO-thread, see l.266

Good catch - Per, can we have a thread check there as well please :)

https://codereview.chromium.org/246433006/diff/140001/content/renderer/media/...
File content/renderer/media/video_track_adapter.h (right):

https://codereview.chromium.org/246433006/diff/140001/content/renderer/media/...
content/renderer/media/video_track_adapter.h:24: // Adaptations is done by
wrapping the original media::VideoFrame in a new
On 2014/05/09 07:46:24, mcasas wrote:
> "VTA is created in the render thread but operates in the IO thread".
> 
> When you say "render thread", do you mean "Render Thread", "UI Thread",
> or just "a rendering thread" ?

out of curiosity... why is this comment here and not below where that text is? 
I found it confusing because it doesn't apply to this text at all (right?)

btw, "the main render thread" is a known concept in the render process as the
first thread of that process and has a known role.

https://codereview.chromium.org/246433006/diff/140001/content/renderer/media/...
content/renderer/media/video_track_adapter.h:49: return io_message_loop_;
thread check?

[mcasas, 2014-05-09 13:11:34]

LGTM with:

* refactor common code in webrtc_getusermedia_browsertest.cc
* nit: some minor documentation cleanup in VTA and VFRA:
- which thread is it created in (i.e. "a render thread" versus "the Render
Thread" etc) and where it operates.
- multiplicity relation between MSVS, VTA, VFRA, MSVTrack.

https://codereview.chromium.org/246433006/diff/140001/content/renderer/media/...
File content/renderer/media/video_track_adapter.h (right):

https://codereview.chromium.org/246433006/diff/140001/content/renderer/media/...
content/renderer/media/video_track_adapter.h:24: // Adaptations is done by
wrapping the original media::VideoFrame in a new
On 2014/05/09 11:16:13, tommi wrote:
> On 2014/05/09 07:46:24, mcasas wrote:
> > "VTA is created in the render thread but operates in the IO thread".
> > 
> > When you say "render thread", do you mean "Render Thread", "UI Thread",
> > or just "a rendering thread" ?
> 
> out of curiosity... why is this comment here and not below where that text is?

> I found it confusing because it doesn't apply to this text at all (right?)
> 
Oops, should go in l.30 of VTA .cc, apologies, must have changed too much
between windows.

> btw, "the main render thread" is a known concept in the render process as the
> first thread of that process and has a known role.

Ok, so the Render Thread, then?

https://codereview.chromium.org/246433006/diff/140001/content/renderer/media/...
File content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc (right):

https://codereview.chromium.org/246433006/diff/140001/content/renderer/media/...
content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:144:
visible_rect.x() / 2;
On 2014/05/09 07:46:24, mcasas wrote:
> Should this have parentheses: (y * stride + x) /2 ? -->
> I420 subsamples both in vertical and horizontal dimensions.

Discussed offline: stride of |kUPlane| is already downsampled.

[perkj_chrome, 2014-05-13 11:05:55]

PTAL

https://codereview.chromium.org/246433006/diff/140001/content/browser/media/w...
File content/browser/media/webrtc_browsertest.cc (right):

https://codereview.chromium.org/246433006/diff/140001/content/browser/media/w...
content/browser/media/webrtc_browsertest.cc:57:
base::PlatformThread::Sleep(base::TimeDelta::FromSeconds(5));
On 2014/05/09 07:46:24, mcasas wrote:
> nit: justify paragraph (to 80col).

this does not belong to my cl. Its Phoglunds.. Not sure what you mean either?

https://codereview.chromium.org/246433006/diff/140001/content/browser/media/w...
content/browser/media/webrtc_browsertest.cc:121:
MakeTypicalPeerConnectionCall(javascript);
On 2014/05/09 07:46:24, mcasas wrote:
> ultra-nit: Three decimals here and four for 16:9, intended?

done

https://codereview.chromium.org/246433006/diff/140001/content/browser/media/w...
File content/browser/media/webrtc_getusermedia_browsertest.cc (right):

https://codereview.chromium.org/246433006/diff/140001/content/browser/media/w...
content/browser/media/webrtc_getusermedia_browsertest.cc:437: std::string
constraints1 = "{video: true, audio: true}";
On 2014/05/09 11:16:13, tommi wrote:
> nit: why not just |constraints|?  (why do you need to string variables to hold
> the same value?)

Refactored.

https://codereview.chromium.org/246433006/diff/140001/content/browser/media/w...
content/browser/media/webrtc_getusermedia_browsertest.cc:476: std::string
command = "twoGetUserMedia(" + constraints1 + ',' +
On 2014/05/09 11:16:13, tommi wrote:
> suggestion: make a method, std::string TwoGetUserMedia(const std::string&
> constraints1, const std::string& constraints2);
> 
> Then have that method do the string concatenation and return
> ExecuteJavascriptAndReturnResult(command).
> 
> That way you keep the construction of the javascript in one place and improve

I went for Miguels request below.
> readability of the tests.

https://codereview.chromium.org/246433006/diff/140001/content/browser/media/w...
content/browser/media/webrtc_getusermedia_browsertest.cc:481: }
On 2014/05/09 07:46:24, mcasas wrote:
> These three tests share code. What about a method
> with params (constraints1, constraints2, expected_result)?

Done.

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

https://codereview.chromium.org/246433006/diff/140001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:58: bool
GetConstraintValue(const blink::WebMediaConstraints& constraints,
On 2014/05/09 11:16:13, tommi wrote:
> do we want to address the error issue in this cl?
Actually its ok to do as you suggest- I check the constraints validity in 
UpdateFormatForConstraint so that mean we will fail if the unparsable constraint
is a mandatory.

So we can simply return false in these methods if parsing fails.

https://codereview.chromium.org/246433006/diff/140001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:66:
base::StringToInt(value_str.utf8(), value);
On 2014/05/09 11:16:13, tommi wrote:
> if not, then would prefer to handle errors from string conversion routines and
> treat these constraints as if they were missing as discussed offline.

Done.

https://codereview.chromium.org/246433006/diff/140001/content/renderer/media/...
content/renderer/media/media_stream_video_source.cc:139: return;
On 2014/05/09 11:16:13, tommi wrote:
> does this return statement add anything?

Here was a bug.. Thanks

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

https://codereview.chromium.org/246433006/diff/140001/content/renderer/media/...
content/renderer/media/media_stream_video_source.h:42: // start.
On 2014/05/09 07:46:24, mcasas wrote:
> power-nit: "start." can go in the previous line.
> 

Done.

https://codereview.chromium.org/246433006/diff/140001/content/renderer/media/...
File content/renderer/media/mock_media_stream_video_source.cc (right):

https://codereview.chromium.org/246433006/diff/140001/content/renderer/media/...
content/renderer/media/mock_media_stream_video_source.cc:10: #include
"base/message_loop/message_loop.h"
On 2014/05/09 11:16:13, tommi wrote:
> is this include necessary?

Moved to MediaStreamVideoSource.

https://codereview.chromium.org/246433006/diff/140001/content/renderer/media/...
content/renderer/media/mock_media_stream_video_source.cc:88:
DCHECK(io_message_loop()->BelongsToCurrentThread());
I need to remove this check if I DCHECK that io_message_loop() is only called on
the main render thread.

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

https://codereview.chromium.org/246433006/diff/140001/content/renderer/media/...
content/renderer/media/video_track_adapter.cc:30: // render thread but operates
on the IO-thread. It does the resolution
On 2014/05/09 11:16:13, tommi wrote:
> On 2014/05/09 07:46:24, mcasas wrote:
> > I think VFRA is created in IO-thread, see l.266
> 
> Good catch - Per, can we have a thread check there as well please :)

Nice. - I also realized I should release const VideoCaptureDeliverFrameCB&
callback on the main render thread to avoid problems where the callback is
created on the main render thread but released on the IO-thread. The code is
from Alphas
https://codereview.chromium.org/264363005/diff/210001/content/renderer/media/...

https://codereview.chromium.org/246433006/diff/140001/content/renderer/media/...
File content/renderer/media/video_track_adapter.h (right):

https://codereview.chromium.org/246433006/diff/140001/content/renderer/media/...
content/renderer/media/video_track_adapter.h:24: // Adaptations is done by
wrapping the original media::VideoFrame in a new
On 2014/05/09 07:46:24, mcasas wrote:
> "VTA is created in the render thread but operates in the IO thread".
> 
> When you say "render thread", do you mean "Render Thread", "UI Thread",
> or just "a rendering thread" ?

main render thread usually means the same thread as RenderThread runs on yes. I
you code search, you will see that its used quite frequently.

https://codereview.chromium.org/246433006/diff/140001/content/renderer/media/...
content/renderer/media/video_track_adapter.h:49: return io_message_loop_;
On 2014/05/09 11:16:13, tommi wrote:
> thread check?

Done.

https://codereview.chromium.org/246433006/diff/140001/content/renderer/media/...
File content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc (right):

https://codereview.chromium.org/246433006/diff/140001/content/renderer/media/...
content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:144:
visible_rect.x() / 2;
On 2014/05/09 07:46:24, mcasas wrote:
> Should this have parentheses: (y * stride + x) /2 ? -->
> I420 subsamples both in vertical and horizontal dimensions.

no-  y is the offset of rows- stride of kUPlane is of the Uplane ie for I420-
stride(Uplane)= stride(Yplane/2).

https://codereview.chromium.org/246433006/diff/140001/content/renderer/media/...
content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:147:
visible_rect.x() / 2;
On 2014/05/09 07:46:24, mcasas wrote:
> Likewise.

Done.

[tommi (sloooow) - chröme, 2014-05-14 09:43:37]

lgtm w/ a few minor suggestions/questions

https://codereview.chromium.org/246433006/diff/180001/content/browser/media/w...
File content/browser/media/webrtc_getusermedia_browsertest.cc (right):

https://codereview.chromium.org/246433006/diff/180001/content/browser/media/w...
content/browser/media/webrtc_getusermedia_browsertest.cc:448: std::string
constraints2 = "{video: true, audio: true}";
nit: just use one string? (that's a pretty good way to guarantee it holds the
same value)

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

https://codereview.chromium.org/246433006/diff/180001/content/renderer/media/...
content/renderer/media/video_track_adapter.cc:29: // |callback| will be deleted
when this exits.
indent

https://codereview.chromium.org/246433006/diff/180001/content/renderer/media/...
content/renderer/media/video_track_adapter.cc:106: DCHECK(render_message_loop_);
w00t! dcheck party!
(it's a good thing)

https://codereview.chromium.org/246433006/diff/180001/content/renderer/media/...
content/renderer/media/video_track_adapter.cc:223: callback.reset(new
VideoCaptureDeliverFrameCB(it->second));
add a comment for why this is done inside a scope... I'm not seeing why :)

I don't see anything constructed on the stack inside the scope, so I think this
should simply be:

scoped_ptr<VideoCaptureDeliverFrameCB> callback(new
VideoCaptureDeliverFrameCB(it->second));
callbacks_.erase(it);
render_message_loop_->PostTask(
    FROM_HERE, base::Bind(&ResetCallbackOnMainRenderThread,
                          base::Passed(&callback)));


However, I'm guessing I'm just not seeing it.

[perkj_chrome, 2014-05-14 12:37:35]

The CQ bit was checked by perkj@chromium.org

[commit-bot: I haz the power, 2014-05-14 12:37:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/perkj@chromium.org/246433006/200001

[perkj_chrome, 2014-05-14 13:17:56]

The CQ bit was unchecked by perkj@chromium.org

[perkj_chrome, 2014-05-14 13:29:33]

The CQ bit was checked by perkj@chromium.org

[perkj_chrome, 2014-05-14 13:29:43]

https://codereview.chromium.org/246433006/diff/180001/content/browser/media/w...
File content/browser/media/webrtc_getusermedia_browsertest.cc (right):

https://codereview.chromium.org/246433006/diff/180001/content/browser/media/w...
content/browser/media/webrtc_getusermedia_browsertest.cc:448: std::string
constraints2 = "{video: true, audio: true}";
On 2014/05/14 09:43:38, tommi wrote:
> nit: just use one string? (that's a pretty good way to guarantee it holds the
> same value)

Done.

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

https://codereview.chromium.org/246433006/diff/180001/content/renderer/media/...
content/renderer/media/video_track_adapter.cc:29: // |callback| will be deleted
when this exits.
On 2014/05/14 09:43:38, tommi wrote:
> indent

Done.

https://codereview.chromium.org/246433006/diff/180001/content/renderer/media/...
content/renderer/media/video_track_adapter.cc:106: DCHECK(render_message_loop_);
On 2014/05/14 09:43:38, tommi wrote:
> w00t! dcheck party!
> (it's a good thing)

Done.

https://codereview.chromium.org/246433006/diff/180001/content/renderer/media/...
content/renderer/media/video_track_adapter.cc:223: callback.reset(new
VideoCaptureDeliverFrameCB(it->second));
On 2014/05/14 09:43:38, tommi wrote:
> add a comment for why this is done inside a scope... I'm not seeing why :)
> 
> I don't see anything constructed on the stack inside the scope, so I think
this
> should simply be:
> 
> scoped_ptr<VideoCaptureDeliverFrameCB> callback(new
> VideoCaptureDeliverFrameCB(it->second));
> callbacks_.erase(it);
> render_message_loop_->PostTask(
>     FROM_HERE, base::Bind(&ResetCallbackOnMainRenderThread,
>                           base::Passed(&callback)));
> 
> 
> However, I'm guessing I'm just not seeing it.
You are right. Since the object is on the heap

[commit-bot: I haz the power, 2014-05-14 13:29:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/perkj@chromium.org/246433006/220001

[perkj_chrome, 2014-05-14 15:17:07]

The CQ bit was unchecked by perkj@chromium.org

[perkj_chrome, 2014-05-15 06:58:25]

The CQ bit was checked by perkj@chromium.org

[commit-bot: I haz the power, 2014-05-15 06:59:48]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/perkj@chromium.org/246433006/240001

[commit-bot: I haz the power, 2014-05-15 08:58:43]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-15 09:01:32]

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

[commit-bot: I haz the power, 2014-05-15 09:01:33]

Try jobs failed on following builders:
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)

[perkj_chrome, 2014-05-15 13:31:23]

The CQ bit was checked by perkj@chromium.org

[commit-bot: I haz the power, 2014-05-15 13:32:00]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/perkj@chromium.org/246433006/260001

[commit-bot: I haz the power, 2014-05-15 17:27:39]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-15 17:30:48]

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

[commit-bot: I haz the power, 2014-05-15 17:30:48]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)

[perkj_chrome, 2014-05-19 18:54:56]

The CQ bit was checked by perkj@chromium.org

[commit-bot: I haz the power, 2014-05-19 18:56:25]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/perkj@chromium.org/246433006/360001

[commit-bot: I haz the power, 2014-05-19 19:29:59]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)

[commit-bot: I haz the power, 2014-05-19 19:33:57]

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

[commit-bot: I haz the power, 2014-05-19 19:33:58]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[mcasas, 2014-05-20 07:19:52]

LGTM with a nit.

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

https://codereview.chromium.org/246433006/diff/360001/content/renderer/media/...
content/renderer/media/video_track_adapter.cc:49: // |callback| will however be
released on thee main render thread.
s/thee/the/

[tommi (sloooow) - chröme, 2014-05-20 07:30:57]

Hej Per - I see you've been trying to land this for almost a week.  Are the
errors genuine or trybot flake?

[mcasas, 2014-05-20 07:36:08]

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

https://codereview.chromium.org/246433006/diff/360001/content/renderer/media/...
content/renderer/media/video_track_adapter.cc:227: base::Passed(&callback)));
PS-nit: consider "RelaseSoon()"

https://code.google.com/p/chromium/codesearch#chromium/src/base/message_loop/...

[perkj_chrome, 2014-05-20 07:38:11]

On 2014/05/20 07:36:08, mcasas wrote:
>
https://codereview.chromium.org/246433006/diff/360001/content/renderer/media/...
> File content/renderer/media/video_track_adapter.cc (right):
> 
>
https://codereview.chromium.org/246433006/diff/360001/content/renderer/media/...
> content/renderer/media/video_track_adapter.cc:227: base::Passed(&callback)));
> PS-nit: consider "RelaseSoon()"
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/message_loop/...

I found the problem yesterday. See latest patchset. The last failures is due to
that the patch don't apply any longer.

[phoglund_chromium, 2014-05-20 08:03:42]

On 2014/05/20 07:38:11, perkj wrote:
> On 2014/05/20 07:36:08, mcasas wrote:
> >
>
https://codereview.chromium.org/246433006/diff/360001/content/renderer/media/...
> > File content/renderer/media/video_track_adapter.cc (right):
> > 
> >
>
https://codereview.chromium.org/246433006/diff/360001/content/renderer/media/...
> > content/renderer/media/video_track_adapter.cc:227:
base::Passed(&callback)));
> > PS-nit: consider "RelaseSoon()"
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/message_loop/...
> 
> I found the problem yesterday. See latest patchset. The last failures is due
to
> that the patch don't apply any longer.

Wohoo, nice job Per!

[perkj_chrome, 2014-05-22 07:06:04]

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

https://codereview.chromium.org/246433006/diff/360001/content/renderer/media/...
content/renderer/media/video_track_adapter.cc:49: // |callback| will however be
released on thee main render thread.
On 2014/05/20 07:19:53, mcasas wrote:
> s/thee/the/

Done.

https://codereview.chromium.org/246433006/diff/360001/content/renderer/media/...
content/renderer/media/video_track_adapter.cc:227: base::Passed(&callback)));
On 2014/05/20 07:36:09, mcasas wrote:
> PS-nit: consider "RelaseSoon()"
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/message_loop/...

It should be DeleteSoon since this is not reference counted. But I had problems
using that in the unittests and Valgrind.

[perkj_chrome, 2014-05-22 07:06:19]

The CQ bit was checked by perkj@chromium.org

[commit-bot: I haz the power, 2014-05-22 07:09:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/perkj@chromium.org/246433006/420001

[commit-bot: I haz the power, 2014-05-22 17:24:03]

Change committed as 272214