Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2156003006:
Android video capture: use new libyuv::Android420ToI420 API for format converting. (Closed)
|
Created:
4 years, 5 months ago by braveyao Modified:
4 years, 5 months ago CC:
chromium-reviews, posciak+watch_chromium.org, jam, feature-media-reviews_chromium.org, darin-cc_chromium.org, mcasas+watch+vc_chromium.org, miu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAndroid video capture: use new libyuv::Android420ToI420 API for format converting.
libyuv offers a new API, Android420ToI420, to convert the
Android YUV_420_888 frame (which has interleaved UV planes)
to normal I420 frame.
This can significantly reduce the de-interlacing time on
Android, up to dozens of milliseconds, which can help a lot
on the overall end-to-end video delay.
BUG=629342
Committed: https://crrev.com/b3edc847c341fa19ccb964ba2ebf89325c8c3723
Cr-Commit-Position: refs/heads/master@{#407183}
Patch Set 1 #
Total comments: 7
Patch Set 2 : address comments and remove unused variables. #Patch Set 3 : rename variable and format #
Total comments: 6
Patch Set 4 : rebase with no code change #
Messages
Total messages: 47 (31 generated)
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
braveyao@chromium.org changed reviewers: + mcasas@chromium.org
Hi Miguel, as we discussed, here is the cl. Please take a look.
Some minor comments. Please review and rewrite the Description of the CL. https://codereview.chromium.org/2156003006/diff/1/media/capture/content/andro... File media/capture/content/android/screen_capture_machine_android.cc (right): https://codereview.chromium.org/2156003006/diff/1/media/capture/content/andro... media/capture/content/android/screen_capture_machine_android.cc:138: scoped_refptr<VideoFrame> unscaled_frame = frame; I found |unscaled_frame| a bit non intuitive, because it can be two things. What about naming it |intermediate_frame| or |temp_| or |scratchpad_| ? https://codereview.chromium.org/2156003006/diff/1/media/capture/video/android... File media/capture/video/android/video_capture_device_android.cc (right): https://codereview.chromium.org/2156003006/diff/1/media/capture/video/android... media/capture/video/android/video_capture_device_android.cc:246: base::TimeTicks current_time = base::TimeTicks::Now(); nit: const https://codereview.chromium.org/2156003006/diff/1/media/capture/video/android... media/capture/video/android/video_capture_device_android.cc:268: buffer.reset(new uint8_t[buffer_length]); std::unique_ptr<uint8_t[]> buffer(new uint8_t[buffer_length]); however, consider using an std::array instead.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Comments are addressed. PTAL. As to the description of cl, could you please be more specific on what to rewrite? I think it meets the request: Longer description of change addressing as appropriate: why the change is made, context if it is part of many changes, description of previous behavior and newly introduced differences, etc. https://codereview.chromium.org/2156003006/diff/1/media/capture/content/andro... File media/capture/content/android/screen_capture_machine_android.cc (right): https://codereview.chromium.org/2156003006/diff/1/media/capture/content/andro... media/capture/content/android/screen_capture_machine_android.cc:138: scoped_refptr<VideoFrame> unscaled_frame = frame; On 2016/07/19 01:22:58, mcasas wrote: > I found |unscaled_frame| a bit non intuitive, because it can > be two things. What about naming it |intermediate_frame| > or |temp_| or |scratchpad_| ? |temp_| is more neutral, but I prefer |unscaled_| since it is created for the possible scaling next. https://codereview.chromium.org/2156003006/diff/1/media/capture/video/android... File media/capture/video/android/video_capture_device_android.cc (right): https://codereview.chromium.org/2156003006/diff/1/media/capture/video/android... media/capture/video/android/video_capture_device_android.cc:246: base::TimeTicks current_time = base::TimeTicks::Now(); On 2016/07/19 01:22:58, mcasas wrote: > nit: const Done. https://codereview.chromium.org/2156003006/diff/1/media/capture/video/android... media/capture/video/android/video_capture_device_android.cc:268: buffer.reset(new uint8_t[buffer_length]); On 2016/07/19 01:22:58, mcasas wrote: > std::unique_ptr<uint8_t[]> buffer(new uint8_t[buffer_length]); > > however, consider using an std::array instead. Done. Maybe this comment is outdated, https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/zvGR9Ke_kHs. But it seems no one uses std::array in chromium at present. Leave it for now.
On 2016/07/19 19:06:02, braveyao wrote: > Comments are addressed. PTAL. > > As to the description of cl, could you please be more specific on what to > rewrite? I think it meets the request: > > Longer description of change addressing as appropriate: why > the change is made, context if it is part of many changes, > description of previous behavior and newly introduced > differences, etc. > Let me go one by one: > When YUV_420_888 format frame is requested on Android, i.e. to CameraDevice or > MediaProjection, the 420 frame got actually has interleaved u/v planes. "frame got"? Do you mean the "received" frame? > So we > have to do de-interlacing before we can use them as normal I420 frames. Who is "we"? > Doing it in JAVA codes (to camera capture) What is a "JAVA codes"? What do you mean "to camera capture"? Do you mean, "for" camera capture? > is pretty slow, which will cost more "will"? The CL has to be written in present tense. If you introduce a future tense, the whole logic of your argumentation loses its track. What you _would_ like to do, is to use the conditional tense to speculate as to what would happen if you were to follow a hypothetical case: "Java pixel format conversion costs/would have a duration of up to/ more than 20 ms..." > than 20ms to a 720P frame. "to a what" ? Do you mean, "for a 720p frame" ? > Doing it in native (to screen capture) " to" what? > is much better, > but still cost In our field, things don't cost, they have a computational cost. Having X and Xing is not the same. > a little bit more than 1ms to a 720P frame. "to" what? Again, "for"? > Now libyuv offers a new API, Android420ToI420, which can reduce the converting > time further to less than 1 ms to a 720P frame. It's more than 20 times faster > than JAVA and about 1.5 times faster than native. So it's important to utilize > the new API ASAP All this is nice, but you should describe it in the bug. The CL description is, well, descriptive, e.g. "This CL introduces the use of the new libyuv::AndroidI420ToI420() for pixel format conversion for both image capture and screen capture in Android." And you are argumenting: http://libweb.surrey.ac.uk/library/skills/writing%20Skills%20Leicester/page_4... Note, again, that this CL description is read by many developers and QAs that have no idea what you're talking about, and you have to make it as easy as possible for them to get an idea of what you're doing in the CL. > https://codereview.chromium.org/2156003006/diff/1/media/capture/content/andro... > File media/capture/content/android/screen_capture_machine_android.cc (right): > > https://codereview.chromium.org/2156003006/diff/1/media/capture/content/andro... > media/capture/content/android/screen_capture_machine_android.cc:138: > scoped_refptr<VideoFrame> unscaled_frame = frame; > On 2016/07/19 01:22:58, mcasas wrote: > > I found |unscaled_frame| a bit non intuitive, because it can > > be two things. What about naming it |intermediate_frame| > > or |temp_| or |scratchpad_| ? > > |temp_| is more neutral, but I prefer |unscaled_| since it is created for the > possible scaling next. Variable naming should refer to what the variable is meant to do, not what it is. > > https://codereview.chromium.org/2156003006/diff/1/media/capture/video/android... > File media/capture/video/android/video_capture_device_android.cc (right): > > https://codereview.chromium.org/2156003006/diff/1/media/capture/video/android... > media/capture/video/android/video_capture_device_android.cc:246: base::TimeTicks > current_time = base::TimeTicks::Now(); > On 2016/07/19 01:22:58, mcasas wrote: > > nit: const > > Done. > > https://codereview.chromium.org/2156003006/diff/1/media/capture/video/android... > media/capture/video/android/video_capture_device_android.cc:268: > buffer.reset(new uint8_t[buffer_length]); > On 2016/07/19 01:22:58, mcasas wrote: > > std::unique_ptr<uint8_t[]> buffer(new uint8_t[buffer_length]); > > > > however, consider using an std::array instead. > > Done. > > Maybe this comment is outdated, > https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/zvGR9Ke_kHs. > But it seems no one uses std::array in chromium at present. Leave it for now.
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Android video capture: use new libyuv::Android420ToI420 API for format converting. When YUV_420_888 format frame is requested on Android, i.e. to CameraDevice or MediaProjection, the 420 frame got actually has interleaved u/v planes. So we have to do de-interlacing before we can use them as normal I420 frames. Doing it in JAVA codes (to camera capture) is pretty slow, which will cost more than 20ms to a 720P frame. Doing it in native (to screen capture) is much better, but still cost a little bit more than 1ms to a 720P frame. Now libyuv offers a new API, Android420ToI420, which can reduce the converting time further to less than 1 ms to a 720P frame. It's more than 20 times faster than JAVA and about 1.5 times faster than native. So it's important to utilize the new API ASAP BUG=629342 ========== to ========== Android video capture: use new libyuv::Android420ToI420 API for format converting. libyuv offers a new API, Android420ToI420, to convert the Android YUV_420_888 frame (which has interleaved UV planes) to normal I420 frame. This can significantly reduce the de-interlacing time on Android, up to dozens of milliseconds, which can help a lot on the overall end-to-end video delay. BUG=629342 ==========
Done. PTAL. https://codereview.chromium.org/2156003006/diff/1/media/capture/content/andro... File media/capture/content/android/screen_capture_machine_android.cc (right): https://codereview.chromium.org/2156003006/diff/1/media/capture/content/andro... media/capture/content/android/screen_capture_machine_android.cc:138: scoped_refptr<VideoFrame> unscaled_frame = frame; On 2016/07/19 19:06:01, braveyao wrote: > On 2016/07/19 01:22:58, mcasas wrote: > > I found |unscaled_frame| a bit non intuitive, because it can > > be two things. What about naming it |intermediate_frame| > > or |temp_| or |scratchpad_| ? > > |temp_| is more neutral, but I prefer |unscaled_| since it is created for the > possible scaling next. Done.
LGTM % comment. Also, this code is notoriously not unit-tested, https://crbug.com/626857 (although it has content_browsertests and browser_tests), can we do something about that ? :) https://codereview.chromium.org/2156003006/diff/40001/media/capture/content/a... File media/capture/content/android/screen_capture_machine_android.cc (right): https://codereview.chromium.org/2156003006/diff/40001/media/capture/content/a... media/capture/content/android/screen_capture_machine_android.cc:76: src + offset, row_stride, temp_frame->visible_data(VideoFrame::kYPlane), A similar thing came up recently. I'm still not sure that Android provides _always_ ABGR, but will defer to you. The best way to handle this is like in [1], checking if Skia's kN32_SkColorType == kRGBA_8888_SkColorType Could you (at least) add a DCHECK_EQ(kN32_SkColorType, kRGBA_8888_SkColorType); ? [1] https://cs.chromium.org/chromium/src/content/renderer/media/video_track_recor...
Drive-by comments: https://codereview.chromium.org/2156003006/diff/40001/media/capture/video/and... File media/capture/video/android/video_capture_device_android.cc (right): https://codereview.chromium.org/2156003006/diff/40001/media/capture/video/and... media/capture/video/android/video_capture_device_android.cc:269: libyuv::Android420ToI420(y_src, y_stride, u_src, uv_row_stride, v_src, Sanity-check question: Is client code allowed to modify this memory? You might want to check the Android documentation (or contact Android devs, if the documentation is not sufficient). https://codereview.chromium.org/2156003006/diff/40001/media/capture/video/and... media/capture/video/android/video_capture_device_android.cc:279: // TODO(qiangchen): Investigate how to get raw timestamp for Android, We solved this problem in the screen capture (part I) change. You can get the timestamp from the Image object: https://developer.android.com/reference/android/media/Image.html#getTimestamp()
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
braveyao@chromium.org changed reviewers: + miu@chromium.org
miu@, thanks for the comments. Please check the answers. https://codereview.chromium.org/2156003006/diff/40001/media/capture/video/and... File media/capture/video/android/video_capture_device_android.cc (right): https://codereview.chromium.org/2156003006/diff/40001/media/capture/video/and... media/capture/video/android/video_capture_device_android.cc:269: libyuv::Android420ToI420(y_src, y_stride, u_src, uv_row_stride, v_src, On 2016/07/20 19:34:22, miu wrote: > Sanity-check question: Is client code allowed to modify this memory? You might > want to check the Android documentation (or contact Android devs, if the > documentation is not sufficient). Not quite understand the concern here. Planes have been copied to local 'buffer' here, so what client code gets should have nothing to do with the input buffer any more (not like the above OnFrameAvailable() ). https://codereview.chromium.org/2156003006/diff/40001/media/capture/video/and... media/capture/video/android/video_capture_device_android.cc:279: // TODO(qiangchen): Investigate how to get raw timestamp for Android, On 2016/07/20 19:34:22, miu wrote: > We solved this problem in the screen capture (part I) change. You can get the > timestamp from the Image object: > https://developer.android.com/reference/android/media/Image.html#getTimestamp() Screen capture doesn't care about A/V sync (no audio capture in MediaProjection). So the difference of drift between capture timestamp and system timestamp has no impact. Video capture has to consider it. Anyway it's something big for a separate cl. :)
The CQ bit was checked by braveyao@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
The CQ bit was checked by braveyao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mcasas@chromium.org Link to the patchset: https://codereview.chromium.org/2156003006/#ps60001 (title: "rebase with no code change")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by braveyao@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Android video capture: use new libyuv::Android420ToI420 API for format converting. libyuv offers a new API, Android420ToI420, to convert the Android YUV_420_888 frame (which has interleaved UV planes) to normal I420 frame. This can significantly reduce the de-interlacing time on Android, up to dozens of milliseconds, which can help a lot on the overall end-to-end video delay. BUG=629342 ========== to ========== Android video capture: use new libyuv::Android420ToI420 API for format converting. libyuv offers a new API, Android420ToI420, to convert the Android YUV_420_888 frame (which has interleaved UV planes) to normal I420 frame. This can significantly reduce the de-interlacing time on Android, up to dozens of milliseconds, which can help a lot on the overall end-to-end video delay. BUG=629342 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Android video capture: use new libyuv::Android420ToI420 API for format converting. libyuv offers a new API, Android420ToI420, to convert the Android YUV_420_888 frame (which has interleaved UV planes) to normal I420 frame. This can significantly reduce the de-interlacing time on Android, up to dozens of milliseconds, which can help a lot on the overall end-to-end video delay. BUG=629342 ========== to ========== Android video capture: use new libyuv::Android420ToI420 API for format converting. libyuv offers a new API, Android420ToI420, to convert the Android YUV_420_888 frame (which has interleaved UV planes) to normal I420 frame. This can significantly reduce the de-interlacing time on Android, up to dozens of milliseconds, which can help a lot on the overall end-to-end video delay. BUG=629342 Committed: https://crrev.com/b3edc847c341fa19ccb964ba2ebf89325c8c3723 Cr-Commit-Position: refs/heads/master@{#407183} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/b3edc847c341fa19ccb964ba2ebf89325c8c3723 Cr-Commit-Position: refs/heads/master@{#407183}
Message was sent while issue was closed.
https://codereview.chromium.org/2156003006/diff/40001/media/capture/video/and... File media/capture/video/android/video_capture_device_android.cc (right): https://codereview.chromium.org/2156003006/diff/40001/media/capture/video/and... media/capture/video/android/video_capture_device_android.cc:269: libyuv::Android420ToI420(y_src, y_stride, u_src, uv_row_stride, v_src, On 2016/07/20 22:02:04, braveyao wrote: > On 2016/07/20 19:34:22, miu wrote: > > Sanity-check question: Is client code allowed to modify this memory? You might > > want to check the Android documentation (or contact Android devs, if the > > documentation is not sufficient). > > Not quite understand the concern here. Planes have been copied to local 'buffer' > here, so what client code gets should have nothing to do with the input buffer > any more (not like the above OnFrameAvailable() ). Oh, never mind. I read this wrong. No problem here! :) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
