Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1917023003:
ScreenCapture for Android phase1, part I (Closed)
|
Created:
4 years, 7 months ago by braveyao Modified:
4 years, 5 months ago CC:
avayvod+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, mcasas+watch+vc_chromium.org, mlamouri+watch-media_chromium.org, posciak+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionScreenCapture for Android phase1, part I
The document about ScreenCapture for Android is here, https://goo.gl/QNH29g.
This cl is mainly based on the https://codereview.chromium.org/1140113002/.
Only the codes under media/ are included here, which implements the JAVA
capture with Android MediaProjection API and JNI connectors, with some issues
fixed so it can always work.
The control part in content/ and chrome/ will be in the next cl, as part II.
BUG=487935
Committed: https://crrev.com/2f731e17983201082d9fc725cf7717868fc1e75d
Cr-Commit-Position: refs/heads/master@{#403555}
Patch Set 1 #
Total comments: 64
Patch Set 2 : address review comments #
Total comments: 7
Patch Set 3 : relocate java files, support YUV format and improve JNI operation #
Total comments: 78
Patch Set 4 : address review comments #
Total comments: 13
Patch Set 5 : address review comments #
Total comments: 4
Patch Set 6 : rebase to use the new CreatePowerSaveBlocker() and pass pixelStride to native #
Total comments: 18
Patch Set 7 : correct YUV420 unpacking, add cropping support #
Total comments: 12
Patch Set 8 : address review comments #
Total comments: 23
Patch Set 9 : address comments #
Total comments: 11
Patch Set 10 : correct RGBA frame convert-scale error and rebase #
Total comments: 8
Patch Set 11 : call ObserveEventAndDecideCapture() as early as possible and rebase #Patch Set 12 : add dependency #Messages
Total messages: 83 (21 generated)
braveyao@chromium.org changed reviewers: + mcasas@chromium.org, miu@chromium.org, sergeyu@chromium.org
Hi All, Here is capture part of screen capture for android. Please help to take a look!
https://codereview.chromium.org/1917023003/diff/1/media/base/android/java/src... File media/base/android/java/src/org/chromium/media/ScreenCapture.java (right): https://codereview.chromium.org/1917023003/diff/1/media/base/android/java/src... media/base/android/java/src/org/chromium/media/ScreenCapture.java:1: // Copyright 2015 The Chromium Authors. All rights reserved. 2016 https://codereview.chromium.org/1917023003/diff/1/media/base/android/java/src... media/base/android/java/src/org/chromium/media/ScreenCapture.java:45: private class CrImageReaderListener implements ImageReader.OnImageAvailableListener { This is _very_ similar class to [1] except for the synchronized part and that here we only accept RGBA_8888. Please do not duplicate code, extend it here. https://code.google.com/p/chromium/codesearch#chromium/src/media/capture/vide... https://codereview.chromium.org/1917023003/diff/1/media/base/android/java/src... media/base/android/java/src/org/chromium/media/ScreenCapture.java:136: } catch (Exception e) { No-no, don't catch generic exceptions out of a large code chunk [1] (Chromium Java style guide follows the Android one). [1] https://source.android.com/source/code-style.html#dont-catch-generic-exception https://codereview.chromium.org/1917023003/diff/1/media/base/android/java/src... media/base/android/java/src/org/chromium/media/ScreenCapture.java:148: // This method was deprecated in API level 23 by onAttach(Context). // TODO(braveyao) make a crbug.com for it and link it here. https://codereview.chromium.org/1917023003/diff/1/media/base/android/java/src... media/base/android/java/src/org/chromium/media/ScreenCapture.java:172: Log.e(TAG, "ScreenCaptureEvent: " + ex); It's not an event, is an exception. https://codereview.chromium.org/1917023003/diff/1/media/base/android/java/src... media/base/android/java/src/org/chromium/media/ScreenCapture.java:193: Log.e(TAG, "ScreenCaptureExcaption " + e); typo https://codereview.chromium.org/1917023003/diff/1/media/base/android/java/src... media/base/android/java/src/org/chromium/media/ScreenCapture.java:206: result = 1; Maybe |result| should be boolean? https://codereview.chromium.org/1917023003/diff/1/media/base/android/java/src... media/base/android/java/src/org/chromium/media/ScreenCapture.java:215: // start screen capture. s/start/Starts/ But, honestly, this comment and l.248 are quite superfluous. https://codereview.chromium.org/1917023003/diff/1/media/base/android/java/src... media/base/android/java/src/org/chromium/media/ScreenCapture.java:227: Log.d(TAG, "mMediaProjection is null"); Log.e https://codereview.chromium.org/1917023003/diff/1/media/base/android/java/src... File media/base/android/java/src/org/chromium/media/ScreenCaptureFactory.java (right): https://codereview.chromium.org/1917023003/diff/1/media/base/android/java/src... media/base/android/java/src/org/chromium/media/ScreenCaptureFactory.java:1: // Copyright 2015 The Chromium Authors. All rights reserved. 2016... https://codereview.chromium.org/1917023003/diff/1/media/base/android/java/src... media/base/android/java/src/org/chromium/media/ScreenCaptureFactory.java:15: class ScreenCaptureFactory { This class is tiny, merge it in ScreenCapture as a factory method. https://codereview.chromium.org/1917023003/diff/1/media/base/android/media_jn... File media/base/android/media_jni_registrar.cc (right): https://codereview.chromium.org/1917023003/diff/1/media/base/android/media_jn... media/base/android/media_jni_registrar.cc:34: ScreenCaptureMachineAndroid::RegisterScreenCaptureMachine}, There is no capture in this registrar anymore, nor Java files related to capture in media/base/android, they are in media/capture/video/android/... You will have to create a media/capture/content/android and add a new registrar there. The capture one will give you an example. https://codereview.chromium.org/1917023003/diff/1/media/capture/content/andro... File media/capture/content/android/screen_capture_device_android.cc (right): https://codereview.chromium.org/1917023003/diff/1/media/capture/content/andro... media/capture/content/android/screen_capture_device_android.cc:15: core_.reset(new ScreenCaptureDeviceCore(base::WrapUnique(machine))); Make |core_| const and initialize it in the initializer list: core_(new ScreenCaptureDeviceCore( make_unique_ptr(new ScreenCaptureMachineAndroid()); https://codereview.chromium.org/1917023003/diff/1/media/capture/content/andro... media/capture/content/android/screen_capture_device_android.cc:30: core_->StopAndDeAllocate(); ScreenCaptureDeviceAndroid is just a forwarder class, I suggest removing it altogether and using ScreenCaptureDeviceCore directly instead, unless I'm missing something. https://codereview.chromium.org/1917023003/diff/1/media/capture/content/andro... File media/capture/content/android/screen_capture_device_android.h (right): https://codereview.chromium.org/1917023003/diff/1/media/capture/content/andro... media/capture/content/android/screen_capture_device_android.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. 2016 here and elsewhere where you add a file. https://codereview.chromium.org/1917023003/diff/1/media/capture/content/andro... media/capture/content/android/screen_capture_device_android.h:5: #ifndef MEDIA_SCREEN_CAPTURE_ANDROID_SCREEN_CAPTURE_DEVICE_ANDROID_H_ Guards don't match path, here and in several other files. https://codereview.chromium.org/1917023003/diff/1/media/capture/content/andro... media/capture/content/android/screen_capture_device_android.h:17: explicit ScreenCaptureDeviceAndroid(); No need for explicit.
Some initial high-level comments: https://codereview.chromium.org/1917023003/diff/1/media/base/android/java/src... File media/base/android/java/src/org/chromium/media/ScreenCapture.java (right): https://codereview.chromium.org/1917023003/diff/1/media/base/android/java/src... media/base/android/java/src/org/chromium/media/ScreenCapture.java:1: // Copyright 2015 The Chromium Authors. All rights reserved. Hmm...these files are so far away (in the directory hierarchy) from their C++ modules. Is this common practice when doing Java in the Chromium project? Would it be easy to move these into media/capture/content/android/...? https://codereview.chromium.org/1917023003/diff/1/media/capture/content/andro... File media/capture/content/android/screen_capture_device_android.h (right): https://codereview.chromium.org/1917023003/diff/1/media/capture/content/andro... media/capture/content/android/screen_capture_device_android.h:23: void StopAndDeAllocate() override; Please also implement the RequestRefreshFrame() method as well: https://code.google.com/p/chromium/codesearch#chromium/src/media/capture/vide... Generally, this just delegates to ScreenCaptureDeviceCore. For example: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/me... https://codereview.chromium.org/1917023003/diff/1/media/capture/content/andro... File media/capture/content/android/screen_capture_machine_android.cc (right): https://codereview.chromium.org/1917023003/diff/1/media/capture/content/andro... media/capture/content/android/screen_capture_machine_android.cc:44: bool oracle_decision = oracle_proxy_->ObserveEventAndDecideCapture( I'm not familiar with the API provided by the Android platform, but would it be possible for this decision to be made *before* the platform does all the work to read-back the screen frame from the GPU? For example, tab capture (WebContentsVideoCaptureDevice) uses an event-driven "subscriber" model, where the compositor notifies it whenever compositing will occur, and provides it with the damage region. Then, the oracle makes a decision whether it will capture the next frame. So, the expensive operation of read-back only happens when it absolutely needs to (saving CPU/GPU/power, smoother/faster performance, etc.). https://codereview.chromium.org/1917023003/diff/1/media/capture/content/andro... media/capture/content/android/screen_capture_machine_android.cc:61: libyuv::ARGBScale(source_argb, source_stride, cropWidth, cropHeight, Can the platform do this? Usually scaling is much more performant in the GPU/compositing side of things, rather than on one thread on the CPU. IIUC, the Java side of this implementation could create a VirtualDisplay with the desired width/height/dpi to accomplish the scaling operation. Also, how is screen rotation handled? You may need to flip and/or letterbox/pillarbox to account for 90/180/270 degrees. https://codereview.chromium.org/1917023003/diff/1/media/capture/content/andro... media/capture/content/android/screen_capture_machine_android.cc:74: media::ConvertRGB32ToYUV( Is it possible to push colorspace conversion upstream as well? On the Java side, ImageReader could be created with ImageFormat.YUV_420_888 format. https://codereview.chromium.org/1917023003/diff/1/media/capture/content/andro... media/capture/content/android/screen_capture_machine_android.cc:119: capture_format_.pixel_format = media::PIXEL_FORMAT_ARGB; I believe the |params| would specify I420 or YV12, since that's the format of the VideoFrames being produced by ScreenCaptureMachineAndroid. https://codereview.chromium.org/1917023003/diff/1/media/capture/content/andro... media/capture/content/android/screen_capture_machine_android.cc:144: void ScreenCaptureMachineAndroid::MaybeCaptureForRefresh() {} Depending on the behavior of the platform, you may need to implement this properly. Ideal: Platform only calls OnFrameAvailable() whenever the screen changed. That means a non-changing screen would effectively "pause" capture. If this can happen, then MaybeCaptureForRefresh() needs to force frame captures to satisfy downstream consumers needing the extra "refresh" frames (e.g., video encoder, streaming implementations, etc.). Power-hungry/Wasteful behavior: Platform always calls OnFrameAvailable() at a constant framerate, regardless of whether the screen content has changed. In this case, you don't need to implement MaybeCaptureForRefresh() because a frame will be produced in the near future anyway.
Thanks for the review! All comments are addressed or answered(due to some work left for phase 2). Please help to have a another look and give further comments! https://codereview.chromium.org/1917023003/diff/1/media/base/android/java/src... File media/base/android/java/src/org/chromium/media/ScreenCapture.java (right): https://codereview.chromium.org/1917023003/diff/1/media/base/android/java/src... media/base/android/java/src/org/chromium/media/ScreenCapture.java:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/04/27 01:19:26, mcasas wrote: > 2016 Done. https://codereview.chromium.org/1917023003/diff/1/media/base/android/java/src... media/base/android/java/src/org/chromium/media/ScreenCapture.java:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/04/27 05:24:53, miu wrote: > Hmm...these files are so far away (in the directory hierarchy) from their C++ > modules. Is this common practice when doing Java in the Chromium project? Would > it be easy to move these into media/capture/content/android/...? It seems like a common practice to gather all the JAVA files under one place for each component of chromium, such as chrome/android/java, mojo/public/java, also media/base/android/java. As to media/base/android, once all the Java files for audio/video/mediacodec are located there. It's very recently that the Java files for video capture are moved to media/capture/vdieo/android, while all others remain at same place. So I'm a little confusing about the moving of video capture. But still think it's good to put the screen capture java files with others. WDYT? https://codereview.chromium.org/1917023003/diff/1/media/base/android/java/src... media/base/android/java/src/org/chromium/media/ScreenCapture.java:45: private class CrImageReaderListener implements ImageReader.OnImageAvailableListener { On 2016/04/27 01:19:26, mcasas wrote: > This is _very_ similar class to [1] except for the > synchronized part and that here we only accept > RGBA_8888. Please do not duplicate code, extend it > here. > > https://code.google.com/p/chromium/codesearch#chromium/src/media/capture/vide... As we discussed offline. I understand the concern here. But not very comfortable to link ScreenCapture with VideoCaptureCamera2. It's too troublesome to extend it here and not good for readability as I imagine. Maybe we just keep it so far. https://codereview.chromium.org/1917023003/diff/1/media/base/android/java/src... media/base/android/java/src/org/chromium/media/ScreenCapture.java:136: } catch (Exception e) { On 2016/04/27 01:19:26, mcasas wrote: > No-no, don't catch generic exceptions out of a > large code chunk [1] (Chromium Java style guide > follows the Android one). > > [1] > https://source.android.com/source/code-style.html#dont-catch-generic-exception Done. https://codereview.chromium.org/1917023003/diff/1/media/base/android/java/src... media/base/android/java/src/org/chromium/media/ScreenCapture.java:148: // This method was deprecated in API level 23 by onAttach(Context). On 2016/04/27 01:19:26, mcasas wrote: > // TODO(braveyao) > make a http://crbug.com for it and link it here. Done. Not add a crbug though, since this will exist for many years before API 23 is the minSdkVersion. https://codereview.chromium.org/1917023003/diff/1/media/base/android/java/src... media/base/android/java/src/org/chromium/media/ScreenCapture.java:172: Log.e(TAG, "ScreenCaptureEvent: " + ex); On 2016/04/27 01:19:26, mcasas wrote: > It's not an event, is an exception. Done. https://codereview.chromium.org/1917023003/diff/1/media/base/android/java/src... media/base/android/java/src/org/chromium/media/ScreenCapture.java:193: Log.e(TAG, "ScreenCaptureExcaption " + e); On 2016/04/27 01:19:26, mcasas wrote: > typo Done. https://codereview.chromium.org/1917023003/diff/1/media/base/android/java/src... media/base/android/java/src/org/chromium/media/ScreenCapture.java:206: result = 1; On 2016/04/27 01:19:26, mcasas wrote: > Maybe |result| should be boolean? Done. https://codereview.chromium.org/1917023003/diff/1/media/base/android/java/src... media/base/android/java/src/org/chromium/media/ScreenCapture.java:215: // start screen capture. On 2016/04/27 01:19:26, mcasas wrote: > s/start/Starts/ > But, honestly, this comment and l.248 are quite > superfluous. Done. https://codereview.chromium.org/1917023003/diff/1/media/base/android/java/src... media/base/android/java/src/org/chromium/media/ScreenCapture.java:227: Log.d(TAG, "mMediaProjection is null"); On 2016/04/27 01:19:26, mcasas wrote: > Log.e Done. https://codereview.chromium.org/1917023003/diff/1/media/base/android/java/src... File media/base/android/java/src/org/chromium/media/ScreenCaptureFactory.java (right): https://codereview.chromium.org/1917023003/diff/1/media/base/android/java/src... media/base/android/java/src/org/chromium/media/ScreenCaptureFactory.java:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/04/27 01:19:26, mcasas wrote: > 2016... Done. https://codereview.chromium.org/1917023003/diff/1/media/base/android/java/src... media/base/android/java/src/org/chromium/media/ScreenCaptureFactory.java:15: class ScreenCaptureFactory { On 2016/04/27 01:19:27, mcasas wrote: > This class is tiny, merge it in ScreenCapture as a > factory method. Done. https://codereview.chromium.org/1917023003/diff/1/media/base/android/media_jn... File media/base/android/media_jni_registrar.cc (right): https://codereview.chromium.org/1917023003/diff/1/media/base/android/media_jn... media/base/android/media_jni_registrar.cc:34: ScreenCaptureMachineAndroid::RegisterScreenCaptureMachine}, On 2016/04/27 01:19:27, mcasas wrote: > There is no capture in this registrar anymore, nor Java > files related to capture in media/base/android, they are > in media/capture/video/android/... > You will have to create a media/capture/content/android > and add a new registrar there. The capture one will give > you an example. Isn't Audio capture also a capture? It seems like a common practice to gather all the JAVA files under one place for each component of chromium, such as chrome/android/java, mojo/public/java, also media/base/android/java once upon a time. I saw you moved the camera capture to another place recently. Any reason for that? https://codereview.chromium.org/1917023003/diff/1/media/capture/content/andro... File media/capture/content/android/screen_capture_device_android.cc (right): https://codereview.chromium.org/1917023003/diff/1/media/capture/content/andro... media/capture/content/android/screen_capture_device_android.cc:15: core_.reset(new ScreenCaptureDeviceCore(base::WrapUnique(machine))); On 2016/04/27 01:19:27, mcasas wrote: > Make |core_| const and initialize it in the > initializer list: > core_(new ScreenCaptureDeviceCore( > make_unique_ptr(new ScreenCaptureMachineAndroid()); Done. Anyway I think it's also good to stay in same way as desktop capture aura does. https://codereview.chromium.org/1917023003/diff/1/media/capture/content/andro... media/capture/content/android/screen_capture_device_android.cc:30: core_->StopAndDeAllocate(); On 2016/04/27 01:19:27, mcasas wrote: > ScreenCaptureDeviceAndroid is just a forwarder > class, I suggest removing it altogether and > using ScreenCaptureDeviceCore directly instead, > unless I'm missing something. Done. Move it to Content as a front, also for the purpose to start power_save_block. https://codereview.chromium.org/1917023003/diff/1/media/capture/content/andro... File media/capture/content/android/screen_capture_device_android.h (right): https://codereview.chromium.org/1917023003/diff/1/media/capture/content/andro... media/capture/content/android/screen_capture_device_android.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/04/27 01:19:27, mcasas wrote: > 2016 here and elsewhere where you > add a file. Done. https://codereview.chromium.org/1917023003/diff/1/media/capture/content/andro... media/capture/content/android/screen_capture_device_android.h:5: #ifndef MEDIA_SCREEN_CAPTURE_ANDROID_SCREEN_CAPTURE_DEVICE_ANDROID_H_ On 2016/04/27 01:19:27, mcasas wrote: > Guards don't match path, here and in several > other files. Done. https://codereview.chromium.org/1917023003/diff/1/media/capture/content/andro... media/capture/content/android/screen_capture_device_android.h:17: explicit ScreenCaptureDeviceAndroid(); On 2016/04/27 01:19:27, mcasas wrote: > No need for explicit. Done. https://codereview.chromium.org/1917023003/diff/1/media/capture/content/andro... media/capture/content/android/screen_capture_device_android.h:23: void StopAndDeAllocate() override; On 2016/04/27 05:24:53, miu wrote: > Please also implement the RequestRefreshFrame() method as well: > https://code.google.com/p/chromium/codesearch#chromium/src/media/capture/vide... > > Generally, this just delegates to ScreenCaptureDeviceCore. For example: > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/me... Done. https://codereview.chromium.org/1917023003/diff/1/media/capture/content/andro... File media/capture/content/android/screen_capture_machine_android.cc (right): https://codereview.chromium.org/1917023003/diff/1/media/capture/content/andro... media/capture/content/android/screen_capture_machine_android.cc:44: bool oracle_decision = oracle_proxy_->ObserveEventAndDecideCapture( On 2016/04/27 05:24:53, miu wrote: > I'm not familiar with the API provided by the Android platform, but would it be > possible for this decision to be made *before* the platform does all the work to > read-back the screen frame from the GPU? > > For example, tab capture (WebContentsVideoCaptureDevice) uses an event-driven > "subscriber" model, where the compositor notifies it whenever compositing will > occur, and provides it with the damage region. Then, the oracle makes a decision > whether it will capture the next frame. So, the expensive operation of read-back > only happens when it absolutely needs to (saving CPU/GPU/power, smoother/faster > performance, etc.). Android will send a copy of the whole display whenever it updates the rendering to screen. We just wait for the incoming frames, kind of same as camera capture. So I suppose there is no way to do the decision beforehand. Any other concern on this? https://codereview.chromium.org/1917023003/diff/1/media/capture/content/andro... media/capture/content/android/screen_capture_machine_android.cc:61: libyuv::ARGBScale(source_argb, source_stride, cropWidth, cropHeight, On 2016/04/27 05:24:53, miu wrote: > Can the platform do this? Usually scaling is much more performant in the > GPU/compositing side of things, rather than on one thread on the CPU. > > IIUC, the Java side of this implementation could create a VirtualDisplay with > the desired width/height/dpi to accomplish the scaling operation. > > Also, how is screen rotation handled? You may need to flip and/or > letterbox/pillarbox to account for 90/180/270 degrees. Exactly. You can do the setting to VirtualDisplay to get required frame size, also call resize() to resize it on the fly.In phase 1 I only guarantee the default setting working. Passing into frame size constraints may work, but I haven't tested it. Currently we will always get a VGA size capture in landscape position by default. When device is in portrait position, the capture frame will have black padding on both side. This piece of code doesn't run here. So I replace it with a DCHECK. Will study it more in phase 2. https://codereview.chromium.org/1917023003/diff/1/media/capture/content/andro... media/capture/content/android/screen_capture_machine_android.cc:74: media::ConvertRGB32ToYUV( On 2016/04/27 05:24:53, miu wrote: > Is it possible to push colorspace conversion upstream as well? On the Java > side, ImageReader could be created with ImageFormat.YUV_420_888 format. Thanks for the advice. I didn't consider this before. With a quick test, I got a same issue as reported in https://code.google.com/p/android/issues/detail?id=180348 on N9, which has no such problem on N5. Anyway, add a TODO for a further study in phase 2. https://codereview.chromium.org/1917023003/diff/1/media/capture/content/andro... media/capture/content/android/screen_capture_machine_android.cc:119: capture_format_.pixel_format = media::PIXEL_FORMAT_ARGB; On 2016/04/27 05:24:53, miu wrote: > I believe the |params| would specify I420 or YV12, since that's the format of > the VideoFrames being produced by ScreenCaptureMachineAndroid. Yes, |params| asks for I420. Same as above comment, I will check if we could get MediaProjection/ImageReader work with that format in phase 2. And here we only care about the frame size. So remove the unnecessary stuff. https://codereview.chromium.org/1917023003/diff/1/media/capture/content/andro... media/capture/content/android/screen_capture_machine_android.cc:144: void ScreenCaptureMachineAndroid::MaybeCaptureForRefresh() {} On 2016/04/27 05:24:53, miu wrote: > Depending on the behavior of the platform, you may need to implement this > properly. > > Ideal: Platform only calls OnFrameAvailable() whenever the screen changed. That > means a non-changing screen would effectively "pause" capture. If this can > happen, then MaybeCaptureForRefresh() needs to force frame captures to satisfy > downstream consumers needing the extra "refresh" frames (e.g., video encoder, > streaming implementations, etc.). > > Power-hungry/Wasteful behavior: Platform always calls OnFrameAvailable() at a > constant framerate, regardless of whether the screen content has changed. In > this case, you don't need to implement MaybeCaptureForRefresh() because a frame > will be produced in the near future anyway. Thanks for explaining this new method. Android also only calls OnFrameAvailable() whenever the screen changed. So the current consequence is the sending fps will drop to 0 in some cases. What's the impact here? I see in ThreadSafeCaptureOracle::AttemptPassiveRefresh() if it returns true, then it won't call to MaybeCaptureForRefresh(). Since Android works in passive way too, e.g. no API to ask for one frame from it, so I think we can make it stop there. Is that right? How can I guarantee it? Anyway it's also a phase 2 job. Will give a further study later.
A few more comments, quick reply since you will likely need to shovel files around. Also, please remove the PatchSets that don't contain reviewers comments, to simplify side-by-side comparisons. https://codereview.chromium.org/1917023003/diff/1/media/base/android/java/src... File media/base/android/java/src/org/chromium/media/ScreenCapture.java (right): https://codereview.chromium.org/1917023003/diff/1/media/base/android/java/src... media/base/android/java/src/org/chromium/media/ScreenCapture.java:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/05/04 18:49:40, braveyao wrote: > On 2016/04/27 05:24:53, miu wrote: > > Hmm...these files are so far away (in the directory hierarchy) from their C++ > > modules. Is this common practice when doing Java in the Chromium project? > Would > > it be easy to move these into media/capture/content/android/...? > > It seems like a common practice to gather all the JAVA files under one place for > each component of chromium, such as chrome/android/java, mojo/public/java, also > media/base/android/java. As to media/base/android, once all the Java files for > audio/video/mediacodec are located there. What do you mean "once all the Java files ..." > It's very recently that the Java files > for video capture are moved to media/capture/vdieo/android, while all others > remain at same place. So I'm a little confusing about the moving of video > capture. But still think it's good to put the screen capture java files with > others. WDYT? Java files, like any other, are grouped in any way that makes sense due to functionality, not language. I agree with miu@ and my comment in media/base/android/media_jni_registrar.cc that these files should go elsewhere. https://codereview.chromium.org/1917023003/diff/1/media/base/android/java/src... media/base/android/java/src/org/chromium/media/ScreenCapture.java:45: private class CrImageReaderListener implements ImageReader.OnImageAvailableListener { On 2016/05/04 18:49:40, braveyao wrote: > On 2016/04/27 01:19:26, mcasas wrote: > > This is _very_ similar class to [1] except for the > > synchronized part and that here we only accept > > RGBA_8888. Please do not duplicate code, extend it > > here. > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/media/capture/vide... > > As we discussed offline. I understand the concern here. But not very comfortable > to link ScreenCapture with VideoCaptureCamera2. It's too troublesome to extend > it here and not good for readability as I imagine. Maybe we just keep it so far. > It's not complicated. You're probably confused as to where to put this common class, since content/ and video/ captures are in different folders, but in Java, everything is reorganised as packages! (See line 5 of this file). You need to make a new Java file, call it CrImageReaderListener, and make sure it goes to package org.chromium.media, then you can import it from any other files, including this one, and just extend it here by doing: private class MyCrImageReaderListener extends CrImageReaderListener { @Override public void OnImageAvailable(ImageReader reader) { // copy here the synchronised part... super.OnImageAvailable( pixelFormat ) } } where you have also added a parameter pixelFormat to CrImageReaderListener.OnImageAvailable and you're using it in the comparison of [1]. [1] https://code.google.com/p/chromium/codesearch#chromium/src/media/capture/vide... https://codereview.chromium.org/1917023003/diff/1/media/base/android/java/src... media/base/android/java/src/org/chromium/media/ScreenCapture.java:136: } catch (Exception e) { On 2016/05/04 18:49:40, braveyao wrote: > On 2016/04/27 01:19:26, mcasas wrote: > > No-no, don't catch generic exceptions out of a > > large code chunk [1] (Chromium Java style guide > > follows the Android one). > > > > [1] > > https://source.android.com/source/code-style.html#dont-catch-generic-exception > > Done. Still you need to not catch Exception, but each individual one being thrown, like in https://code.google.com/p/chromium/codesearch#chromium/src/media/capture/vide... https://codereview.chromium.org/1917023003/diff/1/media/base/android/media_jn... File media/base/android/media_jni_registrar.cc (right): https://codereview.chromium.org/1917023003/diff/1/media/base/android/media_jn... media/base/android/media_jni_registrar.cc:34: ScreenCaptureMachineAndroid::RegisterScreenCaptureMachine}, On 2016/05/04 18:49:40, braveyao wrote: > On 2016/04/27 01:19:27, mcasas wrote: > > There is no capture in this registrar anymore, nor Java > > files related to capture in media/base/android, they are > > in media/capture/video/android/... > > You will have to create a media/capture/content/android > > and add a new registrar there. The capture one will give > > you an example. > > Isn't Audio capture also a capture? Flip the question: is Screen Capture more a Video Capture or an Audio Record "thing" ? > It seems like a common practice to gather > all the JAVA files under one place for each component of chromium, such as > chrome/android/java, mojo/public/java, also media/base/android/java once upon a > time. I saw you moved the camera capture to another place recently. Any reason > for that? See my comment before. Files are grouped together due to functionality, not language-convenience. I personally always found it misleading that the Java files for specific functionality are in a folder called base... https://codereview.chromium.org/1917023003/diff/60001/media/base/android/java... File media/base/android/java/src/org/chromium/media/ScreenCapture.java (right): https://codereview.chromium.org/1917023003/diff/60001/media/base/android/java... media/base/android/java/src/org/chromium/media/ScreenCapture.java:227: nativeOnActivityResult(mNativeScreenCaptureMachineAndroid, result); s/result/resultCode == Activity.RESULT_OK/ ?
avayvod@chromium.org changed reviewers: + avayvod@chromium.org
Re: the placement of the Java files. CC: Min who might have a better insight into the current placement of Java files under media/ https://codereview.chromium.org/1917023003/diff/1/media/base/android/java/src... File media/base/android/java/src/org/chromium/media/ScreenCapture.java (right): https://codereview.chromium.org/1917023003/diff/1/media/base/android/java/src... media/base/android/java/src/org/chromium/media/ScreenCapture.java:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/05/04 at 19:56:47, mcasas wrote: > On 2016/05/04 18:49:40, braveyao wrote: > > On 2016/04/27 05:24:53, miu wrote: > > > Hmm...these files are so far away (in the directory hierarchy) from their C++ > > > modules. Is this common practice when doing Java in the Chromium project? > > Would > > > it be easy to move these into media/capture/content/android/...? > > > > It seems like a common practice to gather all the JAVA files under one place for > > each component of chromium, such as chrome/android/java, mojo/public/java, also > > media/base/android/java. As to media/base/android, once all the Java files for > > audio/video/mediacodec are located there. > > What do you mean "once all the Java files ..." > > > It's very recently that the Java files > > for video capture are moved to media/capture/vdieo/android, while all others > > remain at same place. So I'm a little confusing about the moving of video > > capture. But still think it's good to put the screen capture java files with > > others. WDYT? > > Java files, like any other, are grouped in any way > that makes sense due to functionality, not language. > I agree with miu@ and my comment in > media/base/android/media_jni_registrar.cc > that these files should go elsewhere. FWIW, in general the corresponding file for src/foo/bar/boo/far/xyz.cc would live in src/foo/android/java/src/org/chromium/foo/bar/boo/far/XYZ.java So in this case and all other media Java files, the paths would look like src/media/android/java/src/org/chromium/{base,capture}/... However, it seems like currently Java files under media/ use a different structure: src/media/foo/boo/bar.cc corresponds to src/media/foo/boo/android/java/src/org/chromium/media/Bar.java (examples are media/base and media/capture/video except that two Java files corresponding to media/audio/ counterparts are placed under media/base/android/java with all the rest -- I agree it may look confusing). Therefore the consistent path to this Java file seems to be src/media/capture/content/android/java/src/org/chromium/media/ScreenCapture[Machine].java I agree that until the files under media follow the general rule, this makes more sense than putting them under media/base/android/java.
On 2016/05/04 20:22:39, whywhat_OOO_April_28_May_2 wrote: > Re: the placement of the Java files. > > CC: Min who might have a better insight into the current placement of Java files > under media/ > > https://codereview.chromium.org/1917023003/diff/1/media/base/android/java/src... > File media/base/android/java/src/org/chromium/media/ScreenCapture.java (right): > > https://codereview.chromium.org/1917023003/diff/1/media/base/android/java/src... > media/base/android/java/src/org/chromium/media/ScreenCapture.java:1: // > Copyright 2015 The Chromium Authors. All rights reserved. > On 2016/05/04 at 19:56:47, mcasas wrote: > > On 2016/05/04 18:49:40, braveyao wrote: > > > On 2016/04/27 05:24:53, miu wrote: > > > > Hmm...these files are so far away (in the directory hierarchy) from their > C++ > > > > modules. Is this common practice when doing Java in the Chromium project? > > > Would > > > > it be easy to move these into media/capture/content/android/...? > > > > > > It seems like a common practice to gather all the JAVA files under one place > for > > > each component of chromium, such as chrome/android/java, mojo/public/java, > also > > > media/base/android/java. As to media/base/android, once all the Java files > for > > > audio/video/mediacodec are located there. > > > > What do you mean "once all the Java files ..." > > > > > It's very recently that the Java files > > > for video capture are moved to media/capture/vdieo/android, while all others > > > remain at same place. So I'm a little confusing about the moving of video > > > capture. But still think it's good to put the screen capture java files with > > > others. WDYT? > > > > Java files, like any other, are grouped in any way > > that makes sense due to functionality, not language. > > I agree with miu@ and my comment in > > media/base/android/media_jni_registrar.cc > > that these files should go elsewhere. > > FWIW, in general the corresponding file for src/foo/bar/boo/far/xyz.cc would > live in src/foo/android/java/src/org/chromium/foo/bar/boo/far/XYZ.java > > So in this case and all other media Java files, the paths would look like > src/media/android/java/src/org/chromium/{base,capture}/... > > However, it seems like currently Java files under media/ use a different > structure: > > src/media/foo/boo/bar.cc corresponds to > src/media/foo/boo/android/java/src/org/chromium/media/Bar.java (examples are > media/base and media/capture/video except that two Java files corresponding to > media/audio/ counterparts are placed under media/base/android/java with all the > rest -- I agree it may look confusing). > > Therefore the consistent path to this Java file seems to be > src/media/capture/content/android/java/src/org/chromium/media/ScreenCapture[Machine].java > > I agree that until the files under media follow the general rule, this makes > more sense than putting them under media/base/android/java. yes, I think the java files should go to media/capture
On 2016/05/04 21:39:13, qinmin wrote: > On 2016/05/04 20:22:39, whywhat_OOO_April_28_May_2 wrote: > > Re: the placement of the Java files. > > > > CC: Min who might have a better insight into the current placement of Java > files > > under media/ > > > > > https://codereview.chromium.org/1917023003/diff/1/media/base/android/java/src... > > File media/base/android/java/src/org/chromium/media/ScreenCapture.java > (right): > > > > > https://codereview.chromium.org/1917023003/diff/1/media/base/android/java/src... > > media/base/android/java/src/org/chromium/media/ScreenCapture.java:1: // > > Copyright 2015 The Chromium Authors. All rights reserved. > > On 2016/05/04 at 19:56:47, mcasas wrote: > > > On 2016/05/04 18:49:40, braveyao wrote: > > > > On 2016/04/27 05:24:53, miu wrote: > > > > > Hmm...these files are so far away (in the directory hierarchy) from > their > > C++ > > > > > modules. Is this common practice when doing Java in the Chromium > project? > > > > Would > > > > > it be easy to move these into media/capture/content/android/...? > > > > > > > > It seems like a common practice to gather all the JAVA files under one > place > > for > > > > each component of chromium, such as chrome/android/java, mojo/public/java, > > also > > > > media/base/android/java. As to media/base/android, once all the Java files > > for > > > > audio/video/mediacodec are located there. > > > > > > What do you mean "once all the Java files ..." > > > > > > > It's very recently that the Java files > > > > for video capture are moved to media/capture/vdieo/android, while all > others > > > > remain at same place. So I'm a little confusing about the moving of video > > > > capture. But still think it's good to put the screen capture java files > with > > > > others. WDYT? > > > > > > Java files, like any other, are grouped in any way > > > that makes sense due to functionality, not language. > > > I agree with miu@ and my comment in > > > media/base/android/media_jni_registrar.cc > > > that these files should go elsewhere. > > > > FWIW, in general the corresponding file for src/foo/bar/boo/far/xyz.cc would > > live in src/foo/android/java/src/org/chromium/foo/bar/boo/far/XYZ.java > > > > So in this case and all other media Java files, the paths would look like > > src/media/android/java/src/org/chromium/{base,capture}/... > > > > However, it seems like currently Java files under media/ use a different > > structure: > > > > src/media/foo/boo/bar.cc corresponds to > > src/media/foo/boo/android/java/src/org/chromium/media/Bar.java (examples are > > media/base and media/capture/video except that two Java files corresponding to > > media/audio/ counterparts are placed under media/base/android/java with all > the > > rest -- I agree it may look confusing). > > > > Therefore the consistent path to this Java file seems to be > > > src/media/capture/content/android/java/src/org/chromium/media/ScreenCapture[Machine].java > > > > I agree that until the files under media follow the general rule, this makes > > more sense than putting them under media/base/android/java. > > yes, I think the java files should go to media/capture Thanks so much for the clarification! This is really helpful! Will correct it soon.
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Comments on Patch Set 2: https://codereview.chromium.org/1917023003/diff/1/media/capture/content/andro... File media/capture/content/android/screen_capture_machine_android.cc (right): https://codereview.chromium.org/1917023003/diff/1/media/capture/content/andro... media/capture/content/android/screen_capture_machine_android.cc:44: bool oracle_decision = oracle_proxy_->ObserveEventAndDecideCapture( On 2016/05/04 18:49:41, braveyao wrote: > On 2016/04/27 05:24:53, miu wrote: > > I'm not familiar with the API provided by the Android platform, but would it > be > > possible for this decision to be made *before* the platform does all the work > to > > read-back the screen frame from the GPU? > > > > For example, tab capture (WebContentsVideoCaptureDevice) uses an event-driven > > "subscriber" model, where the compositor notifies it whenever compositing will > > occur, and provides it with the damage region. Then, the oracle makes a > decision > > whether it will capture the next frame. So, the expensive operation of > read-back > > only happens when it absolutely needs to (saving CPU/GPU/power, > smoother/faster > > performance, etc.). > > Android will send a copy of the whole display whenever it updates the rendering > to screen. We just wait for the incoming frames, kind of same as camera capture. > So I suppose there is no way to do the decision beforehand. Any other concern on > this? Acknowledged. Seems the API designer assumed the platform knows better than the application as to whether to do the expensive work. That's unfortunate. ;-) https://codereview.chromium.org/1917023003/diff/1/media/capture/content/andro... media/capture/content/android/screen_capture_machine_android.cc:61: libyuv::ARGBScale(source_argb, source_stride, cropWidth, cropHeight, On 2016/05/04 18:49:41, braveyao wrote: > On 2016/04/27 05:24:53, miu wrote: > > Can the platform do this? Usually scaling is much more performant in the > > GPU/compositing side of things, rather than on one thread on the CPU. > > Exactly. You can do the setting to VirtualDisplay to get required frame size, > also call resize() to resize it on the fly.In phase 1 I only guarantee the On second thought, if the frame->visible_rect() is not known until this point (because it is determined by the oracle), we always have to scale here. (The frame->visible_rect() can change dynamically during a session.) However, do consider setting the VirtualDisplay size to the maximum size requested in the media::VideoCaptureParams passed to ScreenCaptureMachineAndroid::Start(). There's no sense in copying/reading-back more data than is absolutely needed. https://codereview.chromium.org/1917023003/diff/1/media/capture/content/andro... media/capture/content/android/screen_capture_machine_android.cc:74: media::ConvertRGB32ToYUV( On 2016/05/04 18:49:41, braveyao wrote: > On 2016/04/27 05:24:53, miu wrote: > > Is it possible to push colorspace conversion upstream as well? On the Java > > side, ImageReader could be created with ImageFormat.YUV_420_888 format. > > Thanks for the advice. I didn't consider this before. With a quick test, I got a > same issue as reported in > https://code.google.com/p/android/issues/detail?id=180348 on N9, which has no > such problem on N5. > Anyway, add a TODO for a further study in phase 2. In Chromium code, we have to work on many different hardware configurations. It's typical to have to purposely write extra code to workaround issues like this. So, I would suggest you always attempt to use the ImageReader w/ YV12 output, and if you catch the exception indicated in the bug, re-create the reader in RGBA format to allow the pipeline in a "fall-back mode" where it will work at lower performance on buggy platforms. Your code here could simply be: if (data_is_in_argb) { media::ConvertRGB32ToYUV(...); } else { for each plane in (Y, U, V) { memcpy(...); } } https://codereview.chromium.org/1917023003/diff/1/media/capture/content/andro... media/capture/content/android/screen_capture_machine_android.cc:119: capture_format_.pixel_format = media::PIXEL_FORMAT_ARGB; On 2016/05/04 18:49:41, braveyao wrote: > On 2016/04/27 05:24:53, miu wrote: > > I believe the |params| would specify I420 or YV12, since that's the format of > > the VideoFrames being produced by ScreenCaptureMachineAndroid. > > Yes, |params| asks for I420. Same as above comment, I will check if we could get > MediaProjection/ImageReader work with that format in phase 2. And here we only > care about the frame size. So remove the unnecessary stuff. This is orthogonal to the other comment. The |capture_format_.pixel_format| property is referring to the format of the frames being output by this media::VideoCaptureDevice, and has nothing to do with how they are sourced from the operating system. In other words, if you get RGBA frames from Android, the client is the one requiring that they be converted to YV12 for consumption by downstream Chromium code. Actually, ScreenCaptureDeviceCore already checks for I420-only format: https://code.google.com/p/chromium/codesearch#chromium/src/media/capture/cont... Anyway, it seems you've removed this code in the latest Patch Set, so nothing to do here. https://codereview.chromium.org/1917023003/diff/1/media/capture/content/andro... media/capture/content/android/screen_capture_machine_android.cc:144: void ScreenCaptureMachineAndroid::MaybeCaptureForRefresh() {} On 2016/05/04 18:49:41, braveyao wrote: > Thanks for explaining this new method. > Android also only calls OnFrameAvailable() whenever the screen changed. So the > current consequence is the sending fps will drop to 0 in some cases. What's the > impact here? There are concerns on the consuming-end of these video frames. This is explained in detail here: https://code.google.com/p/chromium/codesearch#chromium/src/media/capture/vide... There were several important user-facing bugs resolved by this new API, so not implementing this is asking for trouble. ;-) > I see in ThreadSafeCaptureOracle::AttemptPassiveRefresh() if it returns true, > then it won't call to MaybeCaptureForRefresh(). Since Android works in passive > way too, e.g. no API to ask for one frame from it, so I think we can make it > stop there. Is that right? How can I guarantee it? No. The passive refresh will not always succeed. It depends on whether the buffer pool can "resurrect" the last frame that was sent. Timing, performance, and environmental conditions will influence that, which means it is not guaranteed on any platform. Is there an API for force-capturing the screen? Otherwise, you'll want to always hold onto the last Image's bytes so that "active" refresh can be handled correctly. > Anyway it's also a phase 2 job. Will give a further study later. How so? IIUC, "phase 1" is about implementing video frame capture and "phase 2" is about connecting that into the existing video stack framework in libcontent. https://codereview.chromium.org/1917023003/diff/60001/media/base/android/java... File media/base/android/java/src/org/chromium/media/ScreenCapture.java (right): https://codereview.chromium.org/1917023003/diff/60001/media/base/android/java... media/base/android/java/src/org/chromium/media/ScreenCapture.java:67: buffer.get(mCapturedData); This is copying the image data into a separate byte array, which is expensive. Instead, just pass the ByteBuffer and have the C++ impl read directly from that. https://codereview.chromium.org/1917023003/diff/60001/media/base/android/java... media/base/android/java/src/org/chromium/media/ScreenCapture.java:70: image.getCropRect().width(), image.getCropRect().height(), More information needs to be passed to the native impl: 1. What if the crop rect does not originate at coordinates (0,0)? The native impl needs to know it'll have to start reading at an offset from the start of the buffer. 2. What if the stride is different than the width of the image (see documentation for Plane.getRowStride())? The native impl needs to know how many bytes to skip to get to the start of the next row. Images are not always guaranteed to be "packed." https://codereview.chromium.org/1917023003/diff/60001/media/capture/content/a... File media/capture/content/android/screen_capture_machine_android.cc (right): https://codereview.chromium.org/1917023003/diff/60001/media/capture/content/a... media/capture/content/android/screen_capture_machine_android.cc:65: DCHECK(gfx::Size(cropWidth, cropHeight) == frame->visible_rect().size()); This CHECK will fail unless the client request fixed frame sizes. Having the scaling code (in your last Patch Set) made sense: Downstream consumers may have a very good reason for wanting smaller frames, as "environmental conditions" change (e.g., if the video encoder is too busy, or network bandwidth needs to be reduced).
Thanks a lot for all the comments. Did some major refactoring and got great performance improvement! Please help to take another look! To be more clear, phase 1 is mainly to propose an alternative security solution on Android() and provide a basic working demo of screensharing. So leave some comments for phase 2, in which I'll focus more on the capture functionality/feature/performance/etc., by adding TODOs. The comments help a lot for fulfilling phase2 goals, and are truly appreciated! https://codereview.chromium.org/1917023003/diff/1/media/base/android/java/src... File media/base/android/java/src/org/chromium/media/ScreenCapture.java (right): https://codereview.chromium.org/1917023003/diff/1/media/base/android/java/src... media/base/android/java/src/org/chromium/media/ScreenCapture.java:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/05/04 19:56:47, mcasas wrote: > On 2016/05/04 18:49:40, braveyao wrote: > > On 2016/04/27 05:24:53, miu wrote: > > > Hmm...these files are so far away (in the directory hierarchy) from their > C++ > > > modules. Is this common practice when doing Java in the Chromium project? > > Would > > > it be easy to move these into media/capture/content/android/...? > > > > It seems like a common practice to gather all the JAVA files under one place > for > > each component of chromium, such as chrome/android/java, mojo/public/java, > also > > media/base/android/java. As to media/base/android, once all the Java files for > > audio/video/mediacodec are located there. > > What do you mean "once all the Java files ..." > > > It's very recently that the Java files > > for video capture are moved to media/capture/vdieo/android, while all others > > remain at same place. So I'm a little confusing about the moving of video > > capture. But still think it's good to put the screen capture java files with > > others. WDYT? > > Java files, like any other, are grouped in any way > that makes sense due to functionality, not language. > I agree with miu@ and my comment in > media/base/android/media_jni_registrar.cc > that these files should go elsewhere. Done. https://codereview.chromium.org/1917023003/diff/1/media/base/android/java/src... media/base/android/java/src/org/chromium/media/ScreenCapture.java:45: private class CrImageReaderListener implements ImageReader.OnImageAvailableListener { On 2016/05/04 19:56:47, mcasas wrote: > On 2016/05/04 18:49:40, braveyao wrote: > > On 2016/04/27 01:19:26, mcasas wrote: > > > This is _very_ similar class to [1] except for the > > > synchronized part and that here we only accept > > > RGBA_8888. Please do not duplicate code, extend it > > > here. > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/media/capture/vide... > > > > As we discussed offline. I understand the concern here. But not very > comfortable > > to link ScreenCapture with VideoCaptureCamera2. It's too troublesome to extend > > it here and not good for readability as I imagine. Maybe we just keep it so > far. > > > > It's not complicated. You're probably confused as to > where to put this common class, since content/ and video/ > captures are in different folders, but in Java, everything > is reorganised as packages! (See line 5 of this file). > > You need to make a new Java file, call it > CrImageReaderListener, and make sure it goes to > package org.chromium.media, then you can import > it from any other files, including this one, and > just extend it here by doing: > > private class MyCrImageReaderListener extends CrImageReaderListener { > > @Override > public void OnImageAvailable(ImageReader reader) { > // copy here the synchronised part... > > super.OnImageAvailable( pixelFormat ) > } > } > > where you have also added a parameter pixelFormat to > CrImageReaderListener.OnImageAvailable and you're using > it in the comparison of [1]. > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/media/capture/vide... > > Tried a bit and saw your point. Added a TODO for improvement after we settle down all the buffer-format/jni-interface stuffs. This deserves a single cl. https://codereview.chromium.org/1917023003/diff/1/media/base/android/java/src... media/base/android/java/src/org/chromium/media/ScreenCapture.java:136: } catch (Exception e) { On 2016/05/04 19:56:47, mcasas wrote: > On 2016/05/04 18:49:40, braveyao wrote: > > On 2016/04/27 01:19:26, mcasas wrote: > > > No-no, don't catch generic exceptions out of a > > > large code chunk [1] (Chromium Java style guide > > > follows the Android one). > > > > > > [1] > > > > https://source.android.com/source/code-style.html#dont-catch-generic-exception > > > > Done. > > Still you need to not catch Exception, but > each individual one being thrown, like in > > https://code.google.com/p/chromium/codesearch#chromium/src/media/capture/vide... I'm aware of this. But as to the methods here, there is no specified exceptions named in the API guidance. For example, to FragmentTransaction.commit(), it only says " If the commit is attempted after that point, an exception will be thrown.", http://developer.android.com/reference/android/app/FragmentTransaction.html#c.... Then maybe catch the general exception is still a better option? https://codereview.chromium.org/1917023003/diff/1/media/capture/content/andro... File media/capture/content/android/screen_capture_machine_android.cc (right): https://codereview.chromium.org/1917023003/diff/1/media/capture/content/andro... media/capture/content/android/screen_capture_machine_android.cc:61: libyuv::ARGBScale(source_argb, source_stride, cropWidth, cropHeight, On 2016/05/10 00:29:00, miu wrote: > On 2016/05/04 18:49:41, braveyao wrote: > > On 2016/04/27 05:24:53, miu wrote: > > > Can the platform do this? Usually scaling is much more performant in the > > > GPU/compositing side of things, rather than on one thread on the CPU. > > > > Exactly. You can do the setting to VirtualDisplay to get required frame size, > > also call resize() to resize it on the fly.In phase 1 I only guarantee the > > On second thought, if the frame->visible_rect() is not known until this point > (because it is determined by the oracle), we always have to scale here. (The > frame->visible_rect() can change dynamically during a session.) > > However, do consider setting the VirtualDisplay size to the maximum size > requested in the media::VideoCaptureParams passed to > ScreenCaptureMachineAndroid::Start(). There's no sense in copying/reading-back > more data than is absolutely needed. Done. https://codereview.chromium.org/1917023003/diff/1/media/capture/content/andro... media/capture/content/android/screen_capture_machine_android.cc:74: media::ConvertRGB32ToYUV( On 2016/05/10 00:29:00, miu wrote: > On 2016/05/04 18:49:41, braveyao wrote: > > On 2016/04/27 05:24:53, miu wrote: > > > Is it possible to push colorspace conversion upstream as well? On the Java > > > side, ImageReader could be created with ImageFormat.YUV_420_888 format. > > > > Thanks for the advice. I didn't consider this before. With a quick test, I got > a > > same issue as reported in > > https://code.google.com/p/android/issues/detail?id=180348 on N9, which has no > > such problem on N5. > > Anyway, add a TODO for a further study in phase 2. > > In Chromium code, we have to work on many different hardware configurations. > It's typical to have to purposely write extra code to workaround issues like > this. > > So, I would suggest you always attempt to use the ImageReader w/ YV12 output, > and if you catch the exception indicated in the bug, re-create the reader in > RGBA format to allow the pipeline in a "fall-back mode" where it will work at > lower performance on buggy platforms. > > Your code here could simply be: > > if (data_is_in_argb) { > media::ConvertRGB32ToYUV(...); > } else { > for each plane in (Y, U, V) { > memcpy(...); > } > } Done. BTW: YUV420 has better performance, but worse compatibility. There is only one device in five at my hands that supports YUV. Even on a N5 android L, it will silently fail. So I have to only enable YUV on M devices. Need future investigation. https://codereview.chromium.org/1917023003/diff/1/media/capture/content/andro... media/capture/content/android/screen_capture_machine_android.cc:144: void ScreenCaptureMachineAndroid::MaybeCaptureForRefresh() {} On 2016/05/10 00:29:00, miu wrote: > On 2016/05/04 18:49:41, braveyao wrote: > > Thanks for explaining this new method. > > Android also only calls OnFrameAvailable() whenever the screen changed. So the > > current consequence is the sending fps will drop to 0 in some cases. What's > the > > impact here? > > There are concerns on the consuming-end of these video frames. This is explained > in detail here: > https://code.google.com/p/chromium/codesearch#chromium/src/media/capture/vide... > > There were several important user-facing bugs resolved by this new API, so not > implementing this is asking for trouble. ;-) > > > I see in ThreadSafeCaptureOracle::AttemptPassiveRefresh() if it returns true, > > then it won't call to MaybeCaptureForRefresh(). Since Android works in passive > > way too, e.g. no API to ask for one frame from it, so I think we can make it > > stop there. Is that right? How can I guarantee it? > > No. The passive refresh will not always succeed. It depends on whether the > buffer pool can "resurrect" the last frame that was sent. Timing, performance, > and environmental conditions will influence that, which means it is not > guaranteed on any platform. > > Is there an API for force-capturing the screen? Otherwise, you'll want to always > hold onto the last Image's bytes so that "active" refresh can be handled > correctly. > > > Anyway it's also a phase 2 job. Will give a further study later. > > How so? IIUC, "phase 1" is about implementing video frame capture and "phase 2" > is about connecting that into the existing video stack framework in libcontent. Done. https://codereview.chromium.org/1917023003/diff/60001/media/base/android/java... File media/base/android/java/src/org/chromium/media/ScreenCapture.java (right): https://codereview.chromium.org/1917023003/diff/60001/media/base/android/java... media/base/android/java/src/org/chromium/media/ScreenCapture.java:67: buffer.get(mCapturedData); On 2016/05/10 00:29:00, miu wrote: > This is copying the image data into a separate byte array, which is expensive. > Instead, just pass the ByteBuffer and have the C++ impl read directly from that. Done. https://codereview.chromium.org/1917023003/diff/60001/media/base/android/java... media/base/android/java/src/org/chromium/media/ScreenCapture.java:70: image.getCropRect().width(), image.getCropRect().height(), On 2016/05/10 00:29:00, miu wrote: > More information needs to be passed to the native impl: > > 1. What if the crop rect does not originate at coordinates (0,0)? The native > impl needs to know it'll have to start reading at an offset from the start of > the buffer. > > 2. What if the stride is different than the width of the image (see > documentation for Plane.getRowStride())? The native impl needs to know how many > bytes to skip to get to the start of the next row. Images are not always > guaranteed to be "packed." Yes you're right. I meet a case that stride is different than the width of the image when I tried more capture size. Thanks! Most probably there isn't cropping here, at least for now. Add a TODO for future investigation. https://codereview.chromium.org/1917023003/diff/60001/media/base/android/java... media/base/android/java/src/org/chromium/media/ScreenCapture.java:227: nativeOnActivityResult(mNativeScreenCaptureMachineAndroid, result); On 2016/05/04 19:56:48, mcasas wrote: > s/result/resultCode == Activity.RESULT_OK/ ? Done.
FYI--Will get to this first thing Thursday (demo'ing at IO today).
https://codereview.chromium.org/1917023003/diff/80001/content/browser/media/c... File content/browser/media/capture/screen_capture_device_android.cc (right): https://codereview.chromium.org/1917023003/diff/80001/content/browser/media/c... content/browser/media/capture/screen_capture_device_android.cc:24: return std::unique_ptr<media::VideoCaptureDevice>( Consider base::WrapUnique(), however see my next comment. https://codereview.chromium.org/1917023003/diff/80001/content/browser/media/c... content/browser/media/capture/screen_capture_device_android.cc:25: new ScreenCaptureDeviceAndroid()); If we're just returning a new ScreenCaptureDeviceAndroid, you could just remove this method altogether and have it created directly at the caller site. https://codereview.chromium.org/1917023003/diff/80001/content/browser/media/c... content/browser/media/capture/screen_capture_device_android.cc:39: .release()); Strange somersault? Consider: power_save_blocker_ = std::move(PowerSaverBlocker::Create(...)); Also, no need for content:: since you are in that namespace. https://codereview.chromium.org/1917023003/diff/80001/content/browser/media/c... File content/browser/media/capture/screen_capture_device_android.h (right): https://codereview.chromium.org/1917023003/diff/80001/content/browser/media/c... content/browser/media/capture/screen_capture_device_android.h:16: class ScreenCaptureDeviceAndroid : public media::VideoCaptureDevice { Class comments? Sth like: SCDAndroid is a forwarder to SCDCore while keeping the Power Saving from kicking in between AllocateAndStart() and StopAndDeallocate(). https://codereview.chromium.org/1917023003/diff/80001/content/browser/media/c... content/browser/media/capture/screen_capture_device_android.h:30: std::unique_ptr<media::ScreenCaptureDeviceCore> core_; const. Actually just make it a member: media::ScreenCaptureDeviceCore core_; since anyway creating ScreenCaptureDeviceAndroid already implies we want to go ahead with the capture, there's no benefit in initializing on the heap. https://codereview.chromium.org/1917023003/diff/80001/content/browser/media/c... content/browser/media/capture/screen_capture_device_android.h:32: // The capture device. Remove superfluous comment. https://codereview.chromium.org/1917023003/diff/80001/content/browser/media/c... content/browser/media/capture/screen_capture_device_android.h:33: std::unique_ptr<media::VideoCaptureDevice> video_capture_device_; Unused member?? Remove. https://codereview.chromium.org/1917023003/diff/80001/content/browser/media/c... content/browser/media/capture/screen_capture_device_android.h:36: // screen from sleeping for the drive-by web. I'm surprised you add comments for a 3rd person :) Escape variable names in comments using ||. TODOs should have a crbug.com https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... File media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java (right): https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java:47: // if we can extend from it. Add a bug. https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java:59: if (image == null) return; Also check that |mFormat == image.getFormat()| ? https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java:67: // TODO(braveyao): check if we need to consider corping. s/corping/cropping/ All TODO()s should have a bug. https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java:90: image.getPlanes()[1].getRowStride() / 2, Why / 2 ? The stride of a subsampled plane should already be subsampled, right? Is this based on observation? Seeing the treatment you do of the data in the .cc file, I'd say you're getting NV12, where U and V planes are interlaced in a single buffer. https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java:170: } catch (Exception e) { Again, no catching generic Exceptions. Also, the try {} is too large: I see e.g. getFragmentManager() does not throw [1] (and if it does, then add a comment that the docs are incorrect and isolate it into a try-catch on its own). I only see commit() throwing [2]. If you cannot find docs on a particular exception being thrown, chances are it's a RuntimeExceptio. [1] https://developer.android.com/reference/android/app/Activity.html#getFragment... [2] https://developer.android.com/reference/android/app/FragmentTransaction.html#... https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java:179: } catch (Exception e) { Ditto, narrow down the Exception and the try{} block. From the docs, I don't see any of the statements in l.175-178 throwing. Which one do you see throwing? https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java:184: // Factory methods. s/methods/method/ ? https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java:202: // TODO(braveyao): remove this method after the minSdkVersion of chromium is 23. s/chromium/chrome/ [1] bug for this. [1] https://www.chromium.org/developers/coding-style#TOC-Naming https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java:253: if (requestCode == REQUEST_MEDIA_PROJECTION) { Prefer early return: if (requestCode != REQUEST_MEDIA_PROJECTION) return; // the rest... https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java:254: boolean result; Unused variable? Remove. https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java:281: mThread = new HandlerThread("Screen"); s/Screen/ScreenCapture/ ? https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... File media/capture/content/android/screen_capture_jni_registrar.cc (right): https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/screen_capture_jni_registrar.cc:20: bool RegisterScreenCaptureJni(JNIEnv* env) { To avoid this incorrect diff-ing, upload with git cl upload --similarity=90 (or any other high similarity value). https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... File media/capture/content/android/screen_capture_machine_android.cc (right): https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/screen_capture_machine_android.cc:61: int source_stride = rowStride; Why not use |rowStride| directly? Consider renaming the parameter, if |rowStride| is not to your liking. https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/screen_capture_machine_android.cc:79: ConvertRGB32ToYUV( Why not use libyuv here as well [1] ? [1] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libyuv... https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/screen_capture_machine_android.cc:84: frame->stride(VideoFrame::kYPlane), frame->stride(VideoFrame::kUPlane)); It'd be possible that the if{} in l.64-74 would be executed, leaving the scaled frame in |frame|, and then we'd still be doing l.79 and overwriting the previously scaled |frame|, right? https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/screen_capture_machine_android.cc:104: uint64_t absolute_micro = const https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/screen_capture_machine_android.cc:111: bool oracle_decision = oracle_proxy_->ObserveEventAndDecideCapture( const? Or fold it into l.114. https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/screen_capture_machine_android.cc:169: if (j_capture_.obj() == NULL) { s/NULL/nullptr/ https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/screen_capture_machine_android.cc:176: DCHECK(params.requested_format.frame_size.GetArea() > 0); No need for |> 0|. https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/screen_capture_machine_android.cc:180: jboolean ret = Java_ScreenCapture_startPrompt( const https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/screen_capture_machine_android.cc:184: if (!ret) { Substitute l.184-189 with callback.Run(ret); https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/screen_capture_machine_android.cc:202: if (lastFrame_.get() == NULL) s/NULL/nullptr/ I think you can also say if(!lastFrame_)... https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/screen_capture_machine_android.cc:212: // Deliver the last VideoFrame. nit: Suggest something like: // Deliver again the last captured VideoFrame. https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... File media/capture/content/android/screen_capture_machine_android.h (right): https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/screen_capture_machine_android.h:17: // ScreenCaptureMachineAndroid captures 32bit RGB using SurfaceFlinger. Or YUV420 triplanar? I didn't see SurfaceFlinger references in the Java file though. https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/screen_capture_machine_android.h:20: public base::SupportsWeakPtr<ScreenCaptureMachineAndroid> { Hmm considering the notes in [1], I'd say it's best to not make this class a base::SupportsWeakPtr<> and instead have a member WeakPtrFactory, with a public method GetWeakPtr(). [1] https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p... https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/screen_capture_machine_android.h:29: // Implement org.chromium.media.ScreenCapture.nativeOnFrameAvailable. s/nativeOnFrameAvailable/nativeOnRGBAFrameAvailable/ ? Also correct l.37. I'm surprised this worked correctly: I thought the bindings actually needed to have a callback correctly identified by name, how you tried this last PS?
Patchset #4 (id:100001) has been deleted
Thanks so much! All comments are addressed and refactored the screen_capture_machine_android.cc a bit for the ARGB scaling issue. Please help to take another look. https://codereview.chromium.org/1917023003/diff/80001/content/browser/media/c... File content/browser/media/capture/screen_capture_device_android.cc (right): https://codereview.chromium.org/1917023003/diff/80001/content/browser/media/c... content/browser/media/capture/screen_capture_device_android.cc:24: return std::unique_ptr<media::VideoCaptureDevice>( On 2016/05/19 19:11:39, mcasas wrote: > Consider base::WrapUnique(), however see my > next comment. Done. https://codereview.chromium.org/1917023003/diff/80001/content/browser/media/c... content/browser/media/capture/screen_capture_device_android.cc:25: new ScreenCaptureDeviceAndroid()); On 2016/05/19 19:11:39, mcasas wrote: > If we're just returning a new ScreenCaptureDeviceAndroid, > you could just remove this method altogether and have it > created directly at the caller site. In video_capture_manager.cc, all other capture instances are created by a 'Create' factory method. So just keep the consistence here too :) https://codereview.chromium.org/1917023003/diff/80001/content/browser/media/c... content/browser/media/capture/screen_capture_device_android.cc:39: .release()); On 2016/05/19 19:11:39, mcasas wrote: > Strange somersault? Consider: > > power_save_blocker_ = std::move(PowerSaverBlocker::Create(...)); > > Also, no need for content:: since you are in that > namespace. Done. I see "power_save_blocker_ = PowerSaverBlocker::Create(...)" and "power_save_blocker_.reset(PowerSaverBlocker::Create(...))" in half-half chance. Seem std::move can't be applied here. https://codereview.chromium.org/1917023003/diff/80001/content/browser/media/c... File content/browser/media/capture/screen_capture_device_android.h (right): https://codereview.chromium.org/1917023003/diff/80001/content/browser/media/c... content/browser/media/capture/screen_capture_device_android.h:16: class ScreenCaptureDeviceAndroid : public media::VideoCaptureDevice { On 2016/05/19 19:11:39, mcasas wrote: > Class comments? Sth like: SCDAndroid is a forwarder to > SCDCore while keeping the Power Saving from kicking in > between AllocateAndStart() and StopAndDeallocate(). Done. https://codereview.chromium.org/1917023003/diff/80001/content/browser/media/c... content/browser/media/capture/screen_capture_device_android.h:30: std::unique_ptr<media::ScreenCaptureDeviceCore> core_; On 2016/05/19 19:11:40, mcasas wrote: > const. Actually just make it a member: > media::ScreenCaptureDeviceCore core_; > since anyway creating ScreenCaptureDeviceAndroid > already implies we want to go ahead with the capture, > there's no benefit in initializing on the heap. Done the const part. Not quite understand the rest. Just keep same as other capture implementations. https://codereview.chromium.org/1917023003/diff/80001/content/browser/media/c... content/browser/media/capture/screen_capture_device_android.h:32: // The capture device. On 2016/05/19 19:11:40, mcasas wrote: > Remove superfluous comment. Done. https://codereview.chromium.org/1917023003/diff/80001/content/browser/media/c... content/browser/media/capture/screen_capture_device_android.h:33: std::unique_ptr<media::VideoCaptureDevice> video_capture_device_; On 2016/05/19 19:11:40, mcasas wrote: > Unused member?? Remove. Done. https://codereview.chromium.org/1917023003/diff/80001/content/browser/media/c... content/browser/media/capture/screen_capture_device_android.h:36: // screen from sleeping for the drive-by web. On 2016/05/19 19:11:40, mcasas wrote: > I'm surprised you add comments for a 3rd person :) > Escape variable names in comments using ||. > TODOs should have a http://crbug.com This is merely from aura_window_capture_machine implementation. No idea what's the plan here. So just remove the comment. https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... File media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java (right): https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java:47: // if we can extend from it. On 2016/05/19 19:11:40, mcasas wrote: > Add a bug. Done. I'll eliminate all the TODOs in phase 2 for crbug/487935. So just add it for all the TODOs here. https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java:59: if (image == null) return; On 2016/05/19 19:11:41, mcasas wrote: > Also check that |mFormat == image.getFormat()| ? I move it to "switch()" below. https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java:67: // TODO(braveyao): check if we need to consider corping. On 2016/05/19 19:11:40, mcasas wrote: > s/corping/cropping/ > All TODO()s should have a bug. Done. https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java:90: image.getPlanes()[1].getRowStride() / 2, On 2016/05/19 19:11:41, mcasas wrote: > Why / 2 ? The stride of a subsampled plane should > already be subsampled, right? Is this based on > observation? Seeing the treatment you do of the > data in the .cc file, I'd say you're getting NV12, > where U and V planes are interlaced in a single > buffer. U/V plane should have half width/stride of Y plane. But actually the rowStride of all 3 planes are same. I guess the reason is the pixelStride of U/V plane is 2 from OS. See the comments in VideoCaptureCamera2.java, https://goo.gl/8OhYUg. So I have to unpack them in the .cc file since libyuv can't handle it. Also I forgot where I read it that android getRowStride() always returns the stride of the first plane. So doing '/2' here, also avoid the trouble of operating jint in cc file. https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java:170: } catch (Exception e) { On 2016/05/19 19:11:41, mcasas wrote: > Again, no catching generic Exceptions. Also, the try {} > is too large: I see e.g. getFragmentManager() does not > throw [1] (and if it does, then add a comment that the > docs are incorrect and isolate it into a try-catch on its > own). I only see commit() throwing [2]. If you cannot > find docs on a particular exception being thrown, > chances are it's a RuntimeExceptio. > > [1] > https://developer.android.com/reference/android/app/Activity.html#getFragment... > [2] > https://developer.android.com/reference/android/app/FragmentTransaction.html#... Yes, that's what I understand too. Catching generic Exception can also catch RuntimeException. That's why I used it here. Anyway correct it to be more clear. https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java:179: } catch (Exception e) { On 2016/05/19 19:11:40, mcasas wrote: > Ditto, narrow down the Exception and the > try{} block. From the docs, I don't see any > of the statements in l.175-178 throwing. > Which one do you see throwing? Removed. No, I saw no specific exception in doc. Since doc can't list all the exceptions, as the UnsupportedOperationException for ImageReader is undocumented, so I thought it might be better to try to catch generic one :) https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java:184: // Factory methods. On 2016/05/19 19:11:40, mcasas wrote: > s/methods/method/ ? Done. https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java:202: // TODO(braveyao): remove this method after the minSdkVersion of chromium is 23. On 2016/05/19 19:11:40, mcasas wrote: > s/chromium/chrome/ [1] > bug for this. > > [1] https://www.chromium.org/developers/coding-style#TOC-Naming Thanks for reminding! Read it before but forgot apparently... https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java:253: if (requestCode == REQUEST_MEDIA_PROJECTION) { On 2016/05/19 19:11:40, mcasas wrote: > Prefer early return: > > if (requestCode != REQUEST_MEDIA_PROJECTION) return; > // the rest... Done. https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java:254: boolean result; On 2016/05/19 19:11:40, mcasas wrote: > Unused variable? Remove. Done. https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java:281: mThread = new HandlerThread("Screen"); On 2016/05/19 19:11:40, mcasas wrote: > s/Screen/ScreenCapture/ ? Done. https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... File media/capture/content/android/screen_capture_jni_registrar.cc (right): https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/screen_capture_jni_registrar.cc:20: bool RegisterScreenCaptureJni(JNIEnv* env) { On 2016/05/19 19:11:41, mcasas wrote: > To avoid this incorrect diff-ing, upload with > git cl upload --similarity=90 > > (or any other high similarity value). Will try. Good to know! https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... File media/capture/content/android/screen_capture_machine_android.cc (right): https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/screen_capture_machine_android.cc:61: int source_stride = rowStride; On 2016/05/19 19:11:41, mcasas wrote: > Why not use |rowStride| directly? Consider renaming > the parameter, if |rowStride| is not to your liking. Because the stride might be changed below at l.73. Any way, refactored both jni interface. Hope that's better. https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/screen_capture_machine_android.cc:79: ConvertRGB32ToYUV( On 2016/05/19 19:11:41, mcasas wrote: > Why not use libyuv here as well [1] ? > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libyuv... Done. https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/screen_capture_machine_android.cc:84: frame->stride(VideoFrame::kYPlane), frame->stride(VideoFrame::kUPlane)); On 2016/05/19 19:11:41, mcasas wrote: > It'd be possible that the if{} in l.64-74 would > be executed, leaving the scaled frame in |frame|, > and then we'd still be doing l.79 and overwriting > the previously scaled |frame|, right? Refactored the processing here. The previous l.64-74 has an issue. Now unified the two jni interface and reuse the code in a new func. https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/screen_capture_machine_android.cc:104: uint64_t absolute_micro = On 2016/05/19 19:11:41, mcasas wrote: > const Done. https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/screen_capture_machine_android.cc:111: bool oracle_decision = oracle_proxy_->ObserveEventAndDecideCapture( On 2016/05/19 19:11:41, mcasas wrote: > const? Or fold it into l.114. Done. https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/screen_capture_machine_android.cc:169: if (j_capture_.obj() == NULL) { On 2016/05/19 19:11:41, mcasas wrote: > s/NULL/nullptr/ Done. https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/screen_capture_machine_android.cc:176: DCHECK(params.requested_format.frame_size.GetArea() > 0); On 2016/05/19 19:11:41, mcasas wrote: > No need for |> 0|. Done. https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/screen_capture_machine_android.cc:180: jboolean ret = Java_ScreenCapture_startPrompt( On 2016/05/19 19:11:41, mcasas wrote: > const Done. https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/screen_capture_machine_android.cc:184: if (!ret) { On 2016/05/19 19:11:41, mcasas wrote: > Substitute l.184-189 with > callback.Run(ret); Done. https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/screen_capture_machine_android.cc:202: if (lastFrame_.get() == NULL) On 2016/05/19 19:11:41, mcasas wrote: > s/NULL/nullptr/ > > I think you can also say if(!lastFrame_)... Done. https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/screen_capture_machine_android.cc:212: // Deliver the last VideoFrame. On 2016/05/19 19:11:41, mcasas wrote: > nit: Suggest something like: > // Deliver again the last captured VideoFrame. Done. https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... File media/capture/content/android/screen_capture_machine_android.h (right): https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/screen_capture_machine_android.h:17: // ScreenCaptureMachineAndroid captures 32bit RGB using SurfaceFlinger. On 2016/05/19 19:11:41, mcasas wrote: > Or YUV420 triplanar? I didn't see SurfaceFlinger > references in the Java file though. Done. https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/screen_capture_machine_android.h:20: public base::SupportsWeakPtr<ScreenCaptureMachineAndroid> { On 2016/05/19 19:11:41, mcasas wrote: > Hmm considering the notes in [1], I'd say it's best > to not make this class a base::SupportsWeakPtr<> > and instead have a member WeakPtrFactory, with > a public method GetWeakPtr(). > > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p... Checked again, maybe we don't need a weakptr here at all. Removed. https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/screen_capture_machine_android.h:29: // Implement org.chromium.media.ScreenCapture.nativeOnFrameAvailable. On 2016/05/19 19:11:41, mcasas wrote: > s/nativeOnFrameAvailable/nativeOnRGBAFrameAvailable/ ? > Also correct l.37. > > I'm surprised this worked correctly: I thought the bindings > actually needed to have a callback correctly identified by > name, how you tried this last PS? Shame on me... forgot to modify the comments accordingly. And it works well. What's the concern here?
https://codereview.chromium.org/1917023003/diff/80001/content/browser/media/c... File content/browser/media/capture/screen_capture_device_android.cc (right): https://codereview.chromium.org/1917023003/diff/80001/content/browser/media/c... content/browser/media/capture/screen_capture_device_android.cc:39: .release()); On 2016/05/20 22:27:24, braveyao wrote: > On 2016/05/19 19:11:39, mcasas wrote: > > Strange somersault? Consider: > > > > power_save_blocker_ = std::move(PowerSaverBlocker::Create(...)); > > > > Also, no need for content:: since you are in that > > namespace. > > Done. > > I see "power_save_blocker_ = PowerSaverBlocker::Create(...)" and > "power_save_blocker_.reset(PowerSaverBlocker::Create(...))" in half-half chance. > Seem std::move can't be applied here. I don't understand this response. https://codereview.chromium.org/1917023003/diff/80001/content/browser/media/c... File content/browser/media/capture/screen_capture_device_android.h (right): https://codereview.chromium.org/1917023003/diff/80001/content/browser/media/c... content/browser/media/capture/screen_capture_device_android.h:30: std::unique_ptr<media::ScreenCaptureDeviceCore> core_; On 2016/05/20 22:27:25, braveyao wrote: > On 2016/05/19 19:11:40, mcasas wrote: > > const. Actually just make it a member: > > media::ScreenCaptureDeviceCore core_; > > since anyway creating ScreenCaptureDeviceAndroid > > already implies we want to go ahead with the capture, > > there's no benefit in initializing on the heap. > > Done the const part. > Not quite understand the rest. Just keep same as other capture implementations. I'm saying that you should make it media::ScreenCaptureDeviceCore core_; instead of allocating it on constructor. https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... File media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java (right): https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java:170: } catch (Exception e) { On 2016/05/20 22:27:25, braveyao wrote: > On 2016/05/19 19:11:41, mcasas wrote: > > Again, no catching generic Exceptions. Also, the try {} > > is too large: I see e.g. getFragmentManager() does not > > throw [1] (and if it does, then add a comment that the > > docs are incorrect and isolate it into a try-catch on its > > own). I only see commit() throwing [2]. If you cannot > > find docs on a particular exception being thrown, > > chances are it's a RuntimeExceptio. > > > > [1] > > > https://developer.android.com/reference/android/app/Activity.html#getFragment... > > [2] > > > https://developer.android.com/reference/android/app/FragmentTransaction.html#... > > Yes, that's what I understand too. Catching generic Exception can also catch > RuntimeException. That's why I used it here. Anyway correct it to be more clear. RuntimeException <is a> Exception, and you should catch the most narrow exception available, hence catching a concrete exception is better than catching a RuntimeException is better than catching an Exception (which you should never do). https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java:179: } catch (Exception e) { On 2016/05/20 22:27:25, braveyao wrote: > On 2016/05/19 19:11:40, mcasas wrote: > > Ditto, narrow down the Exception and the > > try{} block. From the docs, I don't see any > > of the statements in l.175-178 throwing. > > Which one do you see throwing? > Removed. > No, I saw no specific exception in doc. Since doc can't list all the exceptions, > as the UnsupportedOperationException for ImageReader is undocumented, so I > thought it might be better to try to catch generic one :) What do you mean, that the Android documentation "can't list all the exceptions"? Then how are we supposed to develop? Cr Java code cannot catch Exceptions perfunctorily. If you think you have found an Exception being thrown and not documented, then catch it _and_ add a comment: // Code blabla throws Exception despite not being // documented, see [1] and [1] _must_ be a link to the AOSP code line(s) where the Exception is thrown. http://androidxref.com/ https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... File media/capture/content/android/screen_capture_machine_android.h (right): https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/screen_capture_machine_android.h:29: // Implement org.chromium.media.ScreenCapture.nativeOnFrameAvailable. On 2016/05/20 22:27:26, braveyao wrote: > On 2016/05/19 19:11:41, mcasas wrote: > > s/nativeOnFrameAvailable/nativeOnRGBAFrameAvailable/ ? > > Also correct l.37. > > > > I'm surprised this worked correctly: I thought the bindings > > actually needed to have a callback correctly identified by > > name, how you tried this last PS? > > Shame on me... forgot to modify the comments accordingly. > And it works well. What's the concern here? I thought the gyp/gn infra uses this annotations to know which native method to map to which Java mNative... member. Maybe not? https://codereview.chromium.org/1917023003/diff/120001/media/capture/content/... File media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java (right): https://codereview.chromium.org/1917023003/diff/120001/media/capture/content/... media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java:47: // if we can extend from it in phase 2 for crbug/487935. https:// prepending all the URLs please. Remove references to "phase 2", these are ephemeral comments that only pertain to your plan, and will become (are?) superfluous. https://codereview.chromium.org/1917023003/diff/120001/media/capture/content/... media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java:68: // crbug/487935. What about: // TODO(braveyao): consider adding cropping, https://crbug.com/487935. Be succint (that means, be brief and to the point). Be informative. Only say what an educated developer can't find out of context or common sense. For example, "check if we need to consider" is too long an conveys little information. Also, it doesn't say what's the criteria for deciding if we want to add cropping or not. Please review your comments from that perspective. https://codereview.chromium.org/1917023003/diff/120001/media/capture/content/... media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java:200: // TODO(braveyao): remove this method after the minSdkVersion of chrome is 23. I still see no https://crbug.com/xyzxyz link. https://codereview.chromium.org/1917023003/diff/120001/media/capture/content/... File media/capture/content/android/screen_capture_machine_android.cc (right): https://codereview.chromium.org/1917023003/diff/120001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:83: y_src = reinterpret_cast<uint8_t*>(env->GetDirectBufferAddress(y_buffer)); uint8* const y_src = ... (here and for |u_src|, |v_src|, obviously). In Chromium, the '*' goes with the type. Always prefer initialization over assignment (e.g. https://www.doc.ic.ac.uk/lab/cplus/c++.rules/chap11.html) https://codereview.chromium.org/1917023003/diff/120001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:91: int uv_plane_len = (int)env->GetDirectBufferCapacity(u_buffer); Hmm if you have to unpack, then incoming data is NV12 (or NV21) [1], so please update the method names accordingly, and instead of unpacking yourself, use, again, libyuv [2], which is never worse and every so often way faster. [1] http://www.fourcc.org/yuv.php#NV12 [2] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libyuv...
Done. PTAL. https://codereview.chromium.org/1917023003/diff/80001/content/browser/media/c... File content/browser/media/capture/screen_capture_device_android.cc (right): https://codereview.chromium.org/1917023003/diff/80001/content/browser/media/c... content/browser/media/capture/screen_capture_device_android.cc:39: .release()); On 2016/05/23 18:19:55, mcasas wrote: > On 2016/05/20 22:27:24, braveyao wrote: > > On 2016/05/19 19:11:39, mcasas wrote: > > > Strange somersault? Consider: > > > > > > power_save_blocker_ = std::move(PowerSaverBlocker::Create(...)); > > > > > > Also, no need for content:: since you are in that > > > namespace. > > > > Done. > > > > I see "power_save_blocker_ = PowerSaverBlocker::Create(...)" and > > "power_save_blocker_.reset(PowerSaverBlocker::Create(...))" in half-half > chance. > > Seem std::move can't be applied here. > > I don't understand this response. Some people use "power_save_blocker_.reset(PowerSaveBlocker::Create(...))", some people use "power_save_blocker_ = PowerSaverBlocker::Create(...)". I'm OK with either of them. And adding std::move() will cause compile failure, "moving a temporary object prevents copy elision". So I don't use it here, as others do. https://codereview.chromium.org/1917023003/diff/80001/content/browser/media/c... File content/browser/media/capture/screen_capture_device_android.h (right): https://codereview.chromium.org/1917023003/diff/80001/content/browser/media/c... content/browser/media/capture/screen_capture_device_android.h:30: std::unique_ptr<media::ScreenCaptureDeviceCore> core_; On 2016/05/23 18:19:55, mcasas wrote: > On 2016/05/20 22:27:25, braveyao wrote: > > On 2016/05/19 19:11:40, mcasas wrote: > > > const. Actually just make it a member: > > > media::ScreenCaptureDeviceCore core_; > > > since anyway creating ScreenCaptureDeviceAndroid > > > already implies we want to go ahead with the capture, > > > there's no benefit in initializing on the heap. > > > > Done the const part. > > Not quite understand the rest. Just keep same as other capture > implementations. > > I'm saying that you should make it > media::ScreenCaptureDeviceCore core_; > instead of allocating it on constructor. Done. https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... File media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java (right): https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java:170: } catch (Exception e) { On 2016/05/23 18:19:55, mcasas wrote: > On 2016/05/20 22:27:25, braveyao wrote: > > On 2016/05/19 19:11:41, mcasas wrote: > > > Again, no catching generic Exceptions. Also, the try {} > > > is too large: I see e.g. getFragmentManager() does not > > > throw [1] (and if it does, then add a comment that the > > > docs are incorrect and isolate it into a try-catch on its > > > own). I only see commit() throwing [2]. If you cannot > > > find docs on a particular exception being thrown, > > > chances are it's a RuntimeExceptio. > > > > > > [1] > > > > > > https://developer.android.com/reference/android/app/Activity.html#getFragment... > > > [2] > > > > > > https://developer.android.com/reference/android/app/FragmentTransaction.html#... > > > > Yes, that's what I understand too. Catching generic Exception can also catch > > RuntimeException. That's why I used it here. Anyway correct it to be more > clear. > > RuntimeException <is a> Exception, and you > should catch the most narrow exception > available, hence catching a concrete > exception is better than catching a > RuntimeException is better than catching an > Exception (which you should never do). Acknowledged. https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java:179: } catch (Exception e) { On 2016/05/23 18:19:55, mcasas wrote: > On 2016/05/20 22:27:25, braveyao wrote: > > On 2016/05/19 19:11:40, mcasas wrote: > > > Ditto, narrow down the Exception and the > > > try{} block. From the docs, I don't see any > > > of the statements in l.175-178 throwing. > > > Which one do you see throwing? > > Removed. > > No, I saw no specific exception in doc. Since doc can't list all the > exceptions, > > as the UnsupportedOperationException for ImageReader is undocumented, so I > > thought it might be better to try to catch generic one :) > > What do you mean, that the Android documentation > "can't list all the exceptions"? Then how are we > supposed to develop? Cr Java code cannot catch > Exceptions perfunctorily. > > If you think you have found an Exception being > thrown and not documented, then catch it _and_ > add a comment: > // Code blabla throws Exception despite not being > // documented, see [1] > and [1] _must_ be a link to the AOSP code line(s) > where the Exception is thrown. http://androidxref.com/ Acknowledged. https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... File media/capture/content/android/screen_capture_machine_android.h (right): https://codereview.chromium.org/1917023003/diff/80001/media/capture/content/a... media/capture/content/android/screen_capture_machine_android.h:29: // Implement org.chromium.media.ScreenCapture.nativeOnFrameAvailable. On 2016/05/23 18:19:55, mcasas wrote: > On 2016/05/20 22:27:26, braveyao wrote: > > On 2016/05/19 19:11:41, mcasas wrote: > > > s/nativeOnFrameAvailable/nativeOnRGBAFrameAvailable/ ? > > > Also correct l.37. > > > > > > I'm surprised this worked correctly: I thought the bindings > > > actually needed to have a callback correctly identified by > > > name, how you tried this last PS? > > > > Shame on me... forgot to modify the comments accordingly. > > And it works well. What's the concern here? > > I thought the gyp/gn infra uses this annotations > to know which native method to map to which > Java mNative... member. Maybe not? Acknowledged. https://codereview.chromium.org/1917023003/diff/120001/media/capture/content/... File media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java (right): https://codereview.chromium.org/1917023003/diff/120001/media/capture/content/... media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java:47: // if we can extend from it in phase 2 for crbug/487935. On 2016/05/23 18:19:55, mcasas wrote: > https:// prepending all the URLs please. > Remove references to "phase 2", these are > ephemeral comments that only pertain to your > plan, and will become (are?) superfluous. Done. https://codereview.chromium.org/1917023003/diff/120001/media/capture/content/... media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java:68: // crbug/487935. On 2016/05/23 18:19:55, mcasas wrote: > What about: > // TODO(braveyao): consider adding cropping, https://crbug.com/487935. > > Be succint (that means, be brief and to the point). > Be informative. Only say what an educated developer > can't find out of context or common sense. For example, > "check if we need to consider" is too long an conveys > little information. Also, it doesn't say what's the criteria > for deciding if we want to add cropping or not. Please > review your comments from that perspective. > Done. https://codereview.chromium.org/1917023003/diff/120001/media/capture/content/... media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java:200: // TODO(braveyao): remove this method after the minSdkVersion of chrome is 23. On 2016/05/23 18:19:55, mcasas wrote: > I still see no https://crbug.com/xyzxyz link. Done. https://codereview.chromium.org/1917023003/diff/120001/media/capture/content/... File media/capture/content/android/screen_capture_machine_android.cc (right): https://codereview.chromium.org/1917023003/diff/120001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:83: y_src = reinterpret_cast<uint8_t*>(env->GetDirectBufferAddress(y_buffer)); On 2016/05/23 18:19:55, mcasas wrote: > uint8* const y_src = ... > (here and for |u_src|, |v_src|, obviously). > > In Chromium, the '*' goes with the type. > Always prefer initialization over assignment > (e.g. https://www.doc.ic.ac.uk/lab/cplus/c++.rules/chap11.html) Done. https://codereview.chromium.org/1917023003/diff/120001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:91: int uv_plane_len = (int)env->GetDirectBufferCapacity(u_buffer); On 2016/05/23 18:19:55, mcasas wrote: > Hmm if you have to unpack, then incoming data > is NV12 (or NV21) [1], so please update the method > names accordingly, and instead of unpacking > yourself, use, again, libyuv [2], which is never > worse and every so often way faster. > > > [1] http://www.fourcc.org/yuv.php#NV12 > [2] > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libyuv... Nope. libyuv::NV21ToI420 doesn't apply here. Is the camera capture output also NV12/NV21? Since they should be unpacked in the same way, so they are the same format. Anyway, any format of 4:2:0 should have a frame size as W x H x 3 / 2. But the output of camera capture and media projection is actually W x H x 4 / 2. It is confusing...
Apologies in the delays from my end. I have been fighting fires. I will take another look by EOD tomorrow.
https://codereview.chromium.org/1917023003/diff/120001/media/capture/content/... File media/capture/content/android/screen_capture_machine_android.cc (right): https://codereview.chromium.org/1917023003/diff/120001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:91: int uv_plane_len = (int)env->GetDirectBufferCapacity(u_buffer); On 2016/05/24 00:03:26, braveyao wrote: > On 2016/05/23 18:19:55, mcasas wrote: > > Hmm if you have to unpack, then incoming data > > is NV12 (or NV21) [1], so please update the method > > names accordingly, and instead of unpacking > > yourself, use, again, libyuv [2], which is never > > worse and every so often way faster. > > > > > > [1] http://www.fourcc.org/yuv.php#NV12 > > [2] > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libyuv... > > Nope. libyuv::NV21ToI420 doesn't apply here. What do you mean it doesn't apply here? I imagine you mean that you are unpacking in-place, reusing |u_src| and |v_src|, and libyuv::NV21ToI420 doesn't like that. Am I right? Then, since you'll be doing a resizing in OnIncomingFrameAvailable(), you should use libyuv::ConvertToI420 to change format and resize at once, indicate as origin pixel format libyuv::FOURCC_NV12 (or NV21). > Is the camera capture output also > NV12/NV21? Let me use code search for you: https://code.google.com/p/chromium/codesearch#chromium/src/media/capture/vide... And https://code.google.com/p/chromium/codesearch#chromium/src/media/capture/vide... the latter implying that pixel formats on Android seems to be a long standing issue. > Since they should be unpacked in the same way, so they are the same > format. I don't understand this. > Anyway, any format of 4:2:0 should have a frame size as W x H x 3 / 2. But the > output of camera capture and media projection is actually W x H x 4 / 2. It is > confusing... We need to dig deeper. If we're requesting I420 triplanar from the Android API and it's providing a biplanar NV12/NV21, then you have to, at least, write a comment on Java side detailing this circumstance. What I'm seeing reading https://developer.android.com/reference/android/graphics/ImageFormat.html#YUV... is that there are no warranties on the packing of the Chrome planes. Which is probably to accommodate different MIPI connections. It's quite likely that you'll have to support both I420 and NV12/NV21 here, by looking at the pixelStrides. https://codereview.chromium.org/1917023003/diff/140001/media/capture/content/... File media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java (right): https://codereview.chromium.org/1917023003/diff/140001/media/capture/content/... media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java:200: // https://crbug.com/614172. Yay! https://codereview.chromium.org/1917023003/diff/140001/media/capture/content/... File media/capture/content/android/screen_capture_machine_android.cc (right): https://codereview.chromium.org/1917023003/diff/140001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:88: uint8* v_src = |y_src|, |u_src| and |v_src| should be const, and uint8_t instead of uint8 (which are deprecated): uint8_t* const y_src = ... etc
Hi Miguel, Could you please help to clarify the question in the reply a bit more? https://codereview.chromium.org/1917023003/diff/120001/media/capture/content/... File media/capture/content/android/screen_capture_machine_android.cc (right): https://codereview.chromium.org/1917023003/diff/120001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:91: int uv_plane_len = (int)env->GetDirectBufferCapacity(u_buffer); On 2016/05/25 17:42:58, mcasas wrote: > On 2016/05/24 00:03:26, braveyao wrote: > > On 2016/05/23 18:19:55, mcasas wrote: > > > Hmm if you have to unpack, then incoming data > > > is NV12 (or NV21) [1], so please update the method > > > names accordingly, and instead of unpacking > > > yourself, use, again, libyuv [2], which is never > > > worse and every so often way faster. > > > > > > > > > [1] http://www.fourcc.org/yuv.php#NV12 > > > [2] > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libyuv... > > > > Nope. libyuv::NV21ToI420 doesn't apply here. > > What do you mean it doesn't apply here? > > I imagine you mean that you are unpacking in-place, > reusing |u_src| and |v_src|, and libyuv::NV21ToI420 > doesn't like that. Am I right? > > Then, since you'll be doing a resizing in > OnIncomingFrameAvailable(), you should use > libyuv::ConvertToI420 to change format and > resize at once, indicate as origin pixel format > libyuv::FOURCC_NV12 (or NV21). > > > Is the camera capture output also > > NV12/NV21? > > Let me use code search for you: > > https://code.google.com/p/chromium/codesearch#chromium/src/media/capture/vide... > > And > > https://code.google.com/p/chromium/codesearch#chromium/src/media/capture/vide... > > the latter implying that pixel formats on Android > seems to be a long standing issue. > > > Since they should be unpacked in the same way, so they are the same > > format. > > I don't understand this. > > > Anyway, any format of 4:2:0 should have a frame size as W x H x 3 / 2. But the > > output of camera capture and media projection is actually W x H x 4 / 2. It is > > confusing... > > We need to dig deeper. If we're requesting I420 > triplanar from the Android API and it's providing > a biplanar NV12/NV21, then you have to, at least, > write a comment on Java side detailing this > circumstance. > > What I'm seeing reading > https://developer.android.com/reference/android/graphics/ImageFormat.html#YUV... > is that there are no warranties on the packing of > the Chrome planes. Which is probably to > accommodate different MIPI connections. It's quite > likely that you'll have to support both I420 and > NV12/NV21 here, by looking at the pixelStrides. I don't understand how it could be NV12/NV21, which suppose to have one y plane and one interleaved uv plane. What we get is a triplanar image from Android, and no matter you put the unpacked U or V plane as the UV plane into NV21ToI420(), no correct I420 frame can be got. https://codereview.chromium.org/1917023003/diff/140001/media/capture/content/... File media/capture/content/android/screen_capture_machine_android.cc (right): https://codereview.chromium.org/1917023003/diff/140001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:88: uint8* v_src = On 2016/05/25 17:42:58, mcasas wrote: > |y_src|, |u_src| and |v_src| should be const, and uint8_t instead of uint8 > (which are deprecated): > > uint8_t* const y_src = ... > etc Will do.
Rebase to use the new CreatePowerSaveBlocker() method. And pass the pixelStride of UV plane to native to make the processing more general. PTAL. https://codereview.chromium.org/1917023003/diff/140001/media/capture/content/... File media/capture/content/android/screen_capture_machine_android.cc (right): https://codereview.chromium.org/1917023003/diff/140001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:88: uint8* v_src = On 2016/05/25 19:00:15, braveyao wrote: > On 2016/05/25 17:42:58, mcasas wrote: > > |y_src|, |u_src| and |v_src| should be const, and uint8_t instead of uint8 > > (which are deprecated): > > > > uint8_t* const y_src = ... > > etc > > Will do. Done.
https://codereview.chromium.org/1917023003/diff/160001/content/browser/media/... File content/browser/media/capture/screen_capture_device_android.h (right): https://codereview.chromium.org/1917023003/diff/160001/content/browser/media/... content/browser/media/capture/screen_capture_device_android.h:24: static std::unique_ptr<media::VideoCaptureDevice> Create(); This Create() function is superfluous. Please remove it, since otherwise it infers that something more than simple construction is occurring. https://codereview.chromium.org/1917023003/diff/160001/content/browser/media/... content/browser/media/capture/screen_capture_device_android.h:35: std::unique_ptr<PowerSaveBlocker> power_save_blocker_; I missed part of the discussion: Why is the capture device creating/owning a PowerSaveBlocker? For other screen mirroring implementations, we usually instantiate a PowerSaveBlocker only when remoting the content (i.e., when a WebRTC PeerConnection is opened). So, is this needed here? https://codereview.chromium.org/1917023003/diff/160001/media/capture/content/... File media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java (right): https://codereview.chromium.org/1917023003/diff/160001/media/capture/content/... media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java:67: // TODO(braveyao): consider adding cropping, https://crbug.com/487935. The documentation states that: "The crop rectangle specifies the region of valid pixels in the image..." This probably means that, in some cases, the hardware requires non-visible rows/columns of pixels around the image that are not displayed on the user's screen. So, you really do need to account for that in this change. Thankfully, this is really simple to do in this change. See my comment a few LOC below... https://codereview.chromium.org/1917023003/diff/160001/media/capture/content/... media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java:75: nativeOnRGBAFrameAvailable(mNativeScreenCaptureMachineAndroid, To make sure only the valid region of pixels is being read, all you need to do here is add an "offset to first pixel in buffer" argument. Also, you'd pass the width/height of the crop rect instead of the image's width/height. Putting it all together: Rect validRect = image.getCropRect(); Image.Plane imagePlane = image.getPlanes()[0]; int byteOffsetToFirstPixelInBuffer = (validRect.top * imagePlane.getRowStride()) + (validRect.left * imagePlane.getPixelStride()); nativeOnRGBAFrameAvailable(mNativeScreenCaptureMachineAndroid, imagePlane.getBuffer(), byteOffsetToFirstPixelInBuffer, // NEW ARGUMENT validRect.width, validRect.height, imagePlane.getRowStride(), image.getTimestamp()); https://codereview.chromium.org/1917023003/diff/160001/media/capture/content/... media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java:88: nativeOnI420FrameAvailable(mNativeScreenCaptureMachineAndroid, Accounting for the crop rect here is similar to the above, except you have to compute the buffer offset for each plane, and ROUND_UP(cropRect.width / 2.0) and ROUND_UP(cropRect.height / 2.0) for the U and V plane widths/heights. https://codereview.chromium.org/1917023003/diff/160001/media/capture/content/... File media/capture/content/android/screen_capture_jni_registrar.cc (right): https://codereview.chromium.org/1917023003/diff/160001/media/capture/content/... media/capture/content/android/screen_capture_jni_registrar.cc:15: static base::android::RegistrationMethod kCaptureRegisteredMethods[] = { s/static/static const/ https://codereview.chromium.org/1917023003/diff/160001/media/capture/content/... File media/capture/content/android/screen_capture_machine_android.cc (right): https://codereview.chromium.org/1917023003/diff/160001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:95: int uv_plane_len = (int)env->GetDirectBufferCapacity(u_buffer); Please add: CHECK_EQ(uv_pixel_stride, 2); (just in case some other platform unexpectedly decides to use some pixel stride other than 1 or 2) https://codereview.chromium.org/1917023003/diff/160001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:184: base::TimeTicks::FromInternalValue(absolute_micro); Never use To/FromInternalValue() in Chromium code, unless you're serializing. Here, you should be using base::TimeTicks::FromMicroseconds(absolute_micro).
https://codereview.chromium.org/1917023003/diff/120001/media/capture/content/... File media/capture/content/android/screen_capture_machine_android.cc (right): https://codereview.chromium.org/1917023003/diff/120001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:91: int uv_plane_len = (int)env->GetDirectBufferCapacity(u_buffer); On 2016/05/25 19:00:15, braveyao wrote: > On 2016/05/25 17:42:58, mcasas wrote: > > On 2016/05/24 00:03:26, braveyao wrote: > > > On 2016/05/23 18:19:55, mcasas wrote: > > > > Hmm if you have to unpack, then incoming data > > > > is NV12 (or NV21) [1], so please update the method > > > > names accordingly, and instead of unpacking > > > > yourself, use, again, libyuv [2], which is never > > > > worse and every so often way faster. > > > > > > > > > > > > [1] http://www.fourcc.org/yuv.php#NV12 > > > > [2] > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libyuv... > > > > > > Nope. libyuv::NV21ToI420 doesn't apply here. > > > > What do you mean it doesn't apply here? > > > > I imagine you mean that you are unpacking in-place, > > reusing |u_src| and |v_src|, and libyuv::NV21ToI420 > > doesn't like that. Am I right? > > > > Then, since you'll be doing a resizing in > > OnIncomingFrameAvailable(), you should use > > libyuv::ConvertToI420 to change format and > > resize at once, indicate as origin pixel format > > libyuv::FOURCC_NV12 (or NV21). > > > > > Is the camera capture output also > > > NV12/NV21? > > > > Let me use code search for you: > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/media/capture/vide... > > > > And > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/media/capture/vide... > > > > the latter implying that pixel formats on Android > > seems to be a long standing issue. > > > > > Since they should be unpacked in the same way, so they are the same > > > format. > > > > I don't understand this. > > > > > Anyway, any format of 4:2:0 should have a frame size as W x H x 3 / 2. But > the > > > output of camera capture and media projection is actually W x H x 4 / 2. It > is > > > confusing... > > > > We need to dig deeper. If we're requesting I420 > > triplanar from the Android API and it's providing > > a biplanar NV12/NV21, then you have to, at least, > > write a comment on Java side detailing this > > circumstance. > > > > What I'm seeing reading > > > https://developer.android.com/reference/android/graphics/ImageFormat.html#YUV... > > is that there are no warranties on the packing of > > the Chrome planes. Which is probably to > > accommodate different MIPI connections. It's quite > > likely that you'll have to support both I420 and > > NV12/NV21 here, by looking at the pixelStrides. > > I don't understand how it could be NV12/NV21, which suppose to have one y plane > and one interleaved uv plane. What we get is a triplanar image from Android, and > no matter you put the unpacked U or V plane as the UV plane into NV21ToI420(), > no correct I420 frame can be got. NV12: 2 pointers: y_plane_nv12 => yyyyyyyyyyy... uv_plane_nv12 => uvuvuvuvuv... what you're getting (possibly) is: y_plane_yuv == y_plane_nv12 u_plane_yuv == uv_plane_nv12 v_plane_yuvv == uv_plane_nv12 +1 and then you get told that the pixel stride (not the row stride) for |u_plane_yuv| and |v_plane_yuv| is 2, so you skip 1 pixel-channel every two. Have you checked the addresses for u and v planes to either verify or refute this theory? Also I commented elsewhere that doing the deinterlacing UV here by hand and then an I420Scale is wasteful, please dig deeper in the libyuv library, or consult fbarchard@.
On 2016/06/07 22:23:19, mcasas wrote: > https://codereview.chromium.org/1917023003/diff/120001/media/capture/content/... > File media/capture/content/android/screen_capture_machine_android.cc (right): > > https://codereview.chromium.org/1917023003/diff/120001/media/capture/content/... > media/capture/content/android/screen_capture_machine_android.cc:91: int > uv_plane_len = (int)env->GetDirectBufferCapacity(u_buffer); > On 2016/05/25 19:00:15, braveyao wrote: > > On 2016/05/25 17:42:58, mcasas wrote: > > > On 2016/05/24 00:03:26, braveyao wrote: > > > > On 2016/05/23 18:19:55, mcasas wrote: > > > > > Hmm if you have to unpack, then incoming data > > > > > is NV12 (or NV21) [1], so please update the method > > > > > names accordingly, and instead of unpacking > > > > > yourself, use, again, libyuv [2], which is never > > > > > worse and every so often way faster. > > > > > > > > > > > > > > > [1] http://www.fourcc.org/yuv.php#NV12 > > > > > [2] > > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libyuv... > > > > > > > > Nope. libyuv::NV21ToI420 doesn't apply here. > > > > > > What do you mean it doesn't apply here? > > > > > > I imagine you mean that you are unpacking in-place, > > > reusing |u_src| and |v_src|, and libyuv::NV21ToI420 > > > doesn't like that. Am I right? > > > > > > Then, since you'll be doing a resizing in > > > OnIncomingFrameAvailable(), you should use > > > libyuv::ConvertToI420 to change format and > > > resize at once, indicate as origin pixel format > > > libyuv::FOURCC_NV12 (or NV21). > > > > > > > Is the camera capture output also > > > > NV12/NV21? > > > > > > Let me use code search for you: > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/media/capture/vide... > > > > > > And > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/media/capture/vide... > > > > > > the latter implying that pixel formats on Android > > > seems to be a long standing issue. > > > > > > > Since they should be unpacked in the same way, so they are the same > > > > format. > > > > > > I don't understand this. > > > > > > > Anyway, any format of 4:2:0 should have a frame size as W x H x 3 / 2. But > > the > > > > output of camera capture and media projection is actually W x H x 4 / 2. > It > > is > > > > confusing... > > > > > > We need to dig deeper. If we're requesting I420 > > > triplanar from the Android API and it's providing > > > a biplanar NV12/NV21, then you have to, at least, > > > write a comment on Java side detailing this > > > circumstance. > > > > > > What I'm seeing reading > > > > > > https://developer.android.com/reference/android/graphics/ImageFormat.html#YUV... > > > is that there are no warranties on the packing of > > > the Chrome planes. Which is probably to > > > accommodate different MIPI connections. It's quite > > > likely that you'll have to support both I420 and > > > NV12/NV21 here, by looking at the pixelStrides. > > > > I don't understand how it could be NV12/NV21, which suppose to have one y > plane > > and one interleaved uv plane. What we get is a triplanar image from Android, > and > > no matter you put the unpacked U or V plane as the UV plane into NV21ToI420(), > > no correct I420 frame can be got. > > NV12: > > 2 pointers: > y_plane_nv12 => yyyyyyyyyyy... > uv_plane_nv12 => uvuvuvuvuv... > > what you're getting (possibly) is: > y_plane_yuv == y_plane_nv12 > u_plane_yuv == uv_plane_nv12 > v_plane_yuvv == uv_plane_nv12 +1 > and then you get told that the pixel stride (not the > row stride) for |u_plane_yuv| and |v_plane_yuv| is 2, so > you skip 1 pixel-channel every two. > > Have you checked the addresses for u and v planes > to either verify or refute this theory? > > Also I commented elsewhere that doing the > deinterlacing UV here by hand and then an > I420Scale is wasteful, please dig deeper in the > libyuv library, or consult fbarchard@. Thanks Miguel! Miu@ has already helped me work out the myth. Yes in memory, U/V planes are interleaved as VUVUVU, as NV21 format. I must did something wrong when I tried NV21ToI420/NV12ToI420 with either u_src or v_src as uv_src input last time. Now NV21ToI420 works. Otherwise I can understand the problem earlier. Working on another patch now.
Corrected YUV420 unpacking issue and added cropping support. PTAL! https://codereview.chromium.org/1917023003/diff/160001/content/browser/media/... File content/browser/media/capture/screen_capture_device_android.h (right): https://codereview.chromium.org/1917023003/diff/160001/content/browser/media/... content/browser/media/capture/screen_capture_device_android.h:24: static std::unique_ptr<media::VideoCaptureDevice> Create(); On 2016/06/02 22:33:51, miu (OOO June 11-19) wrote: > This Create() function is superfluous. Please remove it, since otherwise it > infers that something more than simple construction is occurring. Done. https://codereview.chromium.org/1917023003/diff/160001/content/browser/media/... content/browser/media/capture/screen_capture_device_android.h:35: std::unique_ptr<PowerSaveBlocker> power_save_blocker_; On 2016/06/02 22:33:51, miu (OOO June 11-19) wrote: > I missed part of the discussion: Why is the capture device creating/owning a > PowerSaveBlocker? > > For other screen mirroring implementations, we usually instantiate a > PowerSaveBlocker only when remoting the content (i.e., when a WebRTC > PeerConnection is opened). So, is this needed here? The aura_window_capture_machine.cc (and desktop_capture_device.cc, kind of) does same, I suppose. PowerSaveBlocker is only instantiated when a stream of screen is requested for a peerconnection here too. Any better suggestion? https://codereview.chromium.org/1917023003/diff/160001/media/capture/content/... File media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java (right): https://codereview.chromium.org/1917023003/diff/160001/media/capture/content/... media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java:67: // TODO(braveyao): consider adding cropping, https://crbug.com/487935. On 2016/06/02 22:33:51, miu (OOO June 11-19) wrote: > The documentation states that: "The crop rectangle specifies the region of valid > pixels in the image..." This probably means that, in some cases, the hardware > requires non-visible rows/columns of pixels around the image that are not > displayed on the user's screen. So, you really do need to account for that in > this change. > > Thankfully, this is really simple to do in this change. See my comment a few LOC > below... So far on all the Nexus devices I've tested on, the CropRect is same as the image size. So I'm not sure whether a cropped image could be got from VirtualDisplay. But because of all your detailed comments here, I have no reason to reject it now :) https://codereview.chromium.org/1917023003/diff/160001/media/capture/content/... media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java:75: nativeOnRGBAFrameAvailable(mNativeScreenCaptureMachineAndroid, On 2016/06/02 22:33:51, miu (OOO June 11-19) wrote: > To make sure only the valid region of pixels is being read, all you need to do > here is add an "offset to first pixel in buffer" argument. Also, you'd pass the > width/height of the crop rect instead of the image's width/height. Putting it > all together: > > Rect validRect = image.getCropRect(); > Image.Plane imagePlane = image.getPlanes()[0]; > int byteOffsetToFirstPixelInBuffer = > (validRect.top * imagePlane.getRowStride()) + > (validRect.left * imagePlane.getPixelStride()); > nativeOnRGBAFrameAvailable(mNativeScreenCaptureMachineAndroid, > imagePlane.getBuffer(), > byteOffsetToFirstPixelInBuffer, // NEW ARGUMENT > validRect.width, > validRect.height, > imagePlane.getRowStride(), > image.getTimestamp()); Done. https://codereview.chromium.org/1917023003/diff/160001/media/capture/content/... media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java:88: nativeOnI420FrameAvailable(mNativeScreenCaptureMachineAndroid, On 2016/06/02 22:33:51, miu (OOO June 11-19) wrote: > Accounting for the crop rect here is similar to the above, except you have to > compute the buffer offset for each plane, and ROUND_UP(cropRect.width / 2.0) and > ROUND_UP(cropRect.height / 2.0) for the U and V plane widths/heights. Done. https://codereview.chromium.org/1917023003/diff/160001/media/capture/content/... File media/capture/content/android/screen_capture_jni_registrar.cc (right): https://codereview.chromium.org/1917023003/diff/160001/media/capture/content/... media/capture/content/android/screen_capture_jni_registrar.cc:15: static base::android::RegistrationMethod kCaptureRegisteredMethods[] = { On 2016/06/02 22:33:51, miu wrote: > s/static/static const/ Done. https://codereview.chromium.org/1917023003/diff/160001/media/capture/content/... File media/capture/content/android/screen_capture_machine_android.cc (right): https://codereview.chromium.org/1917023003/diff/160001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:95: int uv_plane_len = (int)env->GetDirectBufferCapacity(u_buffer); On 2016/06/02 22:33:51, miu wrote: > Please add: CHECK_EQ(uv_pixel_stride, 2); (just in case some other platform > unexpectedly decides to use some pixel stride other than 1 or 2) Done. https://codereview.chromium.org/1917023003/diff/160001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:184: base::TimeTicks::FromInternalValue(absolute_micro); On 2016/06/02 22:33:51, miu wrote: > Never use To/FromInternalValue() in Chromium code, unless you're serializing. > Here, you should be using base::TimeTicks::FromMicroseconds(absolute_micro). Done.
https://codereview.chromium.org/1917023003/diff/160001/content/browser/media/... File content/browser/media/capture/screen_capture_device_android.h (right): https://codereview.chromium.org/1917023003/diff/160001/content/browser/media/... content/browser/media/capture/screen_capture_device_android.h:35: std::unique_ptr<PowerSaveBlocker> power_save_blocker_; On 2016/06/08 20:39:33, braveyao wrote: > On 2016/06/02 22:33:51, miu (OOO June 11-19) wrote: > > I missed part of the discussion: Why is the capture device creating/owning a > > PowerSaveBlocker? > > > > For other screen mirroring implementations, we usually instantiate a > > PowerSaveBlocker only when remoting the content (i.e., when a WebRTC > > PeerConnection is opened). So, is this needed here? > > The aura_window_capture_machine.cc (and desktop_capture_device.cc, kind of) does > same, I suppose. PowerSaveBlocker is only instantiated when a stream of screen > is requested for a peerconnection here too. > Any better suggestion? Seems the PowerSaveBlocker should be instantiated by the PeerConnection (IIRC, it already is). The question is whether there are non-PeerConnection use cases where we'd want the blocker. IMHO, this is something the consumer should do. Meaning, the application would know whether the screen should be kept turned on and so code elsewhere should be responsible for instantiating a blocker. Said another way, the capturer cannot assume a "controller" role in the application. https://codereview.chromium.org/1917023003/diff/180001/media/capture/content/... File media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java (right): https://codereview.chromium.org/1917023003/diff/180001/media/capture/content/... media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java:342: ByteBuffer buf, int left, int top, int cropWidth, int cropHeight, int rowStride, nit: It's sufficient to just call these width and height instead of cropWidth and cropHeight (since the rowStride allows the calculation to the byte offset per row). https://codereview.chromium.org/1917023003/diff/180001/media/capture/content/... media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java:347: int uvRowStride, int uvPixelStride, int left, int top, int cropWidth, int cropHeight, nit (ditto): s/crop// https://codereview.chromium.org/1917023003/diff/180001/media/capture/content/... File media/capture/content/android/screen_capture_machine_android.cc (right): https://codereview.chromium.org/1917023003/diff/180001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:52: int offset = top * row_stride + left; offset is in number of bytes, so this should be: int offset = top * row_stride + left * 4; https://codereview.chromium.org/1917023003/diff/180001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:103: CHECK_EQ(uv_pixel_stride, 2); Now that I look at this code more closely, it turns out it'll work for ANY uv_pixel_stride. So, I was wrong to suggest putting in the CHECK_EQ() here. https://codereview.chromium.org/1917023003/diff/180001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:105: u_tmp.reset(new uint8_t[(uv_plane_len + 1) / 2]); nit: For both of these, instead of dividing by 2, you should divide by uv_pixel_stride. https://codereview.chromium.org/1917023003/diff/180001/media/capture/content/... File media/capture/content/android/screen_capture_machine_android.h (right): https://codereview.chromium.org/1917023003/diff/180001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.h:34: jint cropWidth, nit (ditto): s/crop// (and everywhere in the .cc file)
On 2016/06/07 22:23:19, mcasas wrote: > Also I commented elsewhere that doing the > deinterlacing UV here by hand and then an > I420Scale is wasteful, please dig deeper in the > libyuv library, or consult fbarchard@. libyuv doesn't expose exactly what is needed here (not without also making a copy of the Y plane). The closest thing is a row-by-row deinterlacing routine [1], but then braveyao@ would have to write code a bunch of code to: 1) detect SSE2 or NEON or neither, and then call the right flavor of the SplitUVRow_XXX() function; and then 2) write a routine to call the function for each row. [1]: https://cs.chromium.org/chromium/src/third_party/libyuv/include/libyuv/row.h?... Perhaps this could be an optimization for a follow-up change, after libyuv provides the needed code factoring for what we're doing here.
Thanks for the quick review. All comments are addressed. PTAL. https://codereview.chromium.org/1917023003/diff/160001/content/browser/media/... File content/browser/media/capture/screen_capture_device_android.h (right): https://codereview.chromium.org/1917023003/diff/160001/content/browser/media/... content/browser/media/capture/screen_capture_device_android.h:35: std::unique_ptr<PowerSaveBlocker> power_save_blocker_; On 2016/06/08 21:06:49, miu (OOO June 11-19) wrote: > On 2016/06/08 20:39:33, braveyao wrote: > > On 2016/06/02 22:33:51, miu (OOO June 11-19) wrote: > > > I missed part of the discussion: Why is the capture device creating/owning a > > > PowerSaveBlocker? > > > > > > For other screen mirroring implementations, we usually instantiate a > > > PowerSaveBlocker only when remoting the content (i.e., when a WebRTC > > > PeerConnection is opened). So, is this needed here? > > > > The aura_window_capture_machine.cc (and desktop_capture_device.cc, kind of) > does > > same, I suppose. PowerSaveBlocker is only instantiated when a stream of screen > > is requested for a peerconnection here too. > > Any better suggestion? > > Seems the PowerSaveBlocker should be instantiated by the PeerConnection (IIRC, > it already is). The question is whether there are non-PeerConnection use cases > where we'd want the blocker. > > IMHO, this is something the consumer should do. Meaning, the application would > know whether the screen should be kept turned on and so code elsewhere should be > responsible for instantiating a blocker. Said another way, the capturer cannot > assume a "controller" role in the application. Done. You are right. Peerconnection does it somewhere (Try to figure out. media_web_contents_observer.cc?). https://codereview.chromium.org/1917023003/diff/180001/media/capture/content/... File media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java (right): https://codereview.chromium.org/1917023003/diff/180001/media/capture/content/... media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java:342: ByteBuffer buf, int left, int top, int cropWidth, int cropHeight, int rowStride, On 2016/06/08 21:06:50, miu (OOO June 11-19) wrote: > nit: It's sufficient to just call these width and height instead of cropWidth > and cropHeight (since the rowStride allows the calculation to the byte offset > per row). Done. https://codereview.chromium.org/1917023003/diff/180001/media/capture/content/... media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java:347: int uvRowStride, int uvPixelStride, int left, int top, int cropWidth, int cropHeight, On 2016/06/08 21:06:50, miu (OOO June 11-19) wrote: > nit (ditto): s/crop// Done. https://codereview.chromium.org/1917023003/diff/180001/media/capture/content/... File media/capture/content/android/screen_capture_machine_android.cc (right): https://codereview.chromium.org/1917023003/diff/180001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:52: int offset = top * row_stride + left; On 2016/06/08 21:06:50, miu (OOO June 11-19) wrote: > offset is in number of bytes, so this should be: > > int offset = top * row_stride + left * 4; Done. Wrongly chose a left 20 in the test... :( Should use an odd value. https://codereview.chromium.org/1917023003/diff/180001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:103: CHECK_EQ(uv_pixel_stride, 2); On 2016/06/08 21:06:50, miu (OOO June 11-19) wrote: > Now that I look at this code more closely, it turns out it'll work for ANY > uv_pixel_stride. So, I was wrong to suggest putting in the CHECK_EQ() here. Done. https://codereview.chromium.org/1917023003/diff/180001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:105: u_tmp.reset(new uint8_t[(uv_plane_len + 1) / 2]); On 2016/06/08 21:06:50, miu (OOO June 11-19) wrote: > nit: For both of these, instead of dividing by 2, you should divide by > uv_pixel_stride. Done. https://codereview.chromium.org/1917023003/diff/180001/media/capture/content/... File media/capture/content/android/screen_capture_machine_android.h (right): https://codereview.chromium.org/1917023003/diff/180001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.h:34: jint cropWidth, On 2016/06/08 21:06:50, miu (OOO June 11-19) wrote: > nit (ditto): s/crop// (and everywhere in the .cc file) Done.
lgtm. re testing: I see that you're landing this in multiple phases, and perhaps you mean to add browser-level tests in one of the later phases, when more of the implementation is complete? Otherwise, let me throw caution into the wind right now: If you don't write tests for this code, it is doomed to be broken by other dev's future changes. ;-)
The CQ bit was checked by braveyao@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1917023003/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
braveyao@chromium.org changed reviewers: + dalecurtis@chromium.org, michaelbai@chromium.org, tedchoc@chromium.org - avayvod@chromium.org
Need reviews from owners of below files: tedchoc@ chrome/android/BUILD.gn michaelbai@ content/app/android/library_loader_hooks.cc dalecurtis@ media/BUILD.gn & media/media.gyp Thanks a lot!
On 2016/06/09 20:59:11, braveyao wrote: > Need reviews from owners of below files: > > tedchoc@ chrome/android/BUILD.gn > michaelbai@ content/app/android/library_loader_hooks.cc > dalecurtis@ media/BUILD.gn & media/media.gyp > > Thanks a lot! chrome/android/BUILD.gn - lgtm
lgtm content/app/android/library_loader_hooks.cc
mcasas@chromium.org changed reviewers: + qinmin@chromium.org
braveyao@: please wait for all reviewers to provide and finish their reviews. Also I'd like qinmin@ to take a look at media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java https://codereview.chromium.org/1917023003/diff/200001/media/capture/content/... File media/capture/content/android/screen_capture_machine_android.cc (right): https://codereview.chromium.org/1917023003/diff/200001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:51: int offset = top * row_stride + left * 4; const https://codereview.chromium.org/1917023003/diff/200001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:87: uint8_t* y_src = uint8_t* const https://codereview.chromium.org/1917023003/diff/200001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:89: CHECK(y_src); Why CHECK here and take it face-value in l.54? https://codereview.chromium.org/1917023003/diff/200001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:91: reinterpret_cast<uint8_t*>(env->GetDirectBufferAddress(u_buffer)); uint8_t* const https://codereview.chromium.org/1917023003/diff/200001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:94: reinterpret_cast<uint8_t*>(env->GetDirectBufferAddress(v_buffer)); uint8_t* const https://codereview.chromium.org/1917023003/diff/200001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:144: " Maybe android version is too low"; DLOG() and, I think this error message is not really informative. Say I'm a developer working on building Chromium, and I see this message... what does it mean "Maybe"? https://codereview.chromium.org/1917023003/diff/200001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:169: // refresh, the cached last frame will be delivered. Strange reading. Suggestions: ... in _a_ passive way and there _are_ no captured frame_s_ ...When _the_ oracle asks _for a capture refresh_, the _cached captured frame_ is redelivered. https://codereview.chromium.org/1917023003/diff/200001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:210: libyuv::I420Scale(y_src, y_stride, u_src, u_stride, v_src, v_stride, width, I don't like that we have a step of libyuv::ABGRToI420() -- or a hand made deinterlacing. Those step seem wasteful if you're going to Scale here to a smaller resolution, i.e., why not convert-and-scale at the same time? Moreover, you're allocating heap memory (l.47 or l.103-104) for _every_ incoming frame, which is tantamount to memory bashing and, in Android devices, probably not good at all. How are you addressing these concerns? Are you planning on having content_browsertests? I see miu@ has lgtm'd but at the very least, you need a Todo (with a crbug, obviously), to have another, more in depth, look at all this code. https://codereview.chromium.org/1917023003/diff/200001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:220: // Deliver the populated VideoFrame. nit: superfluous comment?
media/ lgtm
Hi Miguel, Done those trivial comments. As to the convert-and-scale comments, please check my response and help to make a call before I upload anther patch. https://codereview.chromium.org/1917023003/diff/200001/media/capture/content/... File media/capture/content/android/screen_capture_machine_android.cc (right): https://codereview.chromium.org/1917023003/diff/200001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:51: int offset = top * row_stride + left * 4; On 2016/06/12 10:11:51, mcasas wrote: > const Done. https://codereview.chromium.org/1917023003/diff/200001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:87: uint8_t* y_src = On 2016/06/12 10:11:51, mcasas wrote: > uint8_t* const Done. https://codereview.chromium.org/1917023003/diff/200001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:89: CHECK(y_src); On 2016/06/12 10:11:50, mcasas wrote: > Why CHECK here and take it face-value in l.54? Done. https://codereview.chromium.org/1917023003/diff/200001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:91: reinterpret_cast<uint8_t*>(env->GetDirectBufferAddress(u_buffer)); On 2016/06/12 10:11:51, mcasas wrote: > uint8_t* const can't be const. It will be changed below. https://codereview.chromium.org/1917023003/diff/200001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:94: reinterpret_cast<uint8_t*>(env->GetDirectBufferAddress(v_buffer)); On 2016/06/12 10:11:51, mcasas wrote: > uint8_t* const ditto https://codereview.chromium.org/1917023003/diff/200001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:144: " Maybe android version is too low"; On 2016/06/12 10:11:50, mcasas wrote: > DLOG() > > and, I think this error message is not really informative. > Say I'm a developer working on building Chromium, and > I see this message... what does it mean "Maybe"? Done. https://codereview.chromium.org/1917023003/diff/200001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:169: // refresh, the cached last frame will be delivered. On 2016/06/12 10:11:50, mcasas wrote: > Strange reading. Suggestions: > > ... in _a_ passive way and there _are_ no > captured frame_s_ ...When _the_ oracle asks > _for a capture refresh_, the _cached captured > frame_ is redelivered. Done. https://codereview.chromium.org/1917023003/diff/200001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:210: libyuv::I420Scale(y_src, y_stride, u_src, u_stride, v_src, v_stride, width, On 2016/06/12 10:11:51, mcasas wrote: > I don't like that we have a step of libyuv::ABGRToI420() > -- or a hand made deinterlacing. Those step seem > wasteful if you're going to Scale here to a smaller > resolution, i.e., why not convert-and-scale at the > same time? > > Moreover, you're allocating heap memory (l.47 or > l.103-104) for _every_ incoming frame, which is > tantamount to memory bashing and, in Android > devices, probably not good at all. > > How are you addressing these concerns? Are > you planning on having content_browsertests? > I see miu@ has lgtm'd but at the very least, you > need a Todo (with a crbug, obviously), to have > another, more in depth, look at all this code. Not that wasteful. As to OnRGBAFrameAvailable, I just want to reuse the codes in OnIncomingFrameAvailable(). So far I haven't observed any scaling happening. I420Scale() will do a plane-copy only for most of time. If you don't mind having some duplicated codes, I can make OnRGABFrameAvailable() to have the codes of OnIncomingFrameAvailable(). You call! Codes are here: void ScreenCaptureMachineAndroid::OnRGBAFrameAvailable(JNIEnv* env, jobject obj, jobject buf, jint row_stride, jint left, jint top, jint width, jint height, jlong timestamp) { uint8_t* const src = reinterpret_cast<uint8_t*>(env->GetDirectBufferAddress(buf)); CHECK(src); const VideoCaptureOracle::Event event = VideoCaptureOracle::kCompositorUpdate; const uint64_t absolute_micro = timestamp / base::Time::kNanosecondsPerMicrosecond; const base::TimeTicks start_time = base::TimeTicks() + base::TimeDelta::FromMicroseconds(absolute_micro); scoped_refptr<VideoFrame> frame; ThreadSafeCaptureOracle::CaptureFrameCallback capture_frame_cb; if (!oracle_proxy_->ObserveEventAndDecideCapture( event, gfx::Rect(), start_time, &frame, &capture_frame_cb)) { return; } DCHECK(frame->format() == PIXEL_FORMAT_I420 || frame->format() == PIXEL_FORMAT_YV12); const int offset = top * row_stride + left * 4; // ABGR little endian (rgba in memory) to I420. libyuv::ABGRToI420( src + offset, row_stride, frame->visible_data(VideoFrame::kYPlane), frame->stride(VideoFrame::kYPlane), frame->visible_data(VideoFrame::kUPlane), frame->stride(VideoFrame::kUPlane), frame->visible_data(VideoFrame::kVPlane), frame->stride(VideoFrame::kVPlane), frame->visible_rect().width(), frame->visible_rect().height()); capture_frame_cb.Run(frame, start_time, true); lastFrame_ = frame; } As to the hand-made-deinterlacing, it is better because the only thing we know for sure is that we can start at the pointers and read every other bytes from there, despite the real output format from Android (I420/NV21/NV12) which is not documented or guaranteed on all platforms. Miu@ and I had a discuss about this and both agree that hand-made-deinterlacing is more appropriate here. https://codereview.chromium.org/1917023003/diff/200001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:220: // Deliver the populated VideoFrame. On 2016/06/12 10:11:50, mcasas wrote: > nit: superfluous comment? Done.
https://codereview.chromium.org/1917023003/diff/200001/media/capture/content/... File media/capture/content/android/screen_capture_machine_android.cc (right): https://codereview.chromium.org/1917023003/diff/200001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:87: uint8_t* y_src = On 2016/06/13 22:30:32, braveyao wrote: > On 2016/06/12 10:11:51, mcasas wrote: > > uint8_t* const > > Done. I don't see the "Done"s being done. Did you forget to upload PS9? https://codereview.chromium.org/1917023003/diff/200001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:115: int uv_offset = (top / 2) * uv_stride + left / 2; l.114 and this line should be const. https://codereview.chromium.org/1917023003/diff/200001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:210: libyuv::I420Scale(y_src, y_stride, u_src, u_stride, v_src, v_stride, width, On 2016/06/13 22:30:32, braveyao wrote: > On 2016/06/12 10:11:51, mcasas wrote: > > I don't like that we have a step of libyuv::ABGRToI420() > > -- or a hand made deinterlacing. Those step seem > > wasteful if you're going to Scale here to a smaller > > resolution, i.e., why not convert-and-scale at the > > same time? > > > > Moreover, you're allocating heap memory (l.47 or > > l.103-104) for _every_ incoming frame, which is > > tantamount to memory bashing and, in Android > > devices, probably not good at all. > > > > How are you addressing these concerns? Are > > you planning on having content_browsertests? > > I see miu@ has lgtm'd but at the very least, you > > need a Todo (with a crbug, obviously), to have > > another, more in depth, look at all this code. > > Not that wasteful. > As to OnRGBAFrameAvailable, I just want to reuse the codes in > OnIncomingFrameAvailable(). So far I haven't observed any scaling happening. > I420Scale() will do a plane-copy only for most of time. What do you mean "for most of time" ? Regardless... IIUC, this is bad because it seems like you're adding a code path (the OnRGBA... --> OnIncomingFrameAvailable() ) that doesn't need to be there, since you have never "observed any scaling happening", which essentially deems this code unnecessary "most of the time" (all the time?). > If you don't mind having > some duplicated codes, I can make OnRGABFrameAvailable() to have the codes of > OnIncomingFrameAvailable(). You call! Then please separate both paths: Create an OnRGBAFrameAvailable() that calls libyuv::ABGRToI420() with the appropriate final resolution and then sends it to |capture_frame_cb|. (If that's what you're doing in the wall of code below, then fine). > Codes are here: > void ScreenCaptureMachineAndroid::OnRGBAFrameAvailable(JNIEnv* env, > jobject obj, > jobject buf, > jint row_stride, > jint left, > jint top, > jint width, > jint height, > jlong timestamp) { > uint8_t* const src = > reinterpret_cast<uint8_t*>(env->GetDirectBufferAddress(buf)); > CHECK(src); > > const VideoCaptureOracle::Event event = VideoCaptureOracle::kCompositorUpdate; > const uint64_t absolute_micro = > timestamp / base::Time::kNanosecondsPerMicrosecond; > const base::TimeTicks start_time = > base::TimeTicks() + base::TimeDelta::FromMicroseconds(absolute_micro); > scoped_refptr<VideoFrame> frame; > ThreadSafeCaptureOracle::CaptureFrameCallback capture_frame_cb; > > if (!oracle_proxy_->ObserveEventAndDecideCapture( > event, gfx::Rect(), start_time, &frame, &capture_frame_cb)) { > return; > } > > DCHECK(frame->format() == PIXEL_FORMAT_I420 || > frame->format() == PIXEL_FORMAT_YV12); > > const int offset = top * row_stride + left * 4; > // ABGR little endian (rgba in memory) to I420. > libyuv::ABGRToI420( > src + offset, row_stride, frame->visible_data(VideoFrame::kYPlane), > frame->stride(VideoFrame::kYPlane), > frame->visible_data(VideoFrame::kUPlane), > frame->stride(VideoFrame::kUPlane), > frame->visible_data(VideoFrame::kVPlane), > frame->stride(VideoFrame::kVPlane), > frame->visible_rect().width(), frame->visible_rect().height()); > > capture_frame_cb.Run(frame, start_time, true); > > lastFrame_ = frame; > } > > As to the hand-made-deinterlacing, it is better because the only thing we know > for sure is that we can start at the pointers and read every other bytes from > there, despite the real output format from Android (I420/NV21/NV12) which is not > documented or guaranteed on all platforms. Miu@ and I had a discuss about this > and both agree that hand-made-deinterlacing is more appropriate here. Then please: 1 - file a bug against libyuv issue tracker describing your situation and why you can't use the library function. https://bugs.chromium.org/p/libyuv then, add a comment in OnI420FrameAvailable() : 2 - explain why you cannot just deinterlace using a "normal" libyuv function 3 - add links to Android Developer documentation backing your statements 4 - and, obviously, add a reference to the libyuv bug number in the comment you're writing. When you add that URL, make sure it is a well formed URL.
Done. PTAL. https://codereview.chromium.org/1917023003/diff/200001/media/capture/content/... File media/capture/content/android/screen_capture_machine_android.cc (right): https://codereview.chromium.org/1917023003/diff/200001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:115: int uv_offset = (top / 2) * uv_stride + left / 2; On 2016/06/15 15:19:13, mcasas wrote: > l.114 and this line should be const. Done. https://codereview.chromium.org/1917023003/diff/200001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:210: libyuv::I420Scale(y_src, y_stride, u_src, u_stride, v_src, v_stride, width, On 2016/06/15 15:19:13, mcasas wrote: > On 2016/06/13 22:30:32, braveyao wrote: > > On 2016/06/12 10:11:51, mcasas wrote: > > > I don't like that we have a step of libyuv::ABGRToI420() > > > -- or a hand made deinterlacing. Those step seem > > > wasteful if you're going to Scale here to a smaller > > > resolution, i.e., why not convert-and-scale at the > > > same time? > > > > > > Moreover, you're allocating heap memory (l.47 or > > > l.103-104) for _every_ incoming frame, which is > > > tantamount to memory bashing and, in Android > > > devices, probably not good at all. > > > > > > How are you addressing these concerns? Are > > > you planning on having content_browsertests? > > > I see miu@ has lgtm'd but at the very least, you > > > need a Todo (with a crbug, obviously), to have > > > another, more in depth, look at all this code. > > > > Not that wasteful. > > As to OnRGBAFrameAvailable, I just want to reuse the codes in > > OnIncomingFrameAvailable(). So far I haven't observed any scaling happening. > > I420Scale() will do a plane-copy only for most of time. > What do you mean "for most of time" ? Regardless... > > IIUC, this is bad because it seems like you're > adding a code path (the OnRGBA... --> OnIncomingFrameAvailable() ) > that doesn't need to be there, since you have never > "observed any scaling happening", which essentially > deems this code unnecessary "most of the time" > (all the time?). > > > If you don't mind having > > some duplicated codes, I can make OnRGABFrameAvailable() to have the codes of > > OnIncomingFrameAvailable(). You call! > > Then please separate both paths: Create an > OnRGBAFrameAvailable() that calls > libyuv::ABGRToI420() with the appropriate final > resolution and then sends it to |capture_frame_cb|. > (If that's what you're doing in the wall of code > below, then fine). > > > Codes are here: > > void ScreenCaptureMachineAndroid::OnRGBAFrameAvailable(JNIEnv* env, > > jobject obj, > > jobject buf, > > jint row_stride, > > jint left, > > jint top, > > jint width, > > jint height, > > jlong timestamp) { > > uint8_t* const src = > > reinterpret_cast<uint8_t*>(env->GetDirectBufferAddress(buf)); > > CHECK(src); > > > > const VideoCaptureOracle::Event event = > VideoCaptureOracle::kCompositorUpdate; > > const uint64_t absolute_micro = > > timestamp / base::Time::kNanosecondsPerMicrosecond; > > const base::TimeTicks start_time = > > base::TimeTicks() + base::TimeDelta::FromMicroseconds(absolute_micro); > > scoped_refptr<VideoFrame> frame; > > ThreadSafeCaptureOracle::CaptureFrameCallback capture_frame_cb; > > > > if (!oracle_proxy_->ObserveEventAndDecideCapture( > > event, gfx::Rect(), start_time, &frame, &capture_frame_cb)) { > > return; > > } > > > > DCHECK(frame->format() == PIXEL_FORMAT_I420 || > > frame->format() == PIXEL_FORMAT_YV12); > > > > const int offset = top * row_stride + left * 4; > > // ABGR little endian (rgba in memory) to I420. > > libyuv::ABGRToI420( > > src + offset, row_stride, frame->visible_data(VideoFrame::kYPlane), > > frame->stride(VideoFrame::kYPlane), > > frame->visible_data(VideoFrame::kUPlane), > > frame->stride(VideoFrame::kUPlane), > > frame->visible_data(VideoFrame::kVPlane), > > frame->stride(VideoFrame::kVPlane), > > frame->visible_rect().width(), frame->visible_rect().height()); > > > > capture_frame_cb.Run(frame, start_time, true); > > > > lastFrame_ = frame; > > } > > > > As to the hand-made-deinterlacing, it is better because the only thing we know > > for sure is that we can start at the pointers and read every other bytes from > > there, despite the real output format from Android (I420/NV21/NV12) which is > not > > documented or guaranteed on all platforms. Miu@ and I had a discuss about this > > and both agree that hand-made-deinterlacing is more appropriate here. > > Then please: > 1 - file a bug against libyuv issue tracker describing your > situation and why you can't use the library function. > https://bugs.chromium.org/p/libyuv > > then, add a comment in OnI420FrameAvailable() : > 2 - explain why you cannot just deinterlace using a "normal" > libyuv function > 3 - add links to Android Developer documentation backing > your statements > 4 - and, obviously, add a reference to the libyuv bug number > in the comment you're writing. When you add that URL, make > sure it is a well formed URL. > Done.
On 2016/06/15 21:56:37, braveyao wrote: > Done. PTAL. > > https://codereview.chromium.org/1917023003/diff/200001/media/capture/content/... > File media/capture/content/android/screen_capture_machine_android.cc (right): > > https://codereview.chromium.org/1917023003/diff/200001/media/capture/content/... > media/capture/content/android/screen_capture_machine_android.cc:115: int > uv_offset = (top / 2) * uv_stride + left / 2; > On 2016/06/15 15:19:13, mcasas wrote: > > l.114 and this line should be const. > > Done. > > https://codereview.chromium.org/1917023003/diff/200001/media/capture/content/... > media/capture/content/android/screen_capture_machine_android.cc:210: > libyuv::I420Scale(y_src, y_stride, u_src, u_stride, v_src, v_stride, width, > On 2016/06/15 15:19:13, mcasas wrote: > > On 2016/06/13 22:30:32, braveyao wrote: > > > On 2016/06/12 10:11:51, mcasas wrote: > > > > I don't like that we have a step of libyuv::ABGRToI420() > > > > -- or a hand made deinterlacing. Those step seem > > > > wasteful if you're going to Scale here to a smaller > > > > resolution, i.e., why not convert-and-scale at the > > > > same time? > > > > > > > > Moreover, you're allocating heap memory (l.47 or > > > > l.103-104) for _every_ incoming frame, which is > > > > tantamount to memory bashing and, in Android > > > > devices, probably not good at all. > > > > > > > > How are you addressing these concerns? Are > > > > you planning on having content_browsertests? > > > > I see miu@ has lgtm'd but at the very least, you > > > > need a Todo (with a crbug, obviously), to have > > > > another, more in depth, look at all this code. > > > > > > Not that wasteful. > > > As to OnRGBAFrameAvailable, I just want to reuse the codes in > > > OnIncomingFrameAvailable(). So far I haven't observed any scaling happening. > > > I420Scale() will do a plane-copy only for most of time. > > What do you mean "for most of time" ? Regardless... > > > > IIUC, this is bad because it seems like you're > > adding a code path (the OnRGBA... --> OnIncomingFrameAvailable() ) > > that doesn't need to be there, since you have never > > "observed any scaling happening", which essentially > > deems this code unnecessary "most of the time" > > (all the time?). > > > > > If you don't mind having > > > some duplicated codes, I can make OnRGABFrameAvailable() to have the codes > of > > > OnIncomingFrameAvailable(). You call! > > > > Then please separate both paths: Create an > > OnRGBAFrameAvailable() that calls > > libyuv::ABGRToI420() with the appropriate final > > resolution and then sends it to |capture_frame_cb|. > > (If that's what you're doing in the wall of code > > below, then fine). > > > > > Codes are here: > > > void ScreenCaptureMachineAndroid::OnRGBAFrameAvailable(JNIEnv* env, > > > jobject obj, > > > jobject buf, > > > jint row_stride, > > > jint left, > > > jint top, > > > jint width, > > > jint height, > > > jlong timestamp) { > > > uint8_t* const src = > > > reinterpret_cast<uint8_t*>(env->GetDirectBufferAddress(buf)); > > > CHECK(src); > > > > > > const VideoCaptureOracle::Event event = > > VideoCaptureOracle::kCompositorUpdate; > > > const uint64_t absolute_micro = > > > timestamp / base::Time::kNanosecondsPerMicrosecond; > > > const base::TimeTicks start_time = > > > base::TimeTicks() + base::TimeDelta::FromMicroseconds(absolute_micro); > > > scoped_refptr<VideoFrame> frame; > > > ThreadSafeCaptureOracle::CaptureFrameCallback capture_frame_cb; > > > > > > if (!oracle_proxy_->ObserveEventAndDecideCapture( > > > event, gfx::Rect(), start_time, &frame, &capture_frame_cb)) { > > > return; > > > } > > > > > > DCHECK(frame->format() == PIXEL_FORMAT_I420 || > > > frame->format() == PIXEL_FORMAT_YV12); > > > > > > const int offset = top * row_stride + left * 4; > > > // ABGR little endian (rgba in memory) to I420. > > > libyuv::ABGRToI420( > > > src + offset, row_stride, frame->visible_data(VideoFrame::kYPlane), > > > frame->stride(VideoFrame::kYPlane), > > > frame->visible_data(VideoFrame::kUPlane), > > > frame->stride(VideoFrame::kUPlane), > > > frame->visible_data(VideoFrame::kVPlane), > > > frame->stride(VideoFrame::kVPlane), > > > frame->visible_rect().width(), frame->visible_rect().height()); > > > > > > capture_frame_cb.Run(frame, start_time, true); > > > > > > lastFrame_ = frame; > > > } > > > > > > As to the hand-made-deinterlacing, it is better because the only thing we > know > > > for sure is that we can start at the pointers and read every other bytes > from > > > there, despite the real output format from Android (I420/NV21/NV12) which is > > not > > > documented or guaranteed on all platforms. Miu@ and I had a discuss about > this > > > and both agree that hand-made-deinterlacing is more appropriate here. > > > > Then please: > > 1 - file a bug against libyuv issue tracker describing your > > situation and why you can't use the library function. > > https://bugs.chromium.org/p/libyuv > > > > then, add a comment in OnI420FrameAvailable() : > > 2 - explain why you cannot just deinterlace using a "normal" > > libyuv function > > 3 - add links to Android Developer documentation backing > > your statements > > 4 - and, obviously, add a reference to the libyuv bug number > > in the comment you're writing. When you add that URL, make > > sure it is a well formed URL. > > > > Done. Hi Miguel, any more comments?
LGTM % you getting qinmin@ to review the Java code. (Btw for some reason my comment #42 shows up green in Rietveld, but I didn't lgtm then, hmmm)
Hi qinmin, any more comments on the Java code?
not lgtm...Some things were broken and need to be revisited since my last lgtm: https://codereview.chromium.org/1917023003/diff/220001/media/capture/content/... File media/capture/content/android/screen_capture_machine_android.cc (right): https://codereview.chromium.org/1917023003/diff/220001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:69: libyuv::ABGRToI420( Now this method has become incorrect. The scaling step is required when: 1. frame->visible_rect().width() != width 2. frame->visible_rect().height() != height (The size of the |frame| can vary and is not necessarily the same as |width|x|height|.) So, if the size is different, then you will need to create a temporary frame to scale the I420 to the final size. If it's not, you want to have convert directly into the final YUV frame. See comment below... https://codereview.chromium.org/1917023003/diff/220001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:216: if (!oracle_proxy_->ObserveEventAndDecideCapture( It would be better if this were executed before any of the UV plane de-interlacing in the calling method. See comment below... https://codereview.chromium.org/1917023003/diff/220001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:237: } I think the simplest solution to my above comments is to delete the OnIncomingFrameAvailable() method, and just write the other two as: void ScreenCaptureMachineAndroid::OnRGBAFrameAvailable(JNIEnv* env, jobject obj, jobject buf, jint row_stride, jint left, jint top, jint width, jint height, jlong timestamp) { const VideoCaptureOracle::Event event = VideoCaptureOracle::kCompositorUpdate; const uint64_t absolute_micro = timestamp / base::Time::kNanosecondsPerMicrosecond; const base::TimeTicks start_time = base::TimeTicks() + base::TimeDelta::FromMicroseconds(absolute_micro); scoped_refptr<VideoFrame> frame; ThreadSafeCaptureOracle::CaptureFrameCallback capture_frame_cb; if (!oracle_proxy_->ObserveEventAndDecideCapture( event, gfx::Rect(), start_time, &frame, &capture_frame_cb)) { return; } DCHECK(frame->format() == PIXEL_FORMAT_I420 || frame->format() == PIXEL_FORMAT_YV12); scoped_refptr<VideoFrame> unscaled_frame = frame; if (frame->visible_rect().width() != width || frame->visible_rect().height() != height) { unscaled_frame = VideoFrame::CreateFrame( PIXEL_FORMAT_I420, gfx::Size(width, height), gfx::Rect(width, height), gfx::Size(width, height), base::TimeDelta()); } uint8_t* const src = reinterpret_cast<uint8_t*>(env->GetDirectBufferAddress(buf)); CHECK(src); const int offset = top * row_stride + left * 4; // ABGR little endian (rgba in memory) to I420. libyuv::ABGRToI420( src + offset, row_stride, unscaled_frame->visible_data(VideoFrame::kYPlane), unscaled_frame->stride(VideoFrame::kYPlane), unscaled_frame->visible_data(VideoFrame::kUPlane), unscaled_frame->stride(VideoFrame::kUPlane), unscaled_frame->visible_data(VideoFrame::kVPlane), unscaled_frame->stride(VideoFrame::kVPlane), unscaled_frame->visible_rect().width(), unscaled_frame->visible_rect().height()); if (unscaled_frame != frame) { libyuv::I420Scale(unscaled_frame->visible_data(VideoFrame::kYPlane), unscaled_frame->stride(VideoFrame::kYPlane), unscaled_frame->visible_data(VideoFrame::kUPlane), unscaled_frame->stride(VideoFrame::kUPlane), unscaled_frame->visible_data(VideoFrame::kVPlane), unscaled_frame->stride(VideoFrame::kVPlane), unscaled_frame->visible_rect().width(), unscaled_frame->visible_rect().height(), frame->visible_data(VideoFrame::kYPlane), frame->stride(VideoFrame::kYPlane), frame->visible_data(VideoFrame::kUPlane), frame->stride(VideoFrame::kUPlane), frame->visible_data(VideoFrame::kVPlane), frame->stride(VideoFrame::kVPlane), frame->visible_rect().width(), frame->visible_rect().height(), libyuv::kFilterBilinear); } capture_frame_cb.Run(frame, start_time, true); lastFrame_ = frame; } void ScreenCaptureMachineAndroid::OnI420FrameAvailable(JNIEnv* env, jobject obj, jobject y_buffer, jint y_stride, jobject u_buffer, jobject v_buffer, jint uv_row_stride, jint uv_pixel_stride, jint left, jint top, jint width, jint height, jlong timestamp) { const VideoCaptureOracle::Event event = VideoCaptureOracle::kCompositorUpdate; const uint64_t absolute_micro = timestamp / base::Time::kNanosecondsPerMicrosecond; const base::TimeTicks start_time = base::TimeTicks() + base::TimeDelta::FromMicroseconds(absolute_micro); scoped_refptr<VideoFrame> frame; ThreadSafeCaptureOracle::CaptureFrameCallback capture_frame_cb; if (!oracle_proxy_->ObserveEventAndDecideCapture( event, gfx::Rect(), start_time, &frame, &capture_frame_cb)) { return; } DCHECK(frame->format() == PIXEL_FORMAT_I420 || frame->format() == PIXEL_FORMAT_YV12); uint8_t* const y_src = reinterpret_cast<uint8_t*>(env->GetDirectBufferAddress(y_buffer)); CHECK(y_src); uint8_t* const u_src = reinterpret_cast<uint8_t*>(env->GetDirectBufferAddress(u_buffer)); CHECK(u_src); uint8_t* const v_src = reinterpret_cast<uint8_t*>(env->GetDirectBufferAddress(v_buffer)); CHECK(v_src); // De-interleave the U and V planes into temporary buffers, if needed. int uv_stride = uv_row_stride; std::unique_ptr<uint8_t[]> u_tmp, v_tmp; if (uv_pixel_stride != 1) { // U and V planes are actually interleaved, unpack them here. // TODO(braveyao): According to // https://developer.android.com/reference/android/graphics/ImageFormat.html#YUV..., // how U and V planes are interlaced is not guaranteed, so there is no an // existing libyuv function suitable for such a job. Filed a request at // https://bugs.chromium.org/p/libyuv/issues/detail?id=604. Switch to new // function when it's available. const int uv_plane_len = (int)env->GetDirectBufferCapacity(u_buffer); u_tmp.reset(new uint8_t[(uv_plane_len + 1) / uv_pixel_stride]); v_tmp.reset(new uint8_t[(uv_plane_len + 1) / uv_pixel_stride]); for (int index = 0; index * uv_pixel_stride <= uv_plane_len; index++) { u_tmp[index] = u_src[index * uv_pixel_stride]; v_tmp[index] = v_src[index * uv_pixel_stride]; } u_src = u_tmp.get(); v_src = v_tmp.get(); uv_stride /= uv_pixel_stride; } const int y_offset = top * y_stride + left; const int uv_offset = (top / 2) * uv_stride + left / 2; // Note: If source width/height are same as the frame's width/height, the // following will, internally, just perform a copy without scaling. libyuv::I420Scale(y_src + y_offset, y_stride, u_src + uv_offset, uv_stride, v_src + uv_offset, uv_stride, width, height, frame->visible_data(VideoFrame::kYPlane), frame->stride(VideoFrame::kYPlane), frame->visible_data(VideoFrame::kUPlane), frame->stride(VideoFrame::kUPlane), frame->visible_data(VideoFrame::kVPlane), frame->stride(VideoFrame::kVPlane), frame->visible_rect().width(), frame->visible_rect().height(), libyuv::kFilterBilinear); capture_frame_cb.Run(frame, start_time, true); lastFrame_ = frame; }
Patchset #10 (id:240001) has been deleted
Patchset #10 (id:260001) has been deleted
Hi Miu, thanks so much for spotting this out. Correct it with the suggestion and keep the OnIncomingFrameAvailable to avoid some duplicated codes. PTAL! https://codereview.chromium.org/1917023003/diff/220001/media/capture/content/... File media/capture/content/android/screen_capture_machine_android.cc (right): https://codereview.chromium.org/1917023003/diff/220001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:69: libyuv::ABGRToI420( On 2016/06/21 03:34:07, miu wrote: > Now this method has become incorrect. The scaling step is required when: > > 1. frame->visible_rect().width() != width > 2. frame->visible_rect().height() != height > > (The size of the |frame| can vary and is not necessarily the same as > |width|x|height|.) > > So, if the size is different, then you will need to create a temporary frame to > scale the I420 to the final size. If it's not, you want to have convert directly > into the final YUV frame. > > See comment below... Done. Misunderstood mcasas@. Thought ABGRToI420 could do convert-scale together. https://codereview.chromium.org/1917023003/diff/220001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:216: if (!oracle_proxy_->ObserveEventAndDecideCapture( On 2016/06/21 03:34:07, miu wrote: > It would be better if this were executed before any of the UV plane > de-interlacing in the calling method. > > See comment below... Yes it's true. But OnIncomingFrameAvailable will be called in two places, OnI420FrameAvailable() and MaybeCaptureForRefresh(). So I'd like to keep OnIncomingFrameAvailable to avoid some duplicated code. WDYT? https://codereview.chromium.org/1917023003/diff/220001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:237: } On 2016/06/21 03:34:07, miu wrote: > I think the simplest solution to my above comments is to delete the > OnIncomingFrameAvailable() method, and just write the other two as: > > void ScreenCaptureMachineAndroid::OnRGBAFrameAvailable(JNIEnv* env, > jobject obj, > jobject buf, > jint row_stride, > jint left, > jint top, > jint width, > jint height, > jlong timestamp) { > const VideoCaptureOracle::Event event = VideoCaptureOracle::kCompositorUpdate; > const uint64_t absolute_micro = > timestamp / base::Time::kNanosecondsPerMicrosecond; > const base::TimeTicks start_time = > base::TimeTicks() + base::TimeDelta::FromMicroseconds(absolute_micro); > scoped_refptr<VideoFrame> frame; > ThreadSafeCaptureOracle::CaptureFrameCallback capture_frame_cb; > > if (!oracle_proxy_->ObserveEventAndDecideCapture( > event, gfx::Rect(), start_time, &frame, &capture_frame_cb)) { > return; > } > > DCHECK(frame->format() == PIXEL_FORMAT_I420 || > frame->format() == PIXEL_FORMAT_YV12); > > scoped_refptr<VideoFrame> unscaled_frame = frame; > if (frame->visible_rect().width() != width || > frame->visible_rect().height() != height) { > unscaled_frame = VideoFrame::CreateFrame( > PIXEL_FORMAT_I420, gfx::Size(width, height), gfx::Rect(width, height), > gfx::Size(width, height), base::TimeDelta()); > } > > uint8_t* const src = > reinterpret_cast<uint8_t*>(env->GetDirectBufferAddress(buf)); > CHECK(src); > const int offset = top * row_stride + left * 4; > // ABGR little endian (rgba in memory) to I420. > libyuv::ABGRToI420( > src + offset, row_stride, > unscaled_frame->visible_data(VideoFrame::kYPlane), > unscaled_frame->stride(VideoFrame::kYPlane), > unscaled_frame->visible_data(VideoFrame::kUPlane), > unscaled_frame->stride(VideoFrame::kUPlane), > unscaled_frame->visible_data(VideoFrame::kVPlane), > unscaled_frame->stride(VideoFrame::kVPlane), > unscaled_frame->visible_rect().width(), > unscaled_frame->visible_rect().height()); > > if (unscaled_frame != frame) { > libyuv::I420Scale(unscaled_frame->visible_data(VideoFrame::kYPlane), > unscaled_frame->stride(VideoFrame::kYPlane), > unscaled_frame->visible_data(VideoFrame::kUPlane), > unscaled_frame->stride(VideoFrame::kUPlane), > unscaled_frame->visible_data(VideoFrame::kVPlane), > unscaled_frame->stride(VideoFrame::kVPlane), > unscaled_frame->visible_rect().width(), > unscaled_frame->visible_rect().height(), > frame->visible_data(VideoFrame::kYPlane), > frame->stride(VideoFrame::kYPlane), > frame->visible_data(VideoFrame::kUPlane), > frame->stride(VideoFrame::kUPlane), > frame->visible_data(VideoFrame::kVPlane), > frame->stride(VideoFrame::kVPlane), > frame->visible_rect().width(), > frame->visible_rect().height(), > libyuv::kFilterBilinear); > } > > capture_frame_cb.Run(frame, start_time, true); > > lastFrame_ = frame; > } > > void ScreenCaptureMachineAndroid::OnI420FrameAvailable(JNIEnv* env, > jobject obj, > jobject y_buffer, > jint y_stride, > jobject u_buffer, > jobject v_buffer, > jint uv_row_stride, > jint uv_pixel_stride, > jint left, > jint top, > jint width, > jint height, > jlong timestamp) { > const VideoCaptureOracle::Event event = VideoCaptureOracle::kCompositorUpdate; > const uint64_t absolute_micro = > timestamp / base::Time::kNanosecondsPerMicrosecond; > const base::TimeTicks start_time = > base::TimeTicks() + base::TimeDelta::FromMicroseconds(absolute_micro); > scoped_refptr<VideoFrame> frame; > ThreadSafeCaptureOracle::CaptureFrameCallback capture_frame_cb; > > if (!oracle_proxy_->ObserveEventAndDecideCapture( > event, gfx::Rect(), start_time, &frame, &capture_frame_cb)) { > return; > } > > DCHECK(frame->format() == PIXEL_FORMAT_I420 || > frame->format() == PIXEL_FORMAT_YV12); > > uint8_t* const y_src = > reinterpret_cast<uint8_t*>(env->GetDirectBufferAddress(y_buffer)); > CHECK(y_src); > uint8_t* const u_src = > reinterpret_cast<uint8_t*>(env->GetDirectBufferAddress(u_buffer)); > CHECK(u_src); > uint8_t* const v_src = > reinterpret_cast<uint8_t*>(env->GetDirectBufferAddress(v_buffer)); > CHECK(v_src); > > // De-interleave the U and V planes into temporary buffers, if needed. > int uv_stride = uv_row_stride; > std::unique_ptr<uint8_t[]> u_tmp, v_tmp; > if (uv_pixel_stride != 1) { > // U and V planes are actually interleaved, unpack them here. > // TODO(braveyao): According to > // > https://developer.android.com/reference/android/graphics/ImageFormat.html#YUV..., > // how U and V planes are interlaced is not guaranteed, so there is no an > // existing libyuv function suitable for such a job. Filed a request at > // https://bugs.chromium.org/p/libyuv/issues/detail?id=604. Switch to new > // function when it's available. > const int uv_plane_len = (int)env->GetDirectBufferCapacity(u_buffer); > u_tmp.reset(new uint8_t[(uv_plane_len + 1) / uv_pixel_stride]); > v_tmp.reset(new uint8_t[(uv_plane_len + 1) / uv_pixel_stride]); > for (int index = 0; index * uv_pixel_stride <= uv_plane_len; index++) { > u_tmp[index] = u_src[index * uv_pixel_stride]; > v_tmp[index] = v_src[index * uv_pixel_stride]; > } > u_src = u_tmp.get(); > v_src = v_tmp.get(); > uv_stride /= uv_pixel_stride; > } > > const int y_offset = top * y_stride + left; > const int uv_offset = (top / 2) * uv_stride + left / 2; > // Note: If source width/height are same as the frame's width/height, the > // following will, internally, just perform a copy without scaling. > libyuv::I420Scale(y_src + y_offset, y_stride, u_src + uv_offset, uv_stride, > v_src + uv_offset, uv_stride, width, height, > frame->visible_data(VideoFrame::kYPlane), > frame->stride(VideoFrame::kYPlane), > frame->visible_data(VideoFrame::kUPlane), > frame->stride(VideoFrame::kUPlane), > frame->visible_data(VideoFrame::kVPlane), > frame->stride(VideoFrame::kVPlane), > frame->visible_rect().width(), > frame->visible_rect().height(), libyuv::kFilterBilinear); > > capture_frame_cb.Run(frame, start_time, true); > > lastFrame_ = frame; > } Done to OnRGBAFrameAvailable and keep the current implementation to others to avoid some duplicated codes as in above comments.
https://codereview.chromium.org/1917023003/diff/220001/media/capture/content/... File media/capture/content/android/screen_capture_machine_android.cc (right): https://codereview.chromium.org/1917023003/diff/220001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:69: libyuv::ABGRToI420( On 2016/06/21 22:10:25, braveyao wrote: > On 2016/06/21 03:34:07, miu wrote: > > Now this method has become incorrect. The scaling step is required when: > > > > 1. frame->visible_rect().width() != width > > 2. frame->visible_rect().height() != height > > > > (The size of the |frame| can vary and is not necessarily the same as > > |width|x|height|.) > > > > So, if the size is different, then you will need to create a temporary frame > to > > scale the I420 to the final size. If it's not, you want to have convert > directly > > into the final YUV frame. > > > > See comment below... > > Done. > > Misunderstood mcasas@. Thought ABGRToI420 could do convert-scale together. What you need is a single step using libyuv::ConvertToI420 (like in [1]) which will convert+rotate+downscale (either or all of them, only when needed). [1] https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/vide...
On 2016/06/21 22:49:04, mcasas wrote: > https://codereview.chromium.org/1917023003/diff/220001/media/capture/content/... > File media/capture/content/android/screen_capture_machine_android.cc (right): > > https://codereview.chromium.org/1917023003/diff/220001/media/capture/content/... > media/capture/content/android/screen_capture_machine_android.cc:69: > libyuv::ABGRToI420( > On 2016/06/21 22:10:25, braveyao wrote: > > On 2016/06/21 03:34:07, miu wrote: > > > Now this method has become incorrect. The scaling step is required when: > > > > > > 1. frame->visible_rect().width() != width > > > 2. frame->visible_rect().height() != height > > > > > > (The size of the |frame| can vary and is not necessarily the same as > > > |width|x|height|.) > > > > > > So, if the size is different, then you will need to create a temporary frame > > to > > > scale the I420 to the final size. If it's not, you want to have convert > > directly > > > into the final YUV frame. > > > > > > See comment below... > > > > Done. > > > > Misunderstood mcasas@. Thought ABGRToI420 could do convert-scale together. > > What you need is a single step using libyuv::ConvertToI420 > (like in [1]) which will convert+rotate+downscale (either or > all of them, only when needed). > > [1] > https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/vide... Hi Miguel, it seems that libyuv::ConvertToI420() can only do cropping, not scaling. A quick test verified it. I suppose this is not what we want here. https://cs.chromium.org/chromium/src/third_party/libyuv/include/libyuv/conver...
https://codereview.chromium.org/1917023003/diff/280001/media/capture/content/... File media/capture/content/android/screen_capture_machine_android.cc (right): https://codereview.chromium.org/1917023003/diff/280001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:76: // ABGR little endian (rgba in memory) to I420. I wonder if this holds true for all Android processors out there. Have you talked to the Android team to see if there are big endian RGBA machines out there they know about? https://codereview.chromium.org/1917023003/diff/280001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:104: } In response to your comment to my comment in PS8 l.69 (which you should have replied there and not by clicking "Reply" in the message entries below): my bad, we still seem to need indeed two steps, ABGRToI420 and I420Scale.
Hi Migual, Miu and Qinmin, any other comments? https://codereview.chromium.org/1917023003/diff/280001/media/capture/content/... File media/capture/content/android/screen_capture_machine_android.cc (right): https://codereview.chromium.org/1917023003/diff/280001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:76: // ABGR little endian (rgba in memory) to I420. On 2016/06/22 22:23:44, mcasas wrote: > I wonder if this holds true for all Android processors > out there. Have you talked to the Android team to > see if there are big endian RGBA machines out there > they know about? Quote Android guys, "we depend on locked RGBA buffers being little endian". https://codereview.chromium.org/1917023003/diff/280001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:104: } On 2016/06/22 22:23:44, mcasas wrote: > In response to your comment to my comment in PS8 > l.69 (which you should have replied there and not by > clicking "Reply" in the message entries below): > my bad, we still seem to need indeed two steps, > ABGRToI420 and I420Scale. Acknowledged.
lgtm % nots https://codereview.chromium.org/1917023003/diff/280001/media/capture/content/... File media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java (right): https://codereview.chromium.org/1917023003/diff/280001/media/capture/content/... media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java:133: private MediaProjection mMediaProjection; move these to the top of the class declaration. And please order the variables: static final should be first, and then final and non final variables https://codereview.chromium.org/1917023003/diff/280001/media/capture/content/... File media/capture/content/android/screen_capture_jni_registrar.cc (right): https://codereview.chromium.org/1917023003/diff/280001/media/capture/content/... media/capture/content/android/screen_capture_jni_registrar.cc:10: nit: remove blank line
On 2016/06/23 19:58:23, qinmin wrote: > lgtm % nots s/nots/nits/ > > https://codereview.chromium.org/1917023003/diff/280001/media/capture/content/... > File > media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java > (right): > > https://codereview.chromium.org/1917023003/diff/280001/media/capture/content/... > media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java:133: > private MediaProjection mMediaProjection; > move these to the top of the class declaration. And please order the variables: > static final should be first, and then final and non final variables > > https://codereview.chromium.org/1917023003/diff/280001/media/capture/content/... > File media/capture/content/android/screen_capture_jni_registrar.cc (right): > > https://codereview.chromium.org/1917023003/diff/280001/media/capture/content/... > media/capture/content/android/screen_capture_jni_registrar.cc:10: > nit: remove blank line
https://codereview.chromium.org/1917023003/diff/220001/media/capture/content/... File media/capture/content/android/screen_capture_machine_android.cc (right): https://codereview.chromium.org/1917023003/diff/220001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:208: const VideoCaptureOracle::Event event = VideoCaptureOracle::kCompositorUpdate; This is the wrong event for the MaybeCaptureForRefresh() case. It should be kActiveRefreshRequest. https://codereview.chromium.org/1917023003/diff/220001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:216: if (!oracle_proxy_->ObserveEventAndDecideCapture( On 2016/06/21 22:10:25, braveyao wrote: > On 2016/06/21 03:34:07, miu wrote: > > It would be better if this were executed before any of the UV plane > > de-interlacing in the calling method. > > Yes it's true. But OnIncomingFrameAvailable will be called in two places, > OnI420FrameAvailable() and MaybeCaptureForRefresh(). So I'd like to keep > OnIncomingFrameAvailable to avoid some duplicated code. WDYT? Normally I'd agree. However, IMHO, the negative performance impact of doing the U and V plane de-interlacing warrants calling ObserveEventAndDecideCapture() as early as possible.
Patchset #11 (id:300001) has been deleted
Hi Miu, all done. PTAL! https://codereview.chromium.org/1917023003/diff/220001/media/capture/content/... File media/capture/content/android/screen_capture_machine_android.cc (right): https://codereview.chromium.org/1917023003/diff/220001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:208: const VideoCaptureOracle::Event event = VideoCaptureOracle::kCompositorUpdate; On 2016/06/23 23:09:13, miu wrote: > This is the wrong event for the MaybeCaptureForRefresh() case. It should be > kActiveRefreshRequest. Done. https://codereview.chromium.org/1917023003/diff/220001/media/capture/content/... media/capture/content/android/screen_capture_machine_android.cc:216: if (!oracle_proxy_->ObserveEventAndDecideCapture( On 2016/06/23 23:09:13, miu wrote: > On 2016/06/21 22:10:25, braveyao wrote: > > On 2016/06/21 03:34:07, miu wrote: > > > It would be better if this were executed before any of the UV plane > > > de-interlacing in the calling method. > > > > Yes it's true. But OnIncomingFrameAvailable will be called in two places, > > OnI420FrameAvailable() and MaybeCaptureForRefresh(). So I'd like to keep > > OnIncomingFrameAvailable to avoid some duplicated code. WDYT? > > Normally I'd agree. However, IMHO, the negative performance impact of doing the > U and V plane de-interlacing warrants calling ObserveEventAndDecideCapture() as > early as possible. Done. https://codereview.chromium.org/1917023003/diff/280001/media/capture/content/... File media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java (right): https://codereview.chromium.org/1917023003/diff/280001/media/capture/content/... media/capture/content/android/java/src/org/chromium/media/ScreenCapture.java:133: private MediaProjection mMediaProjection; On 2016/06/23 19:58:23, qinmin wrote: > move these to the top of the class declaration. And please order the variables: > static final should be first, and then final and non final variables Done. https://codereview.chromium.org/1917023003/diff/280001/media/capture/content/... File media/capture/content/android/screen_capture_jni_registrar.cc (right): https://codereview.chromium.org/1917023003/diff/280001/media/capture/content/... media/capture/content/android/screen_capture_jni_registrar.cc:10: On 2016/06/23 19:58:23, qinmin wrote: > nit: remove blank line Done.
lgtm. Sorry for the delay...this got lost in my e-mail inbox.
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
The patchset sent to the CQ was uploaded after l-g-t-m from michaelbai@chromium.org, tedchoc@chromium.org, dalecurtis@chromium.org, mcasas@chromium.org, qinmin@chromium.org Link to the patchset: https://codereview.chromium.org/1917023003/#ps320001 (title: "call ObserveEventAndDecideCapture() as early as possible and rebase")
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...)
Hi Ted, Could you please help to take another look to content/public/android/BUILD.gn, which is added here to make 'linux_android_rel_ng' bots turn to green from purple. The components_browsertests on linux_android_rel_ng will fail due to missing dependencies to the screen capture java codes. It's ignored all the time due to its purple state with no straight error info :(
On 2016/06/30 22:06:38, braveyao wrote: > Hi Ted, > Could you please help to take another look to content/public/android/BUILD.gn, > which is added here to make 'linux_android_rel_ng' bots turn to green from > purple. > The components_browsertests on linux_android_rel_ng will fail due to missing > dependencies to the screen capture java codes. It's ignored all the time due to > its purple state with no straight error info :( lgtm
The CQ bit was checked by braveyao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaelbai@chromium.org, qinmin@chromium.org, mcasas@chromium.org, dalecurtis@chromium.org, miu@chromium.org Link to the patchset: https://codereview.chromium.org/1917023003/#ps340001 (title: "add dependency")
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.
Committed patchset #12 (id:340001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== ScreenCapture for Android phase1, part I The document about ScreenCapture for Android is here, https://goo.gl/QNH29g. This cl is mainly based on the https://codereview.chromium.org/1140113002/. Only the codes under media/ are included here, which implements the JAVA capture with Android MediaProjection API and JNI connectors, with some issues fixed so it can always work. The control part in content/ and chrome/ will be in the next cl, as part II. BUG=487935 ========== to ========== ScreenCapture for Android phase1, part I The document about ScreenCapture for Android is here, https://goo.gl/QNH29g. This cl is mainly based on the https://codereview.chromium.org/1140113002/. Only the codes under media/ are included here, which implements the JAVA capture with Android MediaProjection API and JNI connectors, with some issues fixed so it can always work. The control part in content/ and chrome/ will be in the next cl, as part II. BUG=487935 Committed: https://crrev.com/2f731e17983201082d9fc725cf7717868fc1e75d Cr-Commit-Position: refs/heads/master@{#403555} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/2f731e17983201082d9fc725cf7717868fc1e75d Cr-Commit-Position: refs/heads/master@{#403555}
Message was sent while issue was closed.
A revert of this CL (patchset #12 id:340001) has been created in https://codereview.chromium.org/2116883002/ by kelvinp@chromium.org. The reason for reverting is: Break compile step on Android Builder logs: https://build.chromium.org/p/chromium/builders/Android/builds/58512/steps/com... Failure: FAILED: obj/media/capture/content/android/android/screen_capture_jni_registrar.o /b/build/slave/cache/cipd/goma/gomacc ../../third_party/android_tools/ndk/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/arm-linux-androideabi-g++ -MMD -MF obj/media/capture/content/android/android/screen_capture_jni_registrar.o.d -DV8_DEPRECATION_WARNINGS -DENABLE_NOTIFICATIONS -DENABLE_BROWSER_CDMS -DENABLE_PRINTING=1 -DENABLE_BASIC_PRINTING=1 -DENABLE_SPELLCHECK=1 -DUSE_BROWSER_SPELLCHECKER=1 -DUSE_OPENSSL_CERTS=1 -DNO_TCMALLOC -DUSE_EXTERNAL_POPUP_MENU=1 -DENABLE_WEBRTC=1 -DDISABLE_NACL -DENABLE_SUPERVISED_USERS=1 -DVIDEO_HOLE=1 -DSAFE_BROWSING_DB_REMOTE -DCHROMIUM_BUILD -DENABLE_MEDIA_ROUTER=1 -DENABLE_WEBVR -DFIELDTRIAL_TESTING_ENABLED -D_FILE_OFFSET_BITS=64 -DANDROID -DHAVE_SYS_UIO_H -DANDROID_NDK_VERSION=r10e -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D_FORTIFY_SOURCE=2 -D__GNU_SOURCE=1 -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -DUSE_NEON -DMEDIA_IMPLEMENTATION -I../.. -Igen -Igen/media/capture/content/android/screen_capture_jni_headers -Igen/media/capture/content/android/screen_capture_jni_headers/media -I../../third_party/libyuv -I../../third_party/libyuv/include -fno-strict-aliasing --param=ssp-buffer-size=4 -fstack-protector -funwind-tables -fPIC -pipe -ffunction-sections -fno-short-enums -finline-limit=64 -march=armv7-a -mfloat-abi=softfp -mthumb -mthumb-interwork -mtune=generic-armv7-a -fno-tree-sra -fno-caller-saves -mfpu=neon -Wall -Werror -Wno-psabi -Wno-unused-local-typedefs -Wno-maybe-uninitialized -Wno-missing-field-initializers -Wno-unused-parameter -Os -fomit-frame-pointer -fno-ident -fdata-sections -ffunction-sections -g1 --sysroot=../../third_party/android_tools/ndk/platforms/android-16/arch-arm -fvisibility=hidden -fno-threadsafe-statics -fvisibility-inlines-hidden -std=gnu++11 -Wno-narrowing -fno-rtti -isystem../../third_party/android_tools/ndk/sources/cxx-stl/llvm-libc++/libcxx/include -isystem../../third_party/android_tools/ndk/sources/cxx-stl/llvm-libc++abi/libcxxabi/include -isystem../../third_party/android_tools/ndk/sources/android/support/include -fno-exceptions -c ../../media/capture/content/android/screen_capture_jni_registrar.cc -o obj/media/capture/content/android/android/screen_capture_jni_registrar.o In file included from ../../media/capture/content/thread_safe_capture_oracle.h:15:0, from ../../media/capture/content/screen_capture_device_core.h:15, from ../../media/capture/content/android/screen_capture_machine_android.h:12, from ../../media/capture/content/android/screen_capture_jni_registrar.cc:10: ../../media/capture/video/video_capture_device.h:33:55: fatal error: media/mojo/interfaces/image_capture.mojom.h: No such file or directory #include "media/mojo/interfaces/image_capture.mojom.h" .
Message was sent while issue was closed.
Patchset #13 (id:360001) has been deleted |
