Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2276593003:
Android Screen Capturer. (Closed)
|
Created:
4 years, 4 months ago by arsany Modified:
4 years, 3 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, dgupta_google.com, helprtc-eng_google.com Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionCreate Android screen capturer.
Committed: https://crrev.com/b75f2541c942e2f35c3b7d7003ed17504176ced1
Cr-Commit-Position: refs/heads/master@{#14010}
Patch Set 1 #Patch Set 2 : Fix comments. #
Total comments: 17
Patch Set 3 : Create Android screen capturer. #
Total comments: 26
Patch Set 4 : Manage MediaProjection inside ScreenCapturerAndroid and allow calling changeFormat() whether captur… #
Total comments: 4
Patch Set 5 : Switch to new Video API. #Patch Set 6 : Merge conflicts with https://codereview.webrtc.org/2298063003 #Patch Set 7 : merge master #Messages
Total messages: 35 (15 generated)
Description was changed from ========== Fix copyright statement. Create Android screen capturer. BUG= ========== to ========== Create Android screen capturer. BUG= ==========
arsany@google.com changed reviewers: + magjed@google.com, magjed@webrtc.org, perkj@webrtc.org, tommi@webrtc.org
Hi all, This is a proposal to upstream the Android screen capturer discussed elsewhere.
magjed@webrtc.org changed reviewers: + sakal@webrtc.org - magjed@google.com
Thanks Arsany! The ScreenCapturerAndroid code is very clean. It would be nice if we could add a setting to AppRTCDemo to use ScreenCapturerAndroid instead of the camera so it's easier to test. Can you provide some example code for how to create the ScreenCapturerAndroid, i.e. the code for setting up the VirtualDisplay? We can probably help you with updating AppRTCDemo. https://codereview.webrtc.org/2276593003/diff/20001/webrtc/api/android/java/s... File webrtc/api/android/java/src/org/webrtc/ScreenCapturerAndroid.java (right): https://codereview.webrtc.org/2276593003/diff/20001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/ScreenCapturerAndroid.java:34: */ Add @TargetApi(20) here. You need to import android.annotation.TargetApi for that. https://codereview.webrtc.org/2276593003/diff/20001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/ScreenCapturerAndroid.java:38: private final static String TAG = "ScreenCapturerAndroid"; This is currently not used, so you can remove it. Or you can add some logs in important methods. https://codereview.webrtc.org/2276593003/diff/20001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/ScreenCapturerAndroid.java:65: public void setup(VirtualDisplay virtualDisplay, int width, int height) { What's the benefit of creating the VirtualDisplay outside of this class? I.e. can we call MediaProjection.createVirtualDisplay inside this class instead? https://codereview.webrtc.org/2276593003/diff/20001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/ScreenCapturerAndroid.java:114: virtualDisplay.setSurface(new Surface(surfaceTextureHelper.getSurfaceTexture())); This function requires API 20. If we create the VirtualDisply with this Surface directly in createVirtualDisplay instead we can support API 19. https://codereview.webrtc.org/2276593003/diff/20001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/ScreenCapturerAndroid.java:142: public void onOutputFormatRequest(int width, int height, int framerate) {} You can implement this with: surfaceTextureHelper.getHandler().post(new Runnable() { @Override public void run() { capturerObserver.onOutputFormatRequest(width, height, framerate); } }); https://codereview.webrtc.org/2276593003/diff/20001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/ScreenCapturerAndroid.java:145: public void changeCaptureFormat(int width, int height, int framerate) {} Does this function make sense for screencast? We should probably implement it then, and otherwise change the interface so it's only required for cameras. I think it's fine to land this CL without an implementation as a first step though. https://codereview.webrtc.org/2276593003/diff/20001/webrtc/api/android/jni/pe... File webrtc/api/android/jni/peerconnection_jni.cc (right): https://codereview.webrtc.org/2276593003/diff/20001/webrtc/api/android/jni/pe... webrtc/api/android/jni/peerconnection_jni.cc:1295: rtc::scoped_refptr<webrtc::AndroidVideoTrackSource> source( Can you update this create function as well? It should be easy to add a 'bool is_screencast' to the AndroidVideoTrackSource ctor: https://cs.chromium.org/chromium/src/third_party/webrtc/api/androidvideotrack... https://codereview.webrtc.org/2276593003/diff/20001/webrtc/api/androidvideoca... File webrtc/api/androidvideocapturer.h (right): https://codereview.webrtc.org/2276593003/diff/20001/webrtc/api/androidvideoca... webrtc/api/androidvideocapturer.h:79: bool is_screencast_; Make this variable const.
Thanks, Magnus. PTAL. Example code: Creation: ----------- mediaProjection = get user permission and create MediaProjection width,height = get screen dimens and apply any scaling videoCapturer = new ScreenCapturerAndroid(mediaProjection, width, height); videoSource = peerConnectionFactory.createVideoSource(videoCapturer, new MediaConstraints()); Change format: --------------------------------- videoSource.stop(); videoCapturer.changeFormat(w, h, fr); videoSource.restart(); Pause: ------- videoSource.stop() mediaProjection.unregisterCallback(mediaProjectionCallback); // Stop mediaProjection to remove cast icon mediaProjection.stop(); Resume: ---------- mediaProjection = get new media projection (can reuse user permission) videoCapturer.updateMediaProjection(mediaProjection); videoSource.restart(); https://codereview.webrtc.org/2276593003/diff/20001/webrtc/api/android/java/s... File webrtc/api/android/java/src/org/webrtc/ScreenCapturerAndroid.java (right): https://codereview.webrtc.org/2276593003/diff/20001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/ScreenCapturerAndroid.java:34: */ On 2016/08/24 12:19:21, magjed_webrtc wrote: > Add @TargetApi(20) here. You need to import android.annotation.TargetApi for > that. We actually need API 21 for the media projection. I added it. https://codereview.webrtc.org/2276593003/diff/20001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/ScreenCapturerAndroid.java:38: private final static String TAG = "ScreenCapturerAndroid"; On 2016/08/24 12:19:22, magjed_webrtc wrote: > This is currently not used, so you can remove it. Or you can add some logs in > important methods. Done. https://codereview.webrtc.org/2276593003/diff/20001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/ScreenCapturerAndroid.java:65: public void setup(VirtualDisplay virtualDisplay, int width, int height) { On 2016/08/24 12:19:22, magjed_webrtc wrote: > What's the benefit of creating the VirtualDisplay outside of this class? I.e. > can we call MediaProjection.createVirtualDisplay inside this class instead? Originally I made this class create and manage the VirtualDisplay and the MediaProjection but this : 1- Made the class not self-contained because getting the media projection requires the enclosing app to define a manifest activity that calls startActivityForResult() to get use permission. 2- Resulted in some application logic sneaking in this class: when the application needs to show the user that it is not capturing, it has to stop the MediaProjection (to make the system remove the cast icon), and then on restart, the capturer needs to create a new MediaProjection. However, we shouldn't make recreating of MediaProjection part of the start/stop cycle because sometimes we need to restart with the same MediaProjection (as in the case of device orientation change). So, I decided to keep this class self-contained and having a simple start/stop logic. So, I made the application create and manage both VirtualDisplay and MediaProjection. However, during this code review, I realized that I can make this class create and manage the VirtualDisplay and make the app pass in the MediaProjection when needed. I made this change, PTAL. I think it is a bit cleaner and more intuitive now. https://codereview.webrtc.org/2276593003/diff/20001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/ScreenCapturerAndroid.java:114: virtualDisplay.setSurface(new Surface(surfaceTextureHelper.getSurfaceTexture())); On 2016/08/24 12:19:22, magjed_webrtc wrote: > This function requires API 20. If we create the VirtualDisply with this Surface > directly in createVirtualDisplay instead we can support API 19. I removed this function since I made this class create the VirtualDisplay. However, as noted above, we need API 21 for the media projection. https://codereview.webrtc.org/2276593003/diff/20001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/ScreenCapturerAndroid.java:142: public void onOutputFormatRequest(int width, int height, int framerate) {} On 2016/08/24 12:19:22, magjed_webrtc wrote: > You can implement this with: > surfaceTextureHelper.getHandler().post(new Runnable() { > @Override > public void run() { > capturerObserver.onOutputFormatRequest(width, height, framerate); > } > }); Done. https://codereview.webrtc.org/2276593003/diff/20001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/ScreenCapturerAndroid.java:145: public void changeCaptureFormat(int width, int height, int framerate) {} On 2016/08/24 12:19:21, magjed_webrtc wrote: > Does this function make sense for screencast? We should probably implement it > then, and otherwise change the interface so it's only required for cameras. I > think it's fine to land this CL without an implementation as a first step > though. We actually need this functionality, especially when the device orientation changes, or when the app needs to output a lower resolution. Originally I had this functionality implemented as part of the setup() method because the video size and the VirtualDisplay should be changed together. However, now after refactoring and making this class create and manage the VirtualDisplay, I implemented this method. I made it a precondition that the VideoSource is stopped before calling it, otherwise it does not work. Stopping the capturer itself from within this method and restarting it does not work. For this reason, I wonder if this method should be defined at the VideoSource level rather than the VideoCapturer. https://codereview.webrtc.org/2276593003/diff/20001/webrtc/api/android/jni/pe... File webrtc/api/android/jni/peerconnection_jni.cc (right): https://codereview.webrtc.org/2276593003/diff/20001/webrtc/api/android/jni/pe... webrtc/api/android/jni/peerconnection_jni.cc:1295: rtc::scoped_refptr<webrtc::AndroidVideoTrackSource> source( On 2016/08/24 12:19:22, magjed_webrtc wrote: > Can you update this create function as well? It should be easy to add a 'bool > is_screencast' to the AndroidVideoTrackSource ctor: > https://cs.chromium.org/chromium/src/third_party/webrtc/api/androidvideotrack... Done. https://codereview.webrtc.org/2276593003/diff/20001/webrtc/api/androidvideoca... File webrtc/api/androidvideocapturer.h (right): https://codereview.webrtc.org/2276593003/diff/20001/webrtc/api/androidvideoca... webrtc/api/androidvideocapturer.h:79: bool is_screencast_; On 2016/08/24 12:19:22, magjed_webrtc wrote: > Make this variable const. Done.
Please note that videoSource.stop() and videoSource.restart() are probably going away in the future. https://codereview.webrtc.org/2276593003/diff/40001/webrtc/api/android/java/s... File webrtc/api/android/java/src/org/webrtc/ScreenCapturerAndroid.java (right): https://codereview.webrtc.org/2276593003/diff/40001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/ScreenCapturerAndroid.java:35: * Note that startCapture(), stopCapture(), and dispose() are called from native code. This is only the case using the old deprecated API that will be removed. https://codereview.webrtc.org/2276593003/diff/40001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/ScreenCapturerAndroid.java:118: width, height, 1 /* minFrameRate */, 30 /* maxFrameRate */)); Maybe minFrameRate 0? maxFrameRate 30 also seems arbitrary, what if the screen supports 60 fps? https://codereview.webrtc.org/2276593003/diff/40001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/ScreenCapturerAndroid.java:122: // Initially called by native code. This can also be called from native code when the Only the case in the old deprecated API. https://codereview.webrtc.org/2276593003/diff/40001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/ScreenCapturerAndroid.java:126: final int ignored_width, I would prefer using these variables as the format (this.width = width, this.height = height). In the old API these will always be width and height reported by getSupportedFormats. In the new API app will call this method. https://codereview.webrtc.org/2276593003/diff/40001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/ScreenCapturerAndroid.java:142: // Called by native code to pause capturing. Only the case in the old deprecated API. https://codereview.webrtc.org/2276593003/diff/40001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/ScreenCapturerAndroid.java:150: surfaceTextureHelper.stopListening(); You should call capturerObserver.onCapturerStopped() after this. https://codereview.webrtc.org/2276593003/diff/40001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/ScreenCapturerAndroid.java:156: // Called from native code to dispose the capturer when the enclosing VideoSource is disposed. Only the case in the old API. In the new API, app should call this. https://codereview.webrtc.org/2276593003/diff/40001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/ScreenCapturerAndroid.java:186: public void changeCaptureFormat(int width, int height, int ignored_framerate) { I think this should match the behavior of camera implementations and allow calling this while capturing.
https://codereview.webrtc.org/2276593003/diff/20001/webrtc/api/android/java/s... File webrtc/api/android/java/src/org/webrtc/ScreenCapturerAndroid.java (right): https://codereview.webrtc.org/2276593003/diff/20001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/ScreenCapturerAndroid.java:65: public void setup(VirtualDisplay virtualDisplay, int width, int height) { On 2016/08/26 02:07:36, arsany wrote: > On 2016/08/24 12:19:22, magjed_webrtc wrote: > > What's the benefit of creating the VirtualDisplay outside of this class? I.e. > > can we call MediaProjection.createVirtualDisplay inside this class instead? > > Originally I made this class create and manage the VirtualDisplay and the > MediaProjection but this : > 1- Made the class not self-contained because getting the media projection > requires the enclosing app to define a manifest activity that calls > startActivityForResult() to get use permission. > 2- Resulted in some application logic sneaking in this class: when the > application needs to show the user that it is not capturing, it has to stop the > MediaProjection (to make the system remove the cast icon), and then on restart, > the capturer needs to create a new MediaProjection. However, we shouldn't make > recreating of MediaProjection part of the start/stop cycle because sometimes we > need to restart with the same MediaProjection (as in the case of device > orientation change). > > So, I decided to keep this class self-contained and having a simple start/stop > logic. So, I made the application create and manage both VirtualDisplay and > MediaProjection. However, during this code review, I realized that I can make > this class create and manage the VirtualDisplay and make the app pass in the > MediaProjection when needed. I made this change, PTAL. I think it is a bit > cleaner and more intuitive now. I see. I agree that we should keep the class self-contained and avoid having clients changing their manifest if they want to use this class. However, I would like to move as much as possible of the code that all clients will need into this class. So would it be possible to: - Let the app handle permission and creating the intent and be responsible for obtaining 'int requestCode' and 'int resultCode' from onActivityResult. - Pass requestCode and resultCode into the constructor in this class instead of passing MediaProjection. - Let this class be responsible for handling the MediaProjection, i.e. create it in startCapture() and close it in stopCapture(). In the case of orientation change or a call to changeCaptureFormat(), this class does not need to recreate the MediaProjection. - Let this class be responsible for handling orientation changes and updating the resolution. It might have to recreate stuff internally, but externally it can just change the frame rotation parameter.
Thanks, Magnus, and Sami I made this class manage MediaProjection. I had to pass in a callback to allow application specific logic if the user revokes the capture permission after initially granting it (this can happen through the cast icon in Android). The logic for detecting orientation change is kept outside this class because it has application specifics. You may see the internal CL I shared by email for the app that uses this class. https://codereview.webrtc.org/2276593003/diff/40001/webrtc/api/android/java/s... File webrtc/api/android/java/src/org/webrtc/ScreenCapturerAndroid.java (right): https://codereview.webrtc.org/2276593003/diff/40001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/ScreenCapturerAndroid.java:35: * Note that startCapture(), stopCapture(), and dispose() are called from native code. On 2016/08/26 07:21:49, sakal wrote: > This is only the case using the old deprecated API that will be removed. Thanks, Sami. I wasn't aware of API deprecation, would it be OK to land this CL with the old API assumptions and migrate later when all code is migrated off the deprecated APIs ? Also, is there documentation for the change somewhere? What do we need to change to use the new API? Is the new API ready for use ? is it used in any apps yet? https://codereview.webrtc.org/2276593003/diff/40001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/ScreenCapturerAndroid.java:118: width, height, 1 /* minFrameRate */, 30 /* maxFrameRate */)); On 2016/08/26 07:21:49, sakal wrote: > Maybe minFrameRate 0? maxFrameRate 30 also seems arbitrary, what if the screen > supports 60 fps? Done. Changed to 0, 60. But I am actually not certain about the impact of these params in our case. Could you please explain briefly how they are used internally. Thanks. https://codereview.webrtc.org/2276593003/diff/40001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/ScreenCapturerAndroid.java:122: // Initially called by native code. This can also be called from native code when the On 2016/08/26 07:21:49, sakal wrote: > Only the case in the old deprecated API. Acknowledged. https://codereview.webrtc.org/2276593003/diff/40001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/ScreenCapturerAndroid.java:126: final int ignored_width, On 2016/08/26 07:21:49, sakal wrote: > I would prefer using these variables as the format (this.width = width, > this.height = height). In the old API these will always be width and height > reported by getSupportedFormats. In the new API app will call this method. Within the old API, getSupportedFormats() get called once in the beginning. We want the app to change format freely (using changeOutputFormat), that's why we can't use the input params here because they will always be the same as the first start. It sounds like the new API will solve this issue. For now, using this old API, I think we need to continue ignoring those params so that the app can have control. The app needs control for example to switch dimensions when the device orientation changes. https://codereview.webrtc.org/2276593003/diff/40001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/ScreenCapturerAndroid.java:142: // Called by native code to pause capturing. On 2016/08/26 07:21:48, sakal wrote: > Only the case in the old deprecated API. Acknowledged. https://codereview.webrtc.org/2276593003/diff/40001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/ScreenCapturerAndroid.java:150: surfaceTextureHelper.stopListening(); On 2016/08/26 07:21:49, sakal wrote: > You should call capturerObserver.onCapturerStopped() after this. Done. https://codereview.webrtc.org/2276593003/diff/40001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/ScreenCapturerAndroid.java:156: // Called from native code to dispose the capturer when the enclosing VideoSource is disposed. On 2016/08/26 07:21:48, sakal wrote: > Only the case in the old API. In the new API, app should call this. Acknowledged. https://codereview.webrtc.org/2276593003/diff/40001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/ScreenCapturerAndroid.java:186: public void changeCaptureFormat(int width, int height, int ignored_framerate) { On 2016/08/26 07:21:49, sakal wrote: > I think this should match the behavior of camera implementations and allow > calling this while capturing. Done. Agreed. PTAL
https://codereview.webrtc.org/2276593003/diff/40001/webrtc/api/android/java/s... File webrtc/api/android/java/src/org/webrtc/ScreenCapturerAndroid.java (right): https://codereview.webrtc.org/2276593003/diff/40001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/ScreenCapturerAndroid.java:35: * Note that startCapture(), stopCapture(), and dispose() are called from native code. On 2016/08/30 00:52:15, arsany wrote: > On 2016/08/26 07:21:49, sakal wrote: > > This is only the case using the old deprecated API that will be removed. > > Thanks, Sami. > I wasn't aware of API deprecation, would it be OK to land this CL with the old > API assumptions and migrate later when all code is migrated off the deprecated > APIs ? > Also, is there documentation for the change somewhere? What do we need to change > to use the new API? Is the new API ready for use ? is it used in any apps yet? The only real documentation of the API change is the CL implementing the change. https://codereview.webrtc.org/2127893002/ The changes needed from the app are described in the CL description. No changes should be needed to the capturer. See also how AppRTC Demo Android was ported: https://codereview.webrtc.org/2127893002/patch/180001/190015 The new API is already used by some apps downstream. I tried to remove the old API yesterday but there were a few remaining that I wasn't aware of. I will email you more details. https://codereview.webrtc.org/2276593003/diff/40001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/ScreenCapturerAndroid.java:118: width, height, 1 /* minFrameRate */, 30 /* maxFrameRate */)); On 2016/08/30 00:52:15, arsany wrote: > On 2016/08/26 07:21:49, sakal wrote: > > Maybe minFrameRate 0? maxFrameRate 30 also seems arbitrary, what if the screen > > supports 60 fps? > > Done. Changed to 0, 60. But I am actually not certain about the impact of these > params in our case. Could you please explain briefly how they are used > internally. Thanks. I didn't really say we should change it to 60. I was just saying that maybe it should be something more eloquent than magic constant. However, if there is no easy way of for example querying the monitor sync rate, I am okay with it. https://codereview.webrtc.org/2276593003/diff/40001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/ScreenCapturerAndroid.java:126: final int ignored_width, On 2016/08/30 00:52:15, arsany wrote: > On 2016/08/26 07:21:49, sakal wrote: > > I would prefer using these variables as the format (this.width = width, > > this.height = height). In the old API these will always be width and height > > reported by getSupportedFormats. In the new API app will call this method. > > Within the old API, getSupportedFormats() get called once in the beginning. > We want the app to change format freely (using changeOutputFormat), that's why > we can't use the input params here because they will always be the same as the > first start. > It sounds like the new API will solve this issue. For now, using this old API, I > think we need to continue ignoring those params so that the app can have > control. > The app needs control for example to switch dimensions when the device > orientation changes. Hmm, I see. I would prefer you use the new API though. The old API will be removed soon.
I think this is starting to look good and that we can land it soon. We can integrate it in AppRTCDemo in a follow-up CL. I would also like to add some kind of default orientation handling in a follow-up CL. https://codereview.webrtc.org/2276593003/diff/40001/webrtc/api/android/java/s... File webrtc/api/android/java/src/org/webrtc/ScreenCapturerAndroid.java (right): https://codereview.webrtc.org/2276593003/diff/40001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/ScreenCapturerAndroid.java:126: final int ignored_width, On 2016/08/30 07:30:13, sakal wrote: > On 2016/08/30 00:52:15, arsany wrote: > > On 2016/08/26 07:21:49, sakal wrote: > > > I would prefer using these variables as the format (this.width = width, > > > this.height = height). In the old API these will always be width and height > > > reported by getSupportedFormats. In the new API app will call this method. > > > > Within the old API, getSupportedFormats() get called once in the beginning. > > We want the app to change format freely (using changeOutputFormat), that's why > > we can't use the input params here because they will always be the same as the > > first start. > > It sounds like the new API will solve this issue. For now, using this old API, > I > > think we need to continue ignoring those params so that the app can have > > control. > > The app needs control for example to switch dimensions when the device > > orientation changes. > > Hmm, I see. I would prefer you use the new API though. The old API will be > removed soon. Yes, please update to the new API in your client, and then use the input parameters here. getSupportedFormats() will not even be called in the new API, and we might remove it as a requirement from the VideoCapturer interface, because I'm not sure it makes sense for e.g. a ScreenCapturer. So in your client, you basically need to change: peerConnectionFactory.createVideoSource(videoCapturer, new MediaConstraints()); to peerConnectionFactory.createVideoSource(videoCapturer); and videoSource.restart(); to videoCapturer.startCapture(width, height, framerate); and videoSource.stop(); to videoCapturer.stopCapture(); https://codereview.webrtc.org/2276593003/diff/60001/webrtc/api/android/java/s... File webrtc/api/android/java/src/org/webrtc/ScreenCapturerAndroid.java (right): https://codereview.webrtc.org/2276593003/diff/60001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/ScreenCapturerAndroid.java:76: public ScreenCapturerAndroid(int width, int height, You should be able to remove width and height as arguments to the ctor if you start using the new API and call startCapture directly with the desired width/height instead. https://codereview.webrtc.org/2276593003/diff/60001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/ScreenCapturerAndroid.java:136: mediaProjection.registerCallback(mediaProjectionCallback, null); Instead of invoking, you can just pass the handler as the second argument: mediaProjection.registerCallback(mediaProjectionCallback, surfaceTextureHelper.getHandler());
Thanks, all done. In a follow-up CL we can pass a config object telling the capturer how to handle device orientation changes. For example: ignore them (which is probably the right thing if the destination window on the other peer is not resized), or allow them with min gap duration (otherwise we freeze if the user switches too often). We can also pass in a scaling config since screen resolutions are often too high and the encoder does not reduce resolution of screencasts. Right now, all this is handled in our app. https://codereview.webrtc.org/2276593003/diff/40001/webrtc/api/android/java/s... File webrtc/api/android/java/src/org/webrtc/ScreenCapturerAndroid.java (right): https://codereview.webrtc.org/2276593003/diff/40001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/ScreenCapturerAndroid.java:35: * Note that startCapture(), stopCapture(), and dispose() are called from native code. On 2016/08/30 07:30:13, sakal wrote: > On 2016/08/30 00:52:15, arsany wrote: > > On 2016/08/26 07:21:49, sakal wrote: > > > This is only the case using the old deprecated API that will be removed. > > > > Thanks, Sami. > > I wasn't aware of API deprecation, would it be OK to land this CL with the old > > API assumptions and migrate later when all code is migrated off the deprecated > > APIs ? > > Also, is there documentation for the change somewhere? What do we need to > change > > to use the new API? Is the new API ready for use ? is it used in any apps yet? > > The only real documentation of the API change is the CL implementing the change. > https://codereview.webrtc.org/2127893002/ The changes needed from the app are > described in the CL description. No changes should be needed to the capturer. > See also how AppRTC Demo Android was ported: > https://codereview.webrtc.org/2127893002/patch/180001/190015 > > The new API is already used by some apps downstream. I tried to remove the old > API yesterday but there were a few remaining that I wasn't aware of. I will > email you more details. Acknowledged. https://codereview.webrtc.org/2276593003/diff/40001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/ScreenCapturerAndroid.java:118: width, height, 1 /* minFrameRate */, 30 /* maxFrameRate */)); On 2016/08/30 07:30:13, sakal wrote: > On 2016/08/30 00:52:15, arsany wrote: > > On 2016/08/26 07:21:49, sakal wrote: > > > Maybe minFrameRate 0? maxFrameRate 30 also seems arbitrary, what if the > screen > > > supports 60 fps? > > > > Done. Changed to 0, 60. But I am actually not certain about the impact of > these > > params in our case. Could you please explain briefly how they are used > > internally. Thanks. > > I didn't really say we should change it to 60. I was just saying that maybe it > should be something more eloquent than magic constant. However, if there is no > easy way of for example querying the monitor sync rate, I am okay with it. Removed this altogether. https://codereview.webrtc.org/2276593003/diff/40001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/ScreenCapturerAndroid.java:122: // Initially called by native code. This can also be called from native code when the On 2016/08/26 07:21:49, sakal wrote: > Only the case in the old deprecated API. Done. https://codereview.webrtc.org/2276593003/diff/40001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/ScreenCapturerAndroid.java:126: final int ignored_width, On 2016/08/30 10:50:54, magjed_webrtc wrote: > On 2016/08/30 07:30:13, sakal wrote: > > On 2016/08/30 00:52:15, arsany wrote: > > > On 2016/08/26 07:21:49, sakal wrote: > > > > I would prefer using these variables as the format (this.width = width, > > > > this.height = height). In the old API these will always be width and > height > > > > reported by getSupportedFormats. In the new API app will call this method. > > > > > > Within the old API, getSupportedFormats() get called once in the beginning. > > > We want the app to change format freely (using changeOutputFormat), that's > why > > > we can't use the input params here because they will always be the same as > the > > > first start. > > > It sounds like the new API will solve this issue. For now, using this old > API, > > I > > > think we need to continue ignoring those params so that the app can have > > > control. > > > The app needs control for example to switch dimensions when the device > > > orientation changes. > > > > Hmm, I see. I would prefer you use the new API though. The old API will be > > removed soon. > > Yes, please update to the new API in your client, and then use the input > parameters here. getSupportedFormats() will not even be called in the new API, > and we might remove it as a requirement from the VideoCapturer interface, > because I'm not sure it makes sense for e.g. a ScreenCapturer. So in your > client, you basically need to change: > peerConnectionFactory.createVideoSource(videoCapturer, new MediaConstraints()); > to > peerConnectionFactory.createVideoSource(videoCapturer); > and > videoSource.restart(); > to > videoCapturer.startCapture(width, height, framerate); > and > videoSource.stop(); > to > videoCapturer.stopCapture(); Done. https://codereview.webrtc.org/2276593003/diff/40001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/ScreenCapturerAndroid.java:142: // Called by native code to pause capturing. On 2016/08/26 07:21:48, sakal wrote: > Only the case in the old deprecated API. Done. https://codereview.webrtc.org/2276593003/diff/40001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/ScreenCapturerAndroid.java:156: // Called from native code to dispose the capturer when the enclosing VideoSource is disposed. On 2016/08/26 07:21:48, sakal wrote: > Only the case in the old API. In the new API, app should call this. Done. https://codereview.webrtc.org/2276593003/diff/60001/webrtc/api/android/java/s... File webrtc/api/android/java/src/org/webrtc/ScreenCapturerAndroid.java (right): https://codereview.webrtc.org/2276593003/diff/60001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/ScreenCapturerAndroid.java:76: public ScreenCapturerAndroid(int width, int height, On 2016/08/30 10:50:54, magjed_webrtc wrote: > You should be able to remove width and height as arguments to the ctor if you > start using the new API and call startCapture directly with the desired > width/height instead. Done. https://codereview.webrtc.org/2276593003/diff/60001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/ScreenCapturerAndroid.java:136: mediaProjection.registerCallback(mediaProjectionCallback, null); On 2016/08/30 10:50:54, magjed_webrtc wrote: > Instead of invoking, you can just pass the handler as the second argument: > mediaProjection.registerCallback(mediaProjectionCallback, > surfaceTextureHelper.getHandler()); Done.
magjed@webrtc.org changed reviewers: - perkj@webrtc.org, tommi@webrtc.org
Description was changed from ========== Create Android screen capturer. BUG= ========== to ========== Create Android screen capturer. ==========
lgtm
The CQ bit was checked by arsany@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/16331)
The CQ bit was checked by arsany@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org Link to the patchset: https://codereview.webrtc.org/2276593003/#ps100001 (title: "Merge conflicts with https://codereview.webrtc.org/2298063003")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/7921)
The CQ bit was checked by arsany@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org Link to the patchset: https://codereview.webrtc.org/2276593003/#ps120001 (title: "merge master")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by arsany@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Create Android screen capturer. ========== to ========== Create Android screen capturer. Committed: https://crrev.com/b75f2541c942e2f35c3b7d7003ed17504176ced1 Cr-Commit-Position: refs/heads/master@{#14010} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/b75f2541c942e2f35c3b7d7003ed17504176ced1 Cr-Commit-Position: refs/heads/master@{#14010} |
