Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
183113004:
Make sure MediaStreamVideoSource support cropping. (Closed)
|
Created:
6 years, 9 months ago by perkj_chrome Modified:
6 years, 9 months ago CC:
chromium-reviews, fischman+watch_chromium.org, jam, mcasas+watch_chromium.org, joi+watch-content_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, wjia+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionMake sure MediaStreamVideoSource support cropping.
Currently cropping is done for Chrome in libjingle. But as part of project Piranha plant we want to make sure local MediaStreamVideoTracks do not depend on libjingle and that the implementation is on par with what libjingle supports.
This cl creates a new wrapped media::VideoFrame with a changed view_rect when cropping is needed.
When the frame is provided to a PeerConnection via WebRtcVideoCapturerAdapter the frame is copied using
libyuv to a tightly packed frame.
Note that libyuv is already used in libjingle from the render process and is used by Chrome in the browser process.
BUG=334241, 349450
TBR Jam for content_tests.gypi change.
R=joi@chromium.org, mflodman@chromium.org, tommi@chromium.org
TBR=jam
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255573
Patch Set 1 #
Total comments: 14
Patch Set 2 : Addressed review comments. #Patch Set 3 : Fix formatting and lint. #Patch Set 4 : Fixed unittest #Patch Set 5 : Move usage of libyuv to webrtc adapter. #Patch Set 6 : Implement cropping using media::VideoFrame::view_rect() #
Total comments: 2
Patch Set 7 : Crop with origin 0,0 and temporary disable failing test. #
Total comments: 63
Patch Set 8 : Addressed code review. #
Total comments: 8
Patch Set 9 : Addressed review comments. #Patch Set 10 : And fixed a mistake in new GetConstraintValue #
Total comments: 1
Patch Set 11 : Rebased #Patch Set 12 : Removed extra spaces in content_unittests #Messages
Total messages: 23 (0 generated)
https://codereview.chromium.org/183113004/diff/1/media/base/video_frame_pool.cc File media/base/video_frame_pool.cc (right): https://codereview.chromium.org/183113004/diff/1/media/base/video_frame_pool.... media/base/video_frame_pool.cc:78: frame->SetTimestamp(timestamp); Note - this change will be landed in a separate cl.
Mostly some nits. https://codereview.chromium.org/183113004/diff/1/content/renderer/media/media... File content/renderer/media/media_stream_video_source.cc (right): https://codereview.chromium.org/183113004/diff/1/content/renderer/media/media... content/renderer/media/media_stream_video_source.cc:40: // MediaStreamVideoSource support cropping of video frames but only up to support -> supports https://codereview.chromium.org/183113004/diff/1/content/renderer/media/media... content/renderer/media/media_stream_video_source.cc:255: *capture_format = GetBestFormatBasedOnArea( It seems this statement will never choose a capture format larger than 640x480. Is that intentional? https://codereview.chromium.org/183113004/diff/1/content/renderer/media/media... content/renderer/media/media_stream_video_source.cc:274: DCHECK(src_width >= dst_width); Why not > rather than >=, to make sure we're not calling this (relatively expensive) function unnecessarily? https://codereview.chromium.org/183113004/diff/1/content/renderer/media/media... File content/renderer/media/media_stream_video_source_unittest.cc (right): https://codereview.chromium.org/183113004/diff/1/content/renderer/media/media... content/renderer/media/media_stream_video_source_unittest.cc:131: // Test that the source crop to the requested max width and source crop -> source crops https://codereview.chromium.org/183113004/diff/1/content/renderer/media/media... content/renderer/media/media_stream_video_source_unittest.cc:135: void TestSourceCroppFrame(int capture_w, TestSourceCroppFrame -> TestSourceCropFrame https://codereview.chromium.org/183113004/diff/1/content/renderer/media/media... content/renderer/media/media_stream_video_source_unittest.cc:406: TEST_F(MediaStreamVideoSourceTest, DeliverSmallerSizeWhenToLargeMax) { WhenToLargeMax -> WhenTooLargeMax
PTAL https://codereview.chromium.org/183113004/diff/1/content/renderer/media/media... File content/renderer/media/media_stream_video_source.cc (right): https://codereview.chromium.org/183113004/diff/1/content/renderer/media/media... content/renderer/media/media_stream_video_source.cc:40: // MediaStreamVideoSource support cropping of video frames but only up to On 2014/02/27 14:52:25, Jói wrote: > support -> supports Done. https://codereview.chromium.org/183113004/diff/1/content/renderer/media/media... content/renderer/media/media_stream_video_source.cc:255: *capture_format = GetBestFormatBasedOnArea( On 2014/02/27 14:52:25, Jói wrote: > It seems this statement will never choose a capture format larger than 640x480. > Is that intentional? Its intentional that we choose a size that we support that is as close as posible to 640*480. Butbefore this function is called the supported formats has been filtered by constraints. So if a constraint has requested a size with minHeight > 480 or minWidth > 640 that is what we will get. See for example the unittest MediaStreamVideoSourceTest, MandatoryMinVgaOptional720P https://codereview.chromium.org/183113004/diff/1/content/renderer/media/media... content/renderer/media/media_stream_video_source.cc:274: DCHECK(src_width >= dst_width); On 2014/02/27 14:52:25, Jói wrote: > Why not > rather than >=, to make sure we're not calling this (relatively > expensive) function unnecessarily? Done. https://codereview.chromium.org/183113004/diff/1/content/renderer/media/media... File content/renderer/media/media_stream_video_source_unittest.cc (right): https://codereview.chromium.org/183113004/diff/1/content/renderer/media/media... content/renderer/media/media_stream_video_source_unittest.cc:131: // Test that the source crop to the requested max width and On 2014/02/27 14:52:25, Jói wrote: > source crop -> source crops Done. https://codereview.chromium.org/183113004/diff/1/content/renderer/media/media... content/renderer/media/media_stream_video_source_unittest.cc:135: void TestSourceCroppFrame(int capture_w, On 2014/02/27 14:52:25, Jói wrote: > TestSourceCroppFrame -> TestSourceCropFrame Done. https://codereview.chromium.org/183113004/diff/1/content/renderer/media/media... content/renderer/media/media_stream_video_source_unittest.cc:406: TEST_F(MediaStreamVideoSourceTest, DeliverSmallerSizeWhenToLargeMax) { On 2014/02/27 14:52:25, Jói wrote: > WhenToLargeMax -> WhenTooLargeMax Done.
This cl needs some more work and discussions.
https://codereview.chromium.org/183113004/diff/1/content/renderer/media/media... File content/renderer/media/media_stream_video_source.cc (right): https://codereview.chromium.org/183113004/diff/1/content/renderer/media/media... content/renderer/media/media_stream_video_source.cc:255: *capture_format = GetBestFormatBasedOnArea( On 2014/03/01 12:32:59, perkj wrote: > On 2014/02/27 14:52:25, Jói wrote: > > It seems this statement will never choose a capture format larger than > 640x480. > > Is that intentional? > > Its intentional that we choose a size that we support that is as close as > posible to 640*480. > Butbefore this function is called the supported formats has been filtered by > constraints. So if a constraint has requested a size with minHeight > 480 or > minWidth > 640 that is what we will get. > > See for example the unittest MediaStreamVideoSourceTest, > MandatoryMinVgaOptional720P OK, thanks, I see the format filtering now. Still not sure why the max width and height are clamped at 640x480 when calling GetBestFormatBasedOnArea. But will hold off on further review based on your last email.
Will hold off on review according to your last post.
ok- I think we have settled on how cropping should be done. I will try to land the needed changes in media::VideoFrame in a separate cl. But please review this. Thanks
LGTM apart from the question below. https://codereview.chromium.org/183113004/diff/120001/content/renderer/media/... File content/renderer/media/media_stream_video_source.cc (right): https://codereview.chromium.org/183113004/diff/120001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:369: ((frame->coded_size().width() - visible_width) / 2) & ~1; Why do you have the & ~1 bit, which (unless my mind is not working today) snaps |horiz_crop| to multiples of 2? The division by 2 already will truncate down to the nearest integer, not up. Consider a visible_width 2 smaller than the current width. Why wouldn't we want to crop that by 1 pixel horizontally (on the left hand side) which since the visible_width is 2 smaller would also crop it by 1 pixel on the right hand side? If there's a reason, perhaps a comment would be in order. I'm guessing this might be because YV12 and I420 are both 16-bit so two pixels pack into one word? Same comment for height below.
Hej I changed to that we now crop and set the offset to 0,0 instead of to the center of the original image. This is temporary until the render pipeline has been fixed. I would like to submit this so that I can land the video track cl and unblock cast. Tommi, can you please review? https://codereview.chromium.org/183113004/diff/120001/content/renderer/media/... File content/renderer/media/media_stream_video_source.cc (right): https://codereview.chromium.org/183113004/diff/120001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:369: ((frame->coded_size().width() - visible_width) / 2) & ~1; On 2014/03/05 15:57:10, Jói wrote: > Why do you have the & ~1 bit, which (unless my mind is not working today) snaps > |horiz_crop| to multiples of 2? The division by 2 already will truncate down to > the nearest integer, not up. > > Consider a visible_width 2 smaller than the current width. Why wouldn't we want > to crop that by 1 pixel horizontally (on the left hand side) which since the > visible_width is 2 smaller would also crop it by 1 pixel on the right hand side? > > If there's a reason, perhaps a comment would be in order. I'm guessing this > might be because YV12 and I420 are both 16-bit so two pixels pack into one word? > > Same comment for height below. Good question- I dont' understand either why setting the offset to an even line is better than add odd. This was taken from the libjingle file WebRtcVideoFrame::Reset that previously did the cropping. I can't see that we need it so I remove it.
https://codereview.chromium.org/183113004/diff/160001/content/browser/media/w... File content/browser/media/webrtc_getusermedia_browsertest.cc (right): https://codereview.chromium.org/183113004/diff/160001/content/browser/media/w... content/browser/media/webrtc_getusermedia_browsertest.cc:397: // Currently the render pipeline don't support cropping where the new cropped don't -> doesn't https://codereview.chromium.org/183113004/diff/160001/content/browser/media/w... content/browser/media/webrtc_getusermedia_browsertest.cc:398: // frame don't have the same top left pixel as the original frame. don't -> doesn't nit: pixel -> coordinates https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... File content/renderer/media/media_stream_video_source.cc (right): https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:41: // kMaxCropFactor. I'm not understanding this. Does this mean that (cx*cy)/2>= cropped_cx*cropped_cy? Does it account for aspect ratio? https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:195: int* dw, int* dh) { full variable names https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:209: if (mandatory_found) What if height is specified as mandatory and width as optional - is there any reason we would not want to pick up the width? I actually don't see much value in the 'mandatory_found' variable in this function or separate height and width strings. Why wouldn't we simply do this: blink::WebString web_string; if (constraints.getMandatoryConstraintValue( MediaStreamVideoSource::kMaxWidth, web_string) || constraints.getOptionalConstraintValue( MediaStreamVideoSource::kMaxWidth, web_string)) { base::StringToInt(web_string.utf8(), dw); } if (constraints.getMandatoryConstraintValue( MediaStreamVideoSource::kMaxHeight, web_string) || constraints.getOptionalConstraintValue( MediaStreamVideoSource::kMaxHeight, web_string)) { base::StringToInt(web_string.utf8(), dh); } https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:241: // resolution since higher resolution cost more in terms of complexity and nit: resolutions https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:242: // many cameras perform worse at its maximum supported resolution. its -> their Regarding the performance statement itself, how do we measure camera performance in this context? I know that the frame sizes will of course be bigger but how does the camera actually perform worse? https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:268: // in MediaStreamVideoSource::DeliverVideoFrame if cropping is needed. I don't understand this actually :) The method doesn't do anything, so how can it be used for keeping a reference? https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:316: // the capabilities has been retrieved. since you're fixing this you might as well fix has->have https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:374: // crbug/349450. The effect of this is that the cropped frame originate nit: originates https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:378: // ((frame->visible_rect().width() - visible_width) / 2); align // https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:389: visible_height); nit: this should be indented by 4 spaces https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... File content/renderer/media/media_stream_video_source_unittest.cc (right): https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... content/renderer/media/media_stream_video_source_unittest.cc:138: int extpected_h, expected_h (or even expected_height) https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... content/renderer/media/media_stream_video_source_unittest.cc:158: one empty line https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... File content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc (right): https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:117: // Libjingle expect contiguous layout of image planes as input. expects https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:129: media::VideoFrame::I420 == frame->format() ? fix indent https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:147: DCHECK(src_width > dst_width || src_height > dst_height); should these checks be >= ? also, should the || be && ? https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:154: const int halfwidth = (src_width + 1) / 2; half_width (or center depending on how you prefer to think it) https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... File content/renderer/media/webrtc/webrtc_video_capturer_adapter.h (right): https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter.h:55: // Buffer used if cropping is needed. can you document ownership/lifetime? https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... File content/renderer/media/webrtc/webrtc_video_capturer_adapter_unittest.cc (right): https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter_unittest.cc:14: : public sigslot::has_slots<>, this code is in chrome... I don't think we want to start bringing in has_slots. use base::Callback (aka Closure) instead https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter_unittest.cc:18: :adapter_(false), space after : https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter_unittest.cc:25: void TestSourceCropFrame(int capture_w, full variable names https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter_unittest.cc:27: int extpected_w, expected https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter_unittest.cc:49: } fix https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter_unittest.cc:53: const cricket::CapturedFrame* captured_frame_; who owns?
Addressed code review comments. PTAL. https://codereview.chromium.org/183113004/diff/160001/content/browser/media/w... File content/browser/media/webrtc_getusermedia_browsertest.cc (right): https://codereview.chromium.org/183113004/diff/160001/content/browser/media/w... content/browser/media/webrtc_getusermedia_browsertest.cc:397: // Currently the render pipeline don't support cropping where the new cropped On 2014/03/06 10:14:09, tommi wrote: > don't -> doesn't Done. https://codereview.chromium.org/183113004/diff/160001/content/browser/media/w... content/browser/media/webrtc_getusermedia_browsertest.cc:398: // frame don't have the same top left pixel as the original frame. On 2014/03/06 10:14:09, tommi wrote: > don't -> doesn't > nit: pixel -> coordinates Done. https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... File content/renderer/media/media_stream_video_source.cc (right): https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:41: // kMaxCropFactor. On 2014/03/06 10:14:09, tommi wrote: > I'm not understanding this. Does this mean that (cx*cy)/2>= > cropped_cx*cropped_cy? > Does it account for aspect ratio? Please see new comment. https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:195: int* dw, int* dh) { On 2014/03/06 10:14:09, tommi wrote: > full variable names Done. https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:209: if (mandatory_found) On 2014/03/06 10:14:09, tommi wrote: > What if height is specified as mandatory and width as optional - is there any > reason we would not want to pick up the width? > > I actually don't see much value in the 'mandatory_found' variable in this > function or separate height and width strings. > Why wouldn't we simply do this: > > blink::WebString web_string; > if (constraints.getMandatoryConstraintValue( > MediaStreamVideoSource::kMaxWidth, web_string) || > constraints.getOptionalConstraintValue( > MediaStreamVideoSource::kMaxWidth, web_string)) { > base::StringToInt(web_string.utf8(), dw); > } > > if (constraints.getMandatoryConstraintValue( > MediaStreamVideoSource::kMaxHeight, web_string) || > constraints.getOptionalConstraintValue( > MediaStreamVideoSource::kMaxHeight, web_string)) { > base::StringToInt(web_string.utf8(), dh); > } I did that first. The whole think gets kind of complicated fast. The idea is to use the filtering mechanism of mandatory constraints and optional constraints against the "native resolution" first and then crop if requested. It feels a bit scary to treat maxWidht from the mandatory set and maxHeight from the optional set. I feels like a higher risk of breaking the expected aspect ratio. Therefore I decided to simply ignore the optional if a mandatory constraint has been given. That is at least not wrong since optional is optional. https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:241: // resolution since higher resolution cost more in terms of complexity and On 2014/03/06 10:14:09, tommi wrote: > nit: resolutions Done. https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:242: // many cameras perform worse at its maximum supported resolution. On 2014/03/06 10:14:09, tommi wrote: > its -> their > > Regarding the performance statement itself, how do we measure camera performance > in this context? > I know that the frame sizes will of course be bigger but how does the camera > actually perform worse? frame rate is lower etc. Updated comment. https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:268: // in MediaStreamVideoSource::DeliverVideoFrame if cropping is needed. On 2014/03/06 10:14:09, tommi wrote: > I don't understand this actually :) > The method doesn't do anything, so how can it be used for keeping a reference? See base::Bind(&ReleaseOriginalFrame, frame)) further down. Do you have a better idea how do do frame->AddRef and frame->Release() bounded to callback? I asked Joi and Alpha. https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:316: // the capabilities has been retrieved. On 2014/03/06 10:14:09, tommi wrote: > since you're fixing this you might as well fix has->have Done. https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:374: // crbug/349450. The effect of this is that the cropped frame originate On 2014/03/06 10:14:09, tommi wrote: > nit: originates Done. https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:378: // ((frame->visible_rect().width() - visible_width) / 2); On 2014/03/06 10:14:09, tommi wrote: > align // Done. https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:389: visible_height); On 2014/03/06 10:14:09, tommi wrote: > nit: this should be indented by 4 spaces Done. https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... File content/renderer/media/media_stream_video_source_unittest.cc (right): https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... content/renderer/media/media_stream_video_source_unittest.cc:138: int extpected_h, On 2014/03/06 10:14:09, tommi wrote: > expected_h > (or even expected_height) Done. https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... content/renderer/media/media_stream_video_source_unittest.cc:158: On 2014/03/06 10:14:09, tommi wrote: > one empty line Done. https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... File content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc (right): https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:117: // Libjingle expect contiguous layout of image planes as input. On 2014/03/06 10:14:09, tommi wrote: > expects Done. https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:129: media::VideoFrame::I420 == frame->format() ? On 2014/03/06 10:14:09, tommi wrote: > fix indent Done. https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:147: DCHECK(src_width > dst_width || src_height > dst_height); On 2014/03/06 10:14:09, tommi wrote: > should these checks be >= ? > > also, should the || be && ? Done. https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:154: const int halfwidth = (src_width + 1) / 2; On 2014/03/06 10:14:09, tommi wrote: > half_width > > (or center depending on how you prefer to think it) Done. https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... File content/renderer/media/webrtc/webrtc_video_capturer_adapter.h (right): https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter.h:55: // Buffer used if cropping is needed. On 2014/03/06 10:14:09, tommi wrote: > can you document ownership/lifetime? Done. https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... File content/renderer/media/webrtc/webrtc_video_capturer_adapter_unittest.cc (right): https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter_unittest.cc:14: : public sigslot::has_slots<>, On 2014/03/06 10:14:09, tommi wrote: > this code is in chrome... I don't think we want to start bringing in has_slots. > > use base::Callback (aka Closure) instead This tests the wrapper that implements a libjingle cricket::VideoCapturer. To test that the video frame we actually provide to libjingle is what we expect we need this. Ie, its needed in order to connect to adapter_.SignalFrameCaptured Note that there is another test (not by me :->) video_destination_handler_unittest.cc that do the same thing for the same reason. https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter_unittest.cc:18: :adapter_(false), On 2014/03/06 10:14:09, tommi wrote: > space after : Done. https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter_unittest.cc:18: :adapter_(false), On 2014/03/06 10:14:09, tommi wrote: > space after : Done. https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter_unittest.cc:25: void TestSourceCropFrame(int capture_w, On 2014/03/06 10:14:09, tommi wrote: > full variable names Done. https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter_unittest.cc:27: int extpected_w, On 2014/03/06 10:14:09, tommi wrote: > expected Done. https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter_unittest.cc:49: } On 2014/03/06 10:14:09, tommi wrote: > fix Done. https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter_unittest.cc:53: const cricket::CapturedFrame* captured_frame_; On 2014/03/06 10:14:09, tommi wrote: > who owns? Humm- right- this is wrong. It points to a stack variable so it was just luck that it works.
https://codereview.chromium.org/183113004/diff/160001/content/browser/media/w... File content/browser/media/webrtc_getusermedia_browsertest.cc (right): https://codereview.chromium.org/183113004/diff/160001/content/browser/media/w... content/browser/media/webrtc_getusermedia_browsertest.cc:398: // frame don't have the same top left pixel as the original frame. On 2014/03/06 12:45:42, perkj wrote: > On 2014/03/06 10:14:09, tommi wrote: > > don't -> doesn't > > nit: pixel -> coordinates > > Done. missed fixing the "don't vs doesn't typo" :) https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... File content/renderer/media/media_stream_video_source.cc (right): https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:195: int* dw, int* dh) { On 2014/03/06 12:45:42, perkj wrote: > On 2014/03/06 10:14:09, tommi wrote: > > full variable names > > Done. Thanks... at first I thought the 'd' stood for delta. So this a good change. https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:209: if (mandatory_found) On 2014/03/06 12:45:42, perkj wrote: > On 2014/03/06 10:14:09, tommi wrote: > > What if height is specified as mandatory and width as optional - is there any > > reason we would not want to pick up the width? > > > > I actually don't see much value in the 'mandatory_found' variable in this > > function or separate height and width strings. > > Why wouldn't we simply do this: > > > > blink::WebString web_string; > > if (constraints.getMandatoryConstraintValue( > > MediaStreamVideoSource::kMaxWidth, web_string) || > > constraints.getOptionalConstraintValue( > > MediaStreamVideoSource::kMaxWidth, web_string)) { > > base::StringToInt(web_string.utf8(), dw); > > } > > > > if (constraints.getMandatoryConstraintValue( > > MediaStreamVideoSource::kMaxHeight, web_string) || > > constraints.getOptionalConstraintValue( > > MediaStreamVideoSource::kMaxHeight, web_string)) { > > base::StringToInt(web_string.utf8(), dh); > > } > > I did that first. > The whole think gets kind of complicated fast. The idea is to use the filtering > mechanism of mandatory constraints and optional constraints against the "native > resolution" first and then crop if requested. It feels a bit scary to treat > maxWidht from the mandatory set and maxHeight from the optional set. I feels > like a higher risk of breaking the expected aspect ratio. Therefore I decided to > simply ignore the optional if a mandatory constraint has been given. That is at > least not wrong since optional is optional. OK that's probably the safe thing to do. Btw, you could reduce code duplication in this function by having a utility function at the top of this file: bool GetConstraintValue(const blink::WebMediaConstraints& constraints, bool mandatory, const char* name, int* value) { blink::WebString value_str; bool ret = mandatory ? constraints.getMandatoryConstraintValue(name, value_str) : constraints.getOptionalConstraintValue(name, value_str); if (ret) *value = base::StringToInt(value.utf8(), value); return ret; } and then here the code will be a little easier to follow: bool mandatory = GetConstraintValue(constraints, true, MediaStreamVideoSource::kMaxWidth, desired_width); mandatory |= GetConstraintValue(constraints, true, MediaStreamVideoSource::kMaxHeight, desired_height); if (mandatory) return; GetConstraintValue(constraints, false, MediaStreamVideoSource::kMaxWidth, desired_width); GetConstraintValue(constraints, false, MediaStreamVideoSource::kMaxHeight, desired_height); https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:268: // in MediaStreamVideoSource::DeliverVideoFrame if cropping is needed. On 2014/03/06 12:45:42, perkj wrote: > On 2014/03/06 10:14:09, tommi wrote: > > I don't understand this actually :) > > The method doesn't do anything, so how can it be used for keeping a reference? > > See base::Bind(&ReleaseOriginalFrame, frame)) further down. Do you have a better > idea how do do frame->AddRef and frame->Release() bounded to callback? I asked > Joi and Alpha. Ah, I missed that. Can you add something to the comments that explain that we keep the reference in the closure that eventually calls this method? https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... File content/renderer/media/media_stream_video_source_unittest.cc (right): https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... content/renderer/media/media_stream_video_source_unittest.cc:138: int extpected_h, On 2014/03/06 12:45:42, perkj wrote: > On 2014/03/06 10:14:09, tommi wrote: > > expected_h > > (or even expected_height) > > Done. missed extpected -> expected https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... File content/renderer/media/webrtc/webrtc_video_capturer_adapter_unittest.cc (right): https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter_unittest.cc:14: : public sigslot::has_slots<>, On 2014/03/06 12:45:42, perkj wrote: > On 2014/03/06 10:14:09, tommi wrote: > > this code is in chrome... I don't think we want to start bringing in > has_slots. > > > > use base::Callback (aka Closure) instead > > This tests the wrapper that implements a libjingle cricket::VideoCapturer. > To test that the video frame we actually provide to libjingle is what we expect > we need this. Ie, its needed in order to connect to > adapter_.SignalFrameCaptured > Note that there is another test (not by me :->) > video_destination_handler_unittest.cc that do the same thing for the same > reason. Ah, I see. I thought WebRtcVideoCapturerAdapter was a pure Chrome type, but it appears to be a hybrid. OK, as is then. https://codereview.chromium.org/183113004/diff/170001/content/renderer/media/... File content/renderer/media/media_stream_video_source.cc (right): https://codereview.chromium.org/183113004/diff/170001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:41: // kMaxCropFactor. Ie - if a constraint it set to maxHeight 360, an original it->is https://codereview.chromium.org/183113004/diff/170001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:42: // input frame height of max 360 * kMaxCropFactor pixels is accepted. ah, I see, so this is a limitation of each of the axis. https://codereview.chromium.org/183113004/diff/170001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:243: // many cameras has lower frame rate and have more noise in the image at cameras has -> cameras have https://codereview.chromium.org/183113004/diff/170001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:244: // its maximum supported resolution. its->their
PTAL https://codereview.chromium.org/183113004/diff/160001/content/browser/media/w... File content/browser/media/webrtc_getusermedia_browsertest.cc (right): https://codereview.chromium.org/183113004/diff/160001/content/browser/media/w... content/browser/media/webrtc_getusermedia_browsertest.cc:398: // frame don't have the same top left pixel as the original frame. On 2014/03/06 13:28:50, tommi wrote: > On 2014/03/06 12:45:42, perkj wrote: > > On 2014/03/06 10:14:09, tommi wrote: > > > don't -> doesn't > > > nit: pixel -> coordinates > > > > Done. > > missed fixing the "don't vs doesn't typo" :) Sorry about that. Strange that I do the same grammatical errors over and over again..... https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... File content/renderer/media/media_stream_video_source.cc (right): https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:195: int* dw, int* dh) { On 2014/03/06 13:28:50, tommi wrote: > On 2014/03/06 12:45:42, perkj wrote: > > On 2014/03/06 10:14:09, tommi wrote: > > > full variable names > > > > Done. > > Thanks... at first I thought the 'd' stood for delta. So this a good change. Done. https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:209: if (mandatory_found) For some reason I get a compile error with const char* name. but not if I do const blink::WebString& name. I don't understand why it matters where the conversion between WebString and const char * is done but.... On 2014/03/06 13:28:50, tommi wrote: > On 2014/03/06 12:45:42, perkj wrote: > > On 2014/03/06 10:14:09, tommi wrote: > > > What if height is specified as mandatory and width as optional - is there > any > > > reason we would not want to pick up the width? > > > > > > I actually don't see much value in the 'mandatory_found' variable in this > > > function or separate height and width strings. > > > Why wouldn't we simply do this: > > > > > > blink::WebString web_string; > > > if (constraints.getMandatoryConstraintValue( > > > MediaStreamVideoSource::kMaxWidth, web_string) || > > > constraints.getOptionalConstraintValue( > > > MediaStreamVideoSource::kMaxWidth, web_string)) { > > > base::StringToInt(web_string.utf8(), dw); > > > } > > > > > > if (constraints.getMandatoryConstraintValue( > > > MediaStreamVideoSource::kMaxHeight, web_string) || > > > constraints.getOptionalConstraintValue( > > > MediaStreamVideoSource::kMaxHeight, web_string)) { > > > base::StringToInt(web_string.utf8(), dh); > > > } > > > > I did that first. > > The whole think gets kind of complicated fast. The idea is to use the > filtering > > mechanism of mandatory constraints and optional constraints against the > "native > > resolution" first and then crop if requested. It feels a bit scary to treat > > maxWidht from the mandatory set and maxHeight from the optional set. I feels > > like a higher risk of breaking the expected aspect ratio. Therefore I decided > to > > simply ignore the optional if a mandatory constraint has been given. That is > at > > least not wrong since optional is optional. > > OK that's probably the safe thing to do. > > Btw, you could reduce code duplication in this function by having a utility > function at the top of this file: > > bool GetConstraintValue(const blink::WebMediaConstraints& constraints, > bool mandatory, const char* name, int* value) { > blink::WebString value_str; > bool ret = mandatory ? > constraints.getMandatoryConstraintValue(name, value_str) : > constraints.getOptionalConstraintValue(name, value_str); > if (ret) > *value = base::StringToInt(value.utf8(), value); > return ret; > } > > > and then here the code will be a little easier to follow: > > bool mandatory = GetConstraintValue(constraints, true, > MediaStreamVideoSource::kMaxWidth, desired_width); > mandatory |= GetConstraintValue(constraints, true, > MediaStreamVideoSource::kMaxHeight, desired_height); > if (mandatory) > return; > > GetConstraintValue(constraints, false, MediaStreamVideoSource::kMaxWidth, > desired_width); > GetConstraintValue(constraints, false, MediaStreamVideoSource::kMaxHeight, > desired_height); > https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:268: // in MediaStreamVideoSource::DeliverVideoFrame if cropping is needed. On 2014/03/06 13:28:50, tommi wrote: > On 2014/03/06 12:45:42, perkj wrote: > > On 2014/03/06 10:14:09, tommi wrote: > > > I don't understand this actually :) > > > The method doesn't do anything, so how can it be used for keeping a > reference? > > > > See base::Bind(&ReleaseOriginalFrame, frame)) further down. Do you have a > better > > idea how do do frame->AddRef and frame->Release() bounded to callback? I asked > > Joi and Alpha. > > Ah, I missed that. Can you add something to the comments that explain that we > keep the reference in the closure that eventually calls this method? Done. https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... File content/renderer/media/media_stream_video_source_unittest.cc (right): https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... content/renderer/media/media_stream_video_source_unittest.cc:138: int extpected_h, On 2014/03/06 13:28:50, tommi wrote: > On 2014/03/06 12:45:42, perkj wrote: > > On 2014/03/06 10:14:09, tommi wrote: > > > expected_h > > > (or even expected_height) > > > > Done. > > missed extpected -> expected Done. https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... File content/renderer/media/webrtc/webrtc_video_capturer_adapter_unittest.cc (right): https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... content/renderer/media/webrtc/webrtc_video_capturer_adapter_unittest.cc:14: : public sigslot::has_slots<>, On 2014/03/06 13:28:50, tommi wrote: > On 2014/03/06 12:45:42, perkj wrote: > > On 2014/03/06 10:14:09, tommi wrote: > > > this code is in chrome... I don't think we want to start bringing in > > has_slots. > > > > > > use base::Callback (aka Closure) instead > > > > This tests the wrapper that implements a libjingle cricket::VideoCapturer. > > To test that the video frame we actually provide to libjingle is what we > expect > > we need this. Ie, its needed in order to connect to > > adapter_.SignalFrameCaptured > > Note that there is another test (not by me :->) > > video_destination_handler_unittest.cc that do the same thing for the same > > reason. > > Ah, I see. I thought WebRtcVideoCapturerAdapter was a pure Chrome type, but it > appears to be a hybrid. OK, as is then. Done. https://codereview.chromium.org/183113004/diff/170001/content/renderer/media/... File content/renderer/media/media_stream_video_source.cc (right): https://codereview.chromium.org/183113004/diff/170001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:41: // kMaxCropFactor. Ie - if a constraint it set to maxHeight 360, an original On 2014/03/06 13:28:50, tommi wrote: > it->is Done. https://codereview.chromium.org/183113004/diff/170001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:42: // input frame height of max 360 * kMaxCropFactor pixels is accepted. On 2014/03/06 13:28:50, tommi wrote: > ah, I see, so this is a limitation of each of the axis. Done. https://codereview.chromium.org/183113004/diff/170001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:243: // many cameras has lower frame rate and have more noise in the image at On 2014/03/06 13:28:50, tommi wrote: > cameras has -> cameras have Done. https://codereview.chromium.org/183113004/diff/170001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:244: // its maximum supported resolution. On 2014/03/06 13:28:50, tommi wrote: > its->their Done.
On 2014/03/06 14:22:53, perkj wrote: > PTAL > > https://codereview.chromium.org/183113004/diff/160001/content/browser/media/w... > File content/browser/media/webrtc_getusermedia_browsertest.cc (right): > > https://codereview.chromium.org/183113004/diff/160001/content/browser/media/w... > content/browser/media/webrtc_getusermedia_browsertest.cc:398: // frame don't > have the same top left pixel as the original frame. > On 2014/03/06 13:28:50, tommi wrote: > > On 2014/03/06 12:45:42, perkj wrote: > > > On 2014/03/06 10:14:09, tommi wrote: > > > > don't -> doesn't > > > > nit: pixel -> coordinates > > > > > > Done. > > > > missed fixing the "don't vs doesn't typo" :) > > Sorry about that. Strange that I do the same grammatical errors over and over > again..... > > https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... > File content/renderer/media/media_stream_video_source.cc (right): > > https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... > content/renderer/media/media_stream_video_source.cc:195: int* dw, int* dh) { > On 2014/03/06 13:28:50, tommi wrote: > > On 2014/03/06 12:45:42, perkj wrote: > > > On 2014/03/06 10:14:09, tommi wrote: > > > > full variable names > > > > > > Done. > > > > Thanks... at first I thought the 'd' stood for delta. So this a good change. > > Done. > > https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... > content/renderer/media/media_stream_video_source.cc:209: if (mandatory_found) > For some reason I get a compile error with const char* name. > but not if I do const blink::WebString& name. I don't understand why it matters > where the conversion between WebString and const char * is done but.... > > On 2014/03/06 13:28:50, tommi wrote: > > On 2014/03/06 12:45:42, perkj wrote: > > > On 2014/03/06 10:14:09, tommi wrote: > > > > What if height is specified as mandatory and width as optional - is there > > any > > > > reason we would not want to pick up the width? > > > > > > > > I actually don't see much value in the 'mandatory_found' variable in this > > > > function or separate height and width strings. > > > > Why wouldn't we simply do this: > > > > > > > > blink::WebString web_string; > > > > if (constraints.getMandatoryConstraintValue( > > > > MediaStreamVideoSource::kMaxWidth, web_string) || > > > > constraints.getOptionalConstraintValue( > > > > MediaStreamVideoSource::kMaxWidth, web_string)) { > > > > base::StringToInt(web_string.utf8(), dw); > > > > } > > > > > > > > if (constraints.getMandatoryConstraintValue( > > > > MediaStreamVideoSource::kMaxHeight, web_string) || > > > > constraints.getOptionalConstraintValue( > > > > MediaStreamVideoSource::kMaxHeight, web_string)) { > > > > base::StringToInt(web_string.utf8(), dh); > > > > } > > > > > > I did that first. > > > The whole think gets kind of complicated fast. The idea is to use the > > filtering > > > mechanism of mandatory constraints and optional constraints against the > > "native > > > resolution" first and then crop if requested. It feels a bit scary to treat > > > maxWidht from the mandatory set and maxHeight from the optional set. I feels > > > like a higher risk of breaking the expected aspect ratio. Therefore I > decided > > to > > > simply ignore the optional if a mandatory constraint has been given. That > is > > at > > > least not wrong since optional is optional. > > > > OK that's probably the safe thing to do. > > > > Btw, you could reduce code duplication in this function by having a utility > > function at the top of this file: > > > > bool GetConstraintValue(const blink::WebMediaConstraints& constraints, > > bool mandatory, const char* name, int* value) { > > blink::WebString value_str; > > bool ret = mandatory ? > > constraints.getMandatoryConstraintValue(name, value_str) : > > constraints.getOptionalConstraintValue(name, value_str); > > if (ret) > > *value = base::StringToInt(value.utf8(), value); > > return ret; > > } > > > > > > and then here the code will be a little easier to follow: > > > > bool mandatory = GetConstraintValue(constraints, true, > > MediaStreamVideoSource::kMaxWidth, desired_width); > > mandatory |= GetConstraintValue(constraints, true, > > MediaStreamVideoSource::kMaxHeight, desired_height); > > if (mandatory) > > return; > > > > GetConstraintValue(constraints, false, MediaStreamVideoSource::kMaxWidth, > > desired_width); > > GetConstraintValue(constraints, false, MediaStreamVideoSource::kMaxHeight, > > desired_height); > > > > https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... > content/renderer/media/media_stream_video_source.cc:268: // in > MediaStreamVideoSource::DeliverVideoFrame if cropping is needed. > On 2014/03/06 13:28:50, tommi wrote: > > On 2014/03/06 12:45:42, perkj wrote: > > > On 2014/03/06 10:14:09, tommi wrote: > > > > I don't understand this actually :) > > > > The method doesn't do anything, so how can it be used for keeping a > > reference? > > > > > > See base::Bind(&ReleaseOriginalFrame, frame)) further down. Do you have a > > better > > > idea how do do frame->AddRef and frame->Release() bounded to callback? I > asked > > > Joi and Alpha. > > > > Ah, I missed that. Can you add something to the comments that explain that we > > keep the reference in the closure that eventually calls this method? > > Done. > > https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... > File content/renderer/media/media_stream_video_source_unittest.cc (right): > > https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... > content/renderer/media/media_stream_video_source_unittest.cc:138: int > extpected_h, > On 2014/03/06 13:28:50, tommi wrote: > > On 2014/03/06 12:45:42, perkj wrote: > > > On 2014/03/06 10:14:09, tommi wrote: > > > > expected_h > > > > (or even expected_height) > > > > > > Done. > > > > missed extpected -> expected > > Done. > > https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... > File content/renderer/media/webrtc/webrtc_video_capturer_adapter_unittest.cc > (right): > > https://codereview.chromium.org/183113004/diff/160001/content/renderer/media/... > content/renderer/media/webrtc/webrtc_video_capturer_adapter_unittest.cc:14: : > public sigslot::has_slots<>, > On 2014/03/06 13:28:50, tommi wrote: > > On 2014/03/06 12:45:42, perkj wrote: > > > On 2014/03/06 10:14:09, tommi wrote: > > > > this code is in chrome... I don't think we want to start bringing in > > > has_slots. > > > > > > > > use base::Callback (aka Closure) instead > > > > > > This tests the wrapper that implements a libjingle cricket::VideoCapturer. > > > To test that the video frame we actually provide to libjingle is what we > > expect > > > we need this. Ie, its needed in order to connect to > > > adapter_.SignalFrameCaptured > > > Note that there is another test (not by me :->) > > > video_destination_handler_unittest.cc that do the same thing for the same > > > reason. > > > > Ah, I see. I thought WebRtcVideoCapturerAdapter was a pure Chrome type, but > it > > appears to be a hybrid. OK, as is then. > > Done. > > https://codereview.chromium.org/183113004/diff/170001/content/renderer/media/... > File content/renderer/media/media_stream_video_source.cc (right): > > https://codereview.chromium.org/183113004/diff/170001/content/renderer/media/... > content/renderer/media/media_stream_video_source.cc:41: // kMaxCropFactor. Ie - > if a constraint it set to maxHeight 360, an original > On 2014/03/06 13:28:50, tommi wrote: > > it->is > > Done. > > https://codereview.chromium.org/183113004/diff/170001/content/renderer/media/... > content/renderer/media/media_stream_video_source.cc:42: // input frame height of > max 360 * kMaxCropFactor pixels is accepted. > On 2014/03/06 13:28:50, tommi wrote: > > ah, I see, so this is a limitation of each of the axis. > > Done. > > https://codereview.chromium.org/183113004/diff/170001/content/renderer/media/... > content/renderer/media/media_stream_video_source.cc:243: // many cameras has > lower frame rate and have more noise in the image at > On 2014/03/06 13:28:50, tommi wrote: > > cameras has -> cameras have > > Done. > > https://codereview.chromium.org/183113004/diff/170001/content/renderer/media/... > content/renderer/media/media_stream_video_source.cc:244: // its maximum > supported resolution. > On 2014/03/06 13:28:50, tommi wrote: > > its->their > > Done. or actually - wait a second....
ok- problem solved. PTAL
lgtm https://codereview.chromium.org/183113004/diff/210001/content/renderer/media/... File content/renderer/media/media_stream_video_source.cc (right): https://codereview.chromium.org/183113004/diff/210001/content/renderer/media/... content/renderer/media/media_stream_video_source.cc:202: base::StringToInt(value_str.utf8(), value); ah, that was my mistake I guess ;)
The CQ bit was checked by perkj@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/perkj@chromium.org/183113004/220002
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) telemetry_perf_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
LGTM for content/renderer/media/webrtc/DEPS
Message was sent while issue was closed.
Committed patchset #12 manually as r255573 (presubmit successful). |
