Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
760593003:
Add PIXEL_FORMAT_NV12 into VideoPixelFormat (Closed)
|
Created:
6 years ago by zhaoze.zhou Modified:
6 years ago CC:
chromium-reviews, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, wjia+watch_chromium.org, miu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionAdd PIXEL_FORMAT_NV12 into VideoPixelFormat
This cl add PIXEL_FORMAT_NV12 into video capture type
and add calls in OnIncomingCapturedData() to call
the new API.
This cl also add unit-test to run OnIncomingCapturedData()
with all supported formats.
Committed: https://crrev.com/12ee8f52ccc7cfa8d95c6a0dc16b81fb2db86227
Cr-Commit-Position: refs/heads/master@{#307497}
Patch Set 1 #Patch Set 2 : #
Total comments: 1
Patch Set 3 : #Patch Set 4 : #
Total comments: 7
Patch Set 5 : #
Total comments: 7
Patch Set 6 : #
Total comments: 4
Patch Set 7 : #
Total comments: 3
Patch Set 8 : change test name #
Total comments: 1
Patch Set 9 : Remove un-needed mock framewok #
Messages
Total messages: 32 (13 generated)
zhaoze.zhou@partner.samsung.com changed reviewers: + b.kelemen@samsung.com
zhaoze.zhou@partner.samsung.com changed reviewers: + vrk@chromium.org
vrk@chromium.org: PTAL. thanks :)
mcasas@chromium.org changed reviewers: + mcasas@chromium.org
Drive-by: We add pixel formats on demand, so either link to a BUG where the necessity is explained or bundle in this CL the (a) use of it. I imagine this is for Android, right? Please state that in the CL description.
On 2014/11/26 19:01:23, mcasas wrote: > Drive-by: We add pixel formats on demand We use this pixel format downstream in an embedded environment similar to chromecast (but not chromecast). There has been discussions with Darin about the possibility of upstreaming the entire port but so far the preference expressed has been to keep this port out of the tree, and only upstream enablers like this patch. I understand that this is slightly controversial as it leaves some untested code-path in the tree with no coverage, but the the format itself is a valid format and I hope this is enough of a justification for now. We're also open to add tests somehow to add coverage/usage for this code-path.
On 2014/12/02 23:36:10, lgombos wrote: > On 2014/11/26 19:01:23, mcasas wrote: > > Drive-by: We add pixel formats on demand > > We use this pixel format downstream in an embedded environment similar to > chromecast (but not chromecast). > > There has been discussions with Darin about the possibility of upstreaming the > entire port but so far the preference expressed has been to keep this port out > of the tree, and only upstream enablers like this patch. > > I understand that this is slightly controversial as it leaves some untested > code-path in the tree with no coverage, but the the format itself is a valid > format and I hope this is enough of a justification for now. We're also open to > add tests somehow to add coverage/usage for this code-path. Understood. Perhaps you can add a unittest that checks e.g. that pings OnIncomingCapturedData with each pixel format checking that it's processed somehow ok? A (very) reduced version of [1]. [1] https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...
sl.ostapenko@samsung.com changed reviewers: + sl.ostapenko@samsung.com
https://codereview.chromium.org/760593003/diff/20001/content/browser/renderer... File content/browser/renderer_host/media/video_capture_controller_unittest.cc (right): https://codereview.chromium.org/760593003/diff/20001/content/browser/renderer... content/browser/renderer_host/media/video_capture_controller_unittest.cc:585: device_->OnIncomingCapturedData( I think you have to add other test, not insert code chunks into existing test. This is very unrelated to the ErrorBeforeDeviceCreation test.
On 2014/12/03 18:45:44, ostap wrote: > https://codereview.chromium.org/760593003/diff/20001/content/browser/renderer... > File content/browser/renderer_host/media/video_capture_controller_unittest.cc > (right): > > https://codereview.chromium.org/760593003/diff/20001/content/browser/renderer... > content/browser/renderer_host/media/video_capture_controller_unittest.cc:585: > device_->OnIncomingCapturedData( > I think you have to add other test, not insert code chunks into existing test. > This is very unrelated to the ErrorBeforeDeviceCreation test. Yeah, I meant add a _new_ unittest checking all supported formats in sequence, and that you could take inspiration from [1] for using the test harness, waiting for callback, etc.
Patchset #4 (id:60001) has been deleted
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/760593003/diff/100001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_controller_unittest.cc (right): https://codereview.chromium.org/760593003/diff/100001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller_unittest.cc:635: // Exercise OnIncomingCapturedData() code path of VideoCaptureController Instead of comment please make the name of the test meaningful enough. Something like CaptureAllVideoFormat? https://codereview.chromium.org/760593003/diff/100001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller_unittest.cc:638: for (int index = media::PIXEL_FORMAT_I420; index <= media::PIXEL_FORMAT_MAX; I would jut assign 0 first, it doesn't matter which format is the first. And I think you can use an enum directly as loop variable, no need to go from int to enum. https://codereview.chromium.org/760593003/diff/100001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller_unittest.cc:642: media::VideoCaptureParams session_100; 100? Copy paste programming? https://codereview.chromium.org/760593003/diff/100001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller_unittest.cc:646: gfx::Size capture_resolution(320,240); (320, 240) ^ | SPACE!!! https://codereview.chromium.org/760593003/diff/100001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller_unittest.cc:653: const VideoCaptureControllerID client_a_route_1(0x99); There is only one client, you can just name it client. Please get rid of fancy variable names. https://codereview.chromium.org/760593003/diff/100001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller_unittest.cc:672: (unsigned char*)buffer.get()->data(), C-style cast? should be static_cast
Patchset #5 (id:120001) has been deleted
https://codereview.chromium.org/760593003/diff/100001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_controller_unittest.cc (right): https://codereview.chromium.org/760593003/diff/100001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller_unittest.cc:635: // Exercise OnIncomingCapturedData() code path of VideoCaptureController On 2014/12/04 17:00:30, kbalazs wrote: > Instead of comment please make the name of the test meaningful enough. Something > like CaptureAllVideoFormat? All Done.
LGTM modulo my nits. Please update description and run some bots (f.i. linux_chromium_dbg). You'll still need some OWNERS rubberstamp. https://codereview.chromium.org/760593003/diff/140001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_controller_unittest.cc (right): https://codereview.chromium.org/760593003/diff/140001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller_unittest.cc:635: TEST_F(VideoCaptureControllerTest, CaptureAllVideoFormats) { Suggest test name like: CaptureInEachVideoFormatInSequence. https://codereview.chromium.org/760593003/diff/140001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller_unittest.cc:636: for (int format = 0; format <= media::PIXEL_FORMAT_MAX; format++) { for (VideoPixelFormat format = 0; format < PIXEL_FORMAT_UNKNOWN; ++format) { https://codereview.chromium.org/760593003/diff/140001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller_unittest.cc:641: gfx::Size capture_resolution(320, 240); const?
https://codereview.chromium.org/760593003/diff/140001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_controller_unittest.cc (right): https://codereview.chromium.org/760593003/diff/140001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller_unittest.cc:635: TEST_F(VideoCaptureControllerTest, CaptureAllVideoFormats) { On 2014/12/04 21:09:06, mcasas wrote: > Suggest test name like: CaptureInEachVideoFormatInSequence. Done. https://codereview.chromium.org/760593003/diff/140001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller_unittest.cc:636: for (int format = 0; format <= media::PIXEL_FORMAT_MAX; format++) { On 2014/12/04 21:09:07, mcasas wrote: > for (VideoPixelFormat format = 0; format < PIXEL_FORMAT_UNKNOWN; ++format) { for this, it will need to cast format to int when doing ++, so can I keep it to int? https://codereview.chromium.org/760593003/diff/140001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller_unittest.cc:641: gfx::Size capture_resolution(320, 240); On 2014/12/04 21:09:06, mcasas wrote: > const? Done.
lgtm https://codereview.chromium.org/760593003/diff/160001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_controller_unittest.cc (right): https://codereview.chromium.org/760593003/diff/160001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller_unittest.cc:645: // add clients nit: Comments need to be in complete sentences. http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Punctuation,_... https://codereview.chromium.org/760593003/diff/160001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller_unittest.cc:660: // Captured a new video frame nit: period at end of sentence.
https://codereview.chromium.org/760593003/diff/160001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_controller_unittest.cc (right): https://codereview.chromium.org/760593003/diff/160001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller_unittest.cc:645: // add clients On 2014/12/04 22:07:22, vrk wrote: > nit: Comments need to be in complete sentences. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Punctuation,_... Done. https://codereview.chromium.org/760593003/diff/160001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller_unittest.cc:660: // Captured a new video frame On 2014/12/04 22:07:22, vrk wrote: > nit: period at end of sentence. Done. https://codereview.chromium.org/760593003/diff/180001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_controller_unittest.cc (right): https://codereview.chromium.org/760593003/diff/180001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller_unittest.cc:636: for (int format = 0; format < media::PIXEL_FORMAT_TEXTURE; ++format) { Ah.. crashed.. OnIncomingCapturedData() didn't support PIXEL_FORMAT_TEXTURE. can I change the for loop to this?
https://codereview.chromium.org/760593003/diff/140001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_controller_unittest.cc (right): https://codereview.chromium.org/760593003/diff/140001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller_unittest.cc:636: for (int format = 0; format <= media::PIXEL_FORMAT_MAX; format++) { On 2014/12/04 21:54:16, zhaoze.zhou wrote: > On 2014/12/04 21:09:07, mcasas wrote: > > for (VideoPixelFormat format = 0; format < PIXEL_FORMAT_UNKNOWN; ++format) { > for this, it will need to cast format to int when doing ++, so can I keep it to > int? Ah, true. Leave it then. https://codereview.chromium.org/760593003/diff/180001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_controller_unittest.cc (right): https://codereview.chromium.org/760593003/diff/180001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller_unittest.cc:636: for (int format = 0; format < media::PIXEL_FORMAT_TEXTURE; ++format) { On 2014/12/04 23:01:34, zhaoze.zhou wrote: > Ah.. crashed.. OnIncomingCapturedData() didn't support > PIXEL_FORMAT_TEXTURE. can I change the for loop to this? Fair enough. I would add a comment saying that you're skipping PIXEL_FORMAT_UNKNOWN and PIXEL_FORMAT_TEXTURE, and rename test to e.g. DataCaptureInEachVideoFormatInSequence.
https://codereview.chromium.org/760593003/diff/180001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_controller_unittest.cc (right): https://codereview.chromium.org/760593003/diff/180001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller_unittest.cc:636: for (int format = 0; format < media::PIXEL_FORMAT_TEXTURE; ++format) { On 2014/12/04 23:11:54, mcasas wrote: > On 2014/12/04 23:01:34, zhaoze.zhou wrote: > > Ah.. crashed.. OnIncomingCapturedData() didn't support > > PIXEL_FORMAT_TEXTURE. can I change the for loop to this? > > Fair enough. > > I would add a comment saying that you're skipping > PIXEL_FORMAT_UNKNOWN and PIXEL_FORMAT_TEXTURE, and > rename test to e.g. DataCaptureInEachVideoFormatInSequence. Done. Thanks for your suggestion. :)
Patchset #9 (id:220001) has been deleted
https://codereview.chromium.org/760593003/diff/200001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_controller_unittest.cc (right): https://codereview.chromium.org/760593003/diff/200001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller_unittest.cc:663: static_cast<unsigned char*>(buffer.get()->data()), on the OnIncomingCapturedData(), there's no mechanism checking for "bad data" from http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_... Seems RGB24 caused crashes on chromeos
The CQ bit was checked by zhaoze.zhou@partner.samsung.com
The CQ bit was unchecked by zhaoze.zhou@partner.samsung.com
Patchset #9 (id:240001) has been deleted
Patchset #9 (id:260001) has been deleted
On 2014/12/05 17:05:11, zhaoze.zhou wrote: > https://codereview.chromium.org/760593003/diff/200001/content/browser/rendere... > File content/browser/renderer_host/media/video_capture_controller_unittest.cc > (right): > > https://codereview.chromium.org/760593003/diff/200001/content/browser/rendere... > content/browser/renderer_host/media/video_capture_controller_unittest.cc:663: > static_cast<unsigned char*>(buffer.get()->data()), > on the OnIncomingCapturedData(), there's no mechanism checking for > "bad data" from > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_... > > Seems RGB24 caused crashes on chromeos Now no crashes.
The CQ bit was checked by zhaoze.zhou@partner.samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/760593003/280001
Message was sent while issue was closed.
Committed patchset #9 (id:280001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/12ee8f52ccc7cfa8d95c6a0dc16b81fb2db86227 Cr-Commit-Position: refs/heads/master@{#307497} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
