Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
17043007:
Fix the WebRTC color bug. (Closed)
|
Created:
7 years, 6 months ago by hkuang Modified:
7 years, 6 months ago CC:
-reviews_chromium.org, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionFix the WebRTC color bug.
On Android, pixel layout is RGBA. However, other Chrome platforms use BGRA.
BUG=239276
TEST= Visit inear.se/bgra on nexus 7. Color is right.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=207472
Patch Set 1 : Fix the WebRTC color bug. #
Total comments: 8
Patch Set 2 : Addressing Andrew's comments. #
Total comments: 2
Patch Set 3 : Fix unit test error on Android. #
Total comments: 6
Patch Set 4 : Addressing Andrew's comments. #
Total comments: 4
Patch Set 5 : Addressing Andrew's comments. #
Messages
Total messages: 24 (0 generated)
PTAL.
https://codereview.chromium.org/17043007/diff/1001/media/base/simd/convert_yu... File media/base/simd/convert_yuv_to_rgb_c.cc (right): https://codereview.chromium.org/17043007/diff/1001/media/base/simd/convert_yu... media/base/simd/convert_yuv_to_rgb_c.cc:30: static inline void ConvertYUVToRGB32_C(uint8 y, OOC does this mean we're using C routines for doing colour conversion on Android?
https://codereview.chromium.org/17043007/diff/1001/media/base/simd/convert_yu... File media/base/simd/convert_yuv_to_rgb_c.cc (right): https://codereview.chromium.org/17043007/diff/1001/media/base/simd/convert_yu... media/base/simd/convert_yuv_to_rgb_c.cc:30: static inline void ConvertYUVToRGB32_C(uint8 y, Yes. It uses this routine. Not quite efficient, but I could not find any neon function in this folder for android. The reason is mainly because the painting path for webmediaplayer_ms is the same as webmediaplayer_impl on Android. On 2013/06/14 21:17:52, scherkus wrote: > OOC does this mean we're using C routines for doing colour conversion on > Android?
https://codereview.chromium.org/17043007/diff/1001/media/base/simd/convert_yu... File media/base/simd/convert_yuv_to_rgb_c.cc (right): https://codereview.chromium.org/17043007/diff/1001/media/base/simd/convert_yu... media/base/simd/convert_yuv_to_rgb_c.cc:19: #define SK_A32_SHIFT 24 nit: can you reorder these so that it follows the pixel layout e.g., RGBA SK_R32_SHIFT SK_G32_SHIFT SK_B32_SHIFT SK_A32_SHIFT https://codereview.chromium.org/17043007/diff/1001/media/base/simd/convert_yu... media/base/simd/convert_yuv_to_rgb_c.cc:30: static inline void ConvertYUVToRGB32_C(uint8 y, On 2013/06/14 21:28:35, hkuang1 wrote: > Yes. It uses this routine. Not quite efficient, but I could not find any neon > function in this folder for android. The reason is mainly because the painting > path for webmediaplayer_ms is the same as webmediaplayer_impl on Android. I see. Have you looked at whether libyuv has a neon optimized routine?
lgtm
https://codereview.chromium.org/17043007/diff/1001/media/base/simd/convert_yu... File media/base/simd/convert_yuv_to_rgb_c.cc (right): https://codereview.chromium.org/17043007/diff/1001/media/base/simd/convert_yu... media/base/simd/convert_yuv_to_rgb_c.cc:30: static inline void ConvertYUVToRGB32_C(uint8 y, I did not check it yet. But I thought I could not use the function in libyuv here as the YUV2RGB conversion does not use the libyuv function. Is there any reason for that? On 2013/06/14 21:32:42, scherkus wrote: > On 2013/06/14 21:28:35, hkuang1 wrote: > > Yes. It uses this routine. Not quite efficient, but I could not find any neon > > function in this folder for android. The reason is mainly because the > painting > > path for webmediaplayer_ms is the same as webmediaplayer_impl on Android. > > I see. Have you looked at whether libyuv has a neon optimized routine?
https://codereview.chromium.org/17043007/diff/1001/media/base/simd/convert_yu... File media/base/simd/convert_yuv_to_rgb_c.cc (right): https://codereview.chromium.org/17043007/diff/1001/media/base/simd/convert_yu... media/base/simd/convert_yuv_to_rgb_c.cc:30: static inline void ConvertYUVToRGB32_C(uint8 y, I did not check it yet. But I thought I could not use the function in libyuv here as the YUV2RGB conversion does not use the libyuv function. Is there any reason for that? On 2013/06/14 21:32:42, scherkus wrote: > On 2013/06/14 21:28:35, hkuang1 wrote: > > Yes. It uses this routine. Not quite efficient, but I could not find any neon > > function in this folder for android. The reason is mainly because the > painting > > path for webmediaplayer_ms is the same as webmediaplayer_impl on Android. > > I see. Have you looked at whether libyuv has a neon optimized routine?
m https://codereview.chromium.org/17043007/diff/1001/media/base/simd/convert_yu... File media/base/simd/convert_yuv_to_rgb_c.cc (right): https://codereview.chromium.org/17043007/diff/1001/media/base/simd/convert_yu... media/base/simd/convert_yuv_to_rgb_c.cc:30: static inline void ConvertYUVToRGB32_C(uint8 y, On 2013/06/14 21:50:58, hkuang1 wrote: > I did not check it yet. But I thought I could not use the function in libyuv > here as the YUV2RGB conversion does not use the libyuv function. Is there any > reason for that? libyuv is a separate collection of yuv routines instead I would consider making the change higher up in the stack instead of adding libyuv code here
https://codereview.chromium.org/17043007/diff/1001/media/base/simd/convert_yu... File media/base/simd/convert_yuv_to_rgb_c.cc (right): https://codereview.chromium.org/17043007/diff/1001/media/base/simd/convert_yu... media/base/simd/convert_yuv_to_rgb_c.cc:30: static inline void ConvertYUVToRGB32_C(uint8 y, On 2013/06/14 22:17:30, scherkus wrote: > On 2013/06/14 21:50:58, hkuang1 wrote: > > I did not check it yet. But I thought I could not use the function in libyuv > > here as the YUV2RGB conversion does not use the libyuv function. Is there any > > reason for that? > > libyuv is a separate collection of yuv routines > > instead I would consider making the change higher up in the stack instead of > adding libyuv code here That is fine, but I think that is a separate issue. I think this should be fixed here in this CL. And then open up a bug to change webrtc to use libyuv.
On 2013/06/14 23:07:18, fgalligan1 wrote: > https://codereview.chromium.org/17043007/diff/1001/media/base/simd/convert_yu... > File media/base/simd/convert_yuv_to_rgb_c.cc (right): > > https://codereview.chromium.org/17043007/diff/1001/media/base/simd/convert_yu... > media/base/simd/convert_yuv_to_rgb_c.cc:30: static inline void > ConvertYUVToRGB32_C(uint8 y, > On 2013/06/14 22:17:30, scherkus wrote: > > On 2013/06/14 21:50:58, hkuang1 wrote: > > > I did not check it yet. But I thought I could not use the function in libyuv > > > here as the YUV2RGB conversion does not use the libyuv function. Is there > any > > > reason for that? > > > > libyuv is a separate collection of yuv routines > > > > instead I would consider making the change higher up in the stack instead of > > adding libyuv code here > > That is fine, but I think that is a separate issue. I think this should be fixed > here in this CL. And then open up a bug to change webrtc to use libyuv. SGTM
https://codereview.chromium.org/17043007/diff/1001/media/base/simd/convert_yu... File media/base/simd/convert_yuv_to_rgb_c.cc (right): https://codereview.chromium.org/17043007/diff/1001/media/base/simd/convert_yu... media/base/simd/convert_yuv_to_rgb_c.cc:19: #define SK_A32_SHIFT 24 On 2013/06/14 21:32:42, scherkus wrote: > nit: can you reorder these so that it follows the pixel layout > > e.g., RGBA > SK_R32_SHIFT > SK_G32_SHIFT > SK_B32_SHIFT > SK_A32_SHIFT Done.
lgtm w/ nits https://codereview.chromium.org/17043007/diff/10001/media/base/simd/convert_y... File media/base/simd/convert_yuv_to_rgb_c.cc (right): https://codereview.chromium.org/17043007/diff/10001/media/base/simd/convert_y... media/base/simd/convert_yuv_to_rgb_c.cc:19: // due to performance issue(crbug/249980). nit: can you better format this comment? for example ... try to use the full 80 character limit we have links should also be full urls and clickable e.g., http://crbug.com/249980
https://codereview.chromium.org/17043007/diff/10001/media/base/simd/convert_y... File media/base/simd/convert_yuv_to_rgb_c.cc (right): https://codereview.chromium.org/17043007/diff/10001/media/base/simd/convert_y... media/base/simd/convert_yuv_to_rgb_c.cc:19: // due to performance issue(crbug/249980). On 2013/06/15 00:31:56, scherkus wrote: > nit: can you better format this comment? > > for example ... try to use the full 80 character limit we have > > links should also be full urls and clickable > > e.g., http://crbug.com/249980 Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hkuang@chromium.org/17043007/14001
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
https://codereview.chromium.org/17043007/diff/54011/media/base/simd/convert_r... File media/base/simd/convert_rgb_to_yuv_c.cc (right): https://codereview.chromium.org/17043007/diff/54011/media/base/simd/convert_r... media/base/simd/convert_rgb_to_yuv_c.cc:32: yplane[j] = clip_byte(((pixel[0] * 66 + pixel[1] * 129 + this is mostly duplicated -- can you instead do something like the following? #if defined(OS_ANDROID) const int r = 2; const int g = 1; const int b = 0; #else const int r = 0; const int g = 1; const int b = 2; #endif ... then referring to pixel with pixel[r], pixel[g], etc... https://codereview.chromium.org/17043007/diff/54011/media/base/yuv_convert_un... File media/base/yuv_convert_unittest.cc (right): https://codereview.chromium.org/17043007/diff/54011/media/base/yuv_convert_un... media/base/yuv_convert_unittest.cc:104: EXPECT_EQ(815021082u, rgb_hash); what about writing and using a BGRA->RGBA swizzler so that we don't need to maintain two sets of hashes? https://codereview.chromium.org/17043007/diff/54011/media/base/yuv_convert_un... media/base/yuv_convert_unittest.cc:358: EXPECT_EQ(320824432u, rgb_hash); these values are the same
https://codereview.chromium.org/17043007/diff/54011/media/base/simd/convert_r... File media/base/simd/convert_rgb_to_yuv_c.cc (right): https://codereview.chromium.org/17043007/diff/54011/media/base/simd/convert_r... media/base/simd/convert_rgb_to_yuv_c.cc:32: yplane[j] = clip_byte(((pixel[0] * 66 + pixel[1] * 129 + On 2013/06/19 19:49:50, scherkus wrote: > this is mostly duplicated -- can you instead do something like the following? > > #if defined(OS_ANDROID) > const int r = 2; > const int g = 1; > const int b = 0; > #else > const int r = 0; > const int g = 1; > const int b = 2; > #endif > > ... then referring to pixel with pixel[r], pixel[g], etc... Done. https://codereview.chromium.org/17043007/diff/54011/media/base/yuv_convert_un... File media/base/yuv_convert_unittest.cc (right): https://codereview.chromium.org/17043007/diff/54011/media/base/yuv_convert_un... media/base/yuv_convert_unittest.cc:104: EXPECT_EQ(815021082u, rgb_hash); On 2013/06/19 19:49:50, scherkus wrote: > what about writing and using a BGRA->RGBA swizzler so that we don't need to > maintain two sets of hashes? Done. Wrote a helper function to convert BGRA->RGBA every time before calculating hash. https://codereview.chromium.org/17043007/diff/54011/media/base/yuv_convert_un... media/base/yuv_convert_unittest.cc:358: EXPECT_EQ(320824432u, rgb_hash); On 2013/06/19 19:49:50, scherkus wrote: > these values are the same A mistake. Delete it.
https://codereview.chromium.org/17043007/diff/54011/media/base/simd/convert_r... File media/base/simd/convert_rgb_to_yuv_c.cc (right): https://codereview.chromium.org/17043007/diff/54011/media/base/simd/convert_r... media/base/simd/convert_rgb_to_yuv_c.cc:32: yplane[j] = clip_byte(((pixel[0] * 66 + pixel[1] * 129 + On 2013/06/19 19:49:50, scherkus wrote: > this is mostly duplicated -- can you instead do something like the following? > > #if defined(OS_ANDROID) > const int r = 2; > const int g = 1; > const int b = 0; > #else > const int r = 0; > const int g = 1; > const int b = 2; > #endif > > ... then referring to pixel with pixel[r], pixel[g], etc... Done. https://codereview.chromium.org/17043007/diff/54011/media/base/yuv_convert_un... File media/base/yuv_convert_unittest.cc (right): https://codereview.chromium.org/17043007/diff/54011/media/base/yuv_convert_un... media/base/yuv_convert_unittest.cc:104: EXPECT_EQ(815021082u, rgb_hash); On 2013/06/19 19:49:50, scherkus wrote: > what about writing and using a BGRA->RGBA swizzler so that we don't need to > maintain two sets of hashes? Done. Wrote a helper function to convert BGRA->RGBA every time before calculating hash. https://codereview.chromium.org/17043007/diff/54011/media/base/yuv_convert_un... media/base/yuv_convert_unittest.cc:358: EXPECT_EQ(320824432u, rgb_hash); On 2013/06/19 19:49:50, scherkus wrote: > these values are the same A mistake. Delete it.
lgtm w/ nits thank you so much for fixing this and the tests! feel free to loop me in on discussions with respect to libyuv https://codereview.chromium.org/17043007/diff/56004/media/base/yuv_convert_un... File media/base/yuv_convert_unittest.cc (right): https://codereview.chromium.org/17043007/diff/56004/media/base/yuv_convert_un... media/base/yuv_convert_unittest.cc:80: static void ConvertBGRAToRGBA(unsigned char* pixels, size_t buffer_size) { wait ... isn't this backwards? your comment in media/base/simd/convert_yuv_to_rgb_c.cc says that desktop is BGRA and Android is RGBA regardless, I suggest sidestepping the issue by renaming this function "SwapRedAndBlueChannels" since it can be used to convert to AND from BGRA<->RGBA ;) https://codereview.chromium.org/17043007/diff/56004/media/base/yuv_convert_un... media/base/yuv_convert_unittest.cc:355: remove extra line that snuck in
https://codereview.chromium.org/17043007/diff/56004/media/base/yuv_convert_un... File media/base/yuv_convert_unittest.cc (right): https://codereview.chromium.org/17043007/diff/56004/media/base/yuv_convert_un... media/base/yuv_convert_unittest.cc:80: static void ConvertBGRAToRGBA(unsigned char* pixels, size_t buffer_size) { On 2013/06/20 00:28:08, scherkus wrote: > wait ... isn't this backwards? > > your comment in media/base/simd/convert_yuv_to_rgb_c.cc says that desktop is > BGRA and Android is RGBA > > regardless, I suggest sidestepping the issue by renaming this function > "SwapRedAndBlueChannels" since it can be used to convert to AND from BGRA<->RGBA > ;) Yes, my bad. Should be RGBA to BGRA. "SwapRedAndBlueChannels" is a better name. Change the name to be SwapRedAndBlueChannels https://codereview.chromium.org/17043007/diff/56004/media/base/yuv_convert_un... media/base/yuv_convert_unittest.cc:355: On 2013/06/20 00:28:08, scherkus wrote: > remove extra line that snuck in Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hkuang@chromium.org/17043007/63001
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_rel. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hkuang@chromium.org/17043007/78001
Message was sent while issue was closed.
Change committed as 207472 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
