Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23870002:
Video Capture: Use libyuv for color conversion in VideoCaptureController. (Closed)
|
Created:
7 years, 3 months ago by mcasas Modified:
7 years, 3 months ago CC:
Jói, chromium-reviews, joi+watch-content_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, jam, miu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionVideo Capture: Use libyuv for color conversion in VideoCaptureController.
Using libyuv for all platforms except Android and iOS. The implementation of OnIncomingCapturedFrame is #ifdef'ed.
BUG=282282
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221814
Patch Set 1 #Patch Set 2 : Special RGB treatment for Windows platforms #
Total comments: 11
Patch Set 3 : wjia comments addressed #
Total comments: 4
Patch Set 4 : Rotation first, flip second #Patch Set 5 : kRGB24 treated separately for OS_WIN platform #Patch Set 6 : kRGB24-OS_WIN case treated more cleanly after consultation with the Ancient Masters #
Total comments: 2
Patch Set 7 : Rewritten kRGB24-OS_WIN #Patch Set 8 : DCHECK for no rotation no flips in kRGB24+OS_WIN #Messages
Total messages: 17 (0 generated)
Hi tommi - could you PTAL at this CL? Thanks!
Wei - I think you're better qualified at reviewing this than I :)
https://codereview.chromium.org/23870002/diff/9001/content/browser/renderer_h... File content/browser/renderer_host/media/video_capture_controller.cc (right): https://codereview.chromium.org/23870002/diff/9001/content/browser/renderer_h... content/browser/renderer_host/media/video_capture_controller.cc:263: #if !defined(OS_IOS) && !defined(OS_ANDROID) Is it possible to add color conversion for Android/IOS in libyuv? That would reduce lots of dupped code. https://codereview.chromium.org/23870002/diff/9001/content/browser/renderer_h... content/browser/renderer_host/media/video_capture_controller.cc:270: bool flip_horiz) { Do we need to support rotation for desktop platforms? If not, this function can be simplified further. FYI, Rotation was added when Android code was checked in. https://codereview.chromium.org/23870002/diff/9001/content/browser/renderer_h... content/browser/renderer_host/media/video_capture_controller.cc:283: frame_info_.height), It's more readable to drop "gfx::Size" to a new line, e.g., dst = buffer_pool_->ReserveI420VideoFrame( gfx::Size(frame_info_.width, frame_info_.height), rotation); https://codereview.chromium.org/23870002/diff/9001/content/browser/renderer_h... content/browser/renderer_host/media/video_capture_controller.cc:305: rotation_mode = libyuv::kRotate270; Is this mapping correct? How do you test it? flip_vert and flip_horiz are not used essentially in this function. That could mean something is missing. https://codereview.chromium.org/23870002/diff/9001/content/browser/renderer_h... content/browser/renderer_host/media/video_capture_controller.cc:361: libyuv::ConvertToI420(data, Do you need to call this for kRGB24 on WIN (it has been converted above)? It doesn't seem to be the case.
Removed DCHECK. Renamed destination_colorspace to origin_colorspace. Added code to consolidate rotation and both flips into one rotation and one flip
Addressed wjia's comments. https://codereview.chromium.org/23870002/diff/9001/content/browser/renderer_h... File content/browser/renderer_host/media/video_capture_controller.cc (right): https://codereview.chromium.org/23870002/diff/9001/content/browser/renderer_h... content/browser/renderer_host/media/video_capture_controller.cc:263: #if !defined(OS_IOS) && !defined(OS_ANDROID) That'll be cool indeed, I can follow through with frank and magnus to see what's the intended future on it. But is not available yet for sure. https://codereview.chromium.org/23870002/diff/9001/content/browser/renderer_h... content/browser/renderer_host/media/video_capture_controller.cc:270: bool flip_horiz) { I don't know. But why would we change this generic signature? In that case it might be better to have two functions, OnIncomingCaptureFrame and OnIncomingCaptureFrameExtended, what about that? If you ask me, I'd keep it as it was supported before this CL. We can tackle the eventual modification later on. https://codereview.chromium.org/23870002/diff/9001/content/browser/renderer_h... content/browser/renderer_host/media/video_capture_controller.cc:283: frame_info_.height), Indeed. Done. https://codereview.chromium.org/23870002/diff/9001/content/browser/renderer_h... content/browser/renderer_host/media/video_capture_controller.cc:297: libyuv::FourCC destination_colorspace = libyuv::FOURCC_ANY; Actually I made a mistake here, this variable should be called "origin_colorspace" :) https://codereview.chromium.org/23870002/diff/9001/content/browser/renderer_h... content/browser/renderer_host/media/video_capture_controller.cc:305: rotation_mode = libyuv::kRotate270; I missed two things indeed: 1) using horizontal flip to further calculate rotation_mode 2) using vertical_flip in the call to libyuv::ConverToI420, to change the sign of src_height (ConvertToI420 supports vertical flip). When both are consolidated, and assuming flips happen before rotation (as in libyuv), we can consolidate all three numbers into two: new_rotation = rotation + [180 * horizontal_flip] new_vertical_flip = XOR(horizontal_flip, vertical_flip) where [] means modulo 360. I tested it by forcing rotations and now flips in code. https://codereview.chromium.org/23870002/diff/9001/content/browser/renderer_h... content/browser/renderer_host/media/video_capture_controller.cc:361: libyuv::ConvertToI420(data, It would be called with a destination_colorspace = libyuv::FOURCC_ANY, so the call would do no colorspace conversion but it is still needed for rotation, which is not done in media::ConvertRGB24ToYUV, admitting that rotation is still needed/desirable.
https://codereview.chromium.org/23870002/diff/15001/content/browser/renderer_... File content/browser/renderer_host/media/video_capture_controller.cc (right): https://codereview.chromium.org/23870002/diff/15001/content/browser/renderer_... content/browser/renderer_host/media/video_capture_controller.cc:293: // Assuming flips happen before rotations (as in libyuv), we consolidate both I should have added some comments for this function. Actually, flips happen after rotation, at least for RotatePackedYV12Frame(). https://codereview.chromium.org/23870002/diff/15001/content/browser/renderer_... content/browser/renderer_host/media/video_capture_controller.cc:360: libyuv::ConvertToI420(data, When origin_colorspace = libyuv::FOURCC_ANY, does this function do anything to data (if color conversion is not done)? Basically, I want to make sure it's correct for kRGB24 on WIN, since color conversion is done with media::ConvertRGB24ToYUV(), but rotation should be done by this libyuv::ConvertToI420().
Addressed second round of wjia's comments. Thanks for the quick turnover! https://codereview.chromium.org/23870002/diff/15001/content/browser/renderer_... File content/browser/renderer_host/media/video_capture_controller.cc (right): https://codereview.chromium.org/23870002/diff/15001/content/browser/renderer_... content/browser/renderer_host/media/video_capture_controller.cc:293: // Assuming flips happen before rotations (as in libyuv), we consolidate both In that case the equations and the comment should be rewritten, but they happen to be almost the same, just changing new_rotation = (rotation + 180 * horizontal_flip) modulo 360 to new_rotation = (rotation + 180 * vertical_flip) modulo 360 https://codereview.chromium.org/23870002/diff/15001/content/browser/renderer_... content/browser/renderer_host/media/video_capture_controller.cc:360: libyuv::ConvertToI420(data, When FOURCC_ANY is used as origin colorspace, a rotation is performed if needed, but no colorspace conversion is performed (see the lines around [1]). Rotation is not done in place (except for YUV easy modes, see [2]), though, so a buffer is reallocated and free'd. [1] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libyuv... [2] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libyuv...
Addressed wjia@'s offline comments about kRGB24 color conversion in OS_WIN case. Thanks for your time! After local consultation with the Ancient Masters, and to ease the review, the block after the colorspace switch-case looks like #ifdef OS_WIN if colorspace is kRGB24 media:: colorspace conversion #else if FALSE #endif else libyuv colorspace conversion and rotation Note that for kRGB24-OS_WIN, rotation and/or flipping is not supported, same as before this CL.
With kRGB24 on WIN fixed, we are almost there. One more comment. Please make sure the mapping of ration and flips from chrome to libyuv is correct. https://codereview.chromium.org/23870002/diff/35001/content/browser/renderer_... File content/browser/renderer_host/media/video_capture_controller.cc (right): https://codereview.chromium.org/23870002/diff/35001/content/browser/renderer_... content/browser/renderer_host/media/video_capture_controller.cc:359: else { "if (false)" looks a bit weird. How about the following: int need_convert_rgb24_on_win = false; #if defined(OS_WIN) // kRGB24 on Windows start at the bottom line and has a negative stride. This // is not supported by libyuv, so the media API is used instead. // Rotation and flipping is not supported in this case. if (frame_info_.color == media::VideoCaptureCapability::kRGB24) need_convert_rgb24_on_win = true; #endif if (need_convert_rgb24_on_win) { int rgb_stride = -3 * (frame_info_.width + chopped_width_); const uint8* rgb_src = data + 3 * (frame_info_.width + chopped_width_) * (frame_info_.height - 1 + chopped_height_); media::ConvertRGB24ToYUV(rgb_src, yplane, uplane, vplane, frame_info_.width, frame_info_.height, rgb_stride, yplane_stride, uv_plane_stride); } else {
Hi wjia@, I took your suggestion in, thanks. About rotation and flipping, for the platforms using them (android and ios), this CL does not change the usage. Just for the record, I made a test with libyuv in Android and rotations looked correct to me. https://codereview.chromium.org/23870002/diff/35001/content/browser/renderer_... File content/browser/renderer_host/media/video_capture_controller.cc (right): https://codereview.chromium.org/23870002/diff/35001/content/browser/renderer_... content/browser/renderer_host/media/video_capture_controller.cc:359: else { Yes I also thought about adding a flag for this particular case, but preferred wrapping media::ConvertRGB24ToYUV in an #if..#endif. But this solution is also ok since anyway media video_utils is linked in. So: lgtm ! :) Done.
lgtm with comment: please add comment for the function indicating that "rotation for rgb24 on WIN is not supported". A DCHECK is also ok.
Changed comment to: // Rotation and flipping is not supported in kRGB24 and OS_WIN case. And added a DCHECK inside the if (kRGB24) when OS_WIN is defined: DCHECK(!rotation && !flip_vert && !flip_horiz);
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mcasas@chromium.org/23870002/53001
Retried try job too often on linux_aura for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mcasas@chromium.org/23870002/53001
Message was sent while issue was closed.
Change committed as 221814 |
