Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2273573003:
Support for video file instead of camera and output video out to file (Closed)
DescriptionSupport for video file instead of camera and output video out to file
When video out to file is enabled the remote video which is recorded is
not show on screen.
You can use this command line for file input and output:
monkeyrunner ./webrtc/examples/androidapp/start_loopback_stubbed_camera_saved_video_out.py --devname 02157df28cd47001 --videoin /storage/emulated/0/reference_video_1280x720_30fps.y4m --videoout /storage/emulated/0/output.y4m --videoout_width 1280 --videoout_height 720 --videooutsave /tmp/out.y4m
BUG=webrtc:6545
Committed: https://crrev.com/44666997ca912705f8f96c9bd211e719525a3ccc
Cr-Commit-Position: refs/heads/master@{#14660}
Patch Set 1 #Patch Set 2 : Added file to start automated loopback run #
Total comments: 28
Patch Set 3 : Refactoring of FileVideoCapturer with separate timer class #Patch Set 4 : Merge of recent changes on master and change to be compatible with that #Patch Set 5 : \ #
Total comments: 11
Patch Set 6 : Improved error handling for video in and video out to file #
Total comments: 29
Patch Set 7 : Changed based on review comments #Patch Set 8 : Added . to comments #
Total comments: 75
Patch Set 9 : Fixing review comments, except unittesting #
Total comments: 51
Patch Set 10 : Fixed review comments. Unittests in follow up CL. #
Total comments: 24
Patch Set 11 : Fixed review comments #Patch Set 12 : Fixed bug from refactoring frameAspectRatio in VideoFileRenderer #Patch Set 13 : Better synchronization of release in VideoFileRenderer #
Total comments: 6
Patch Set 14 : Removed unnecessary mutex in VideoFileRenderer and did other review fixes #Patch Set 15 : Fixed I introduced when changing from HandlerThread.quitSafely() to HandlerThread.quit() #Patch Set 16 : Merge changes from master #Patch Set 17 : Fixed copyright header for start_loopback_stubbed_camera_saved_video_out.py #Messages
Total messages: 76 (44 generated)
kjellander@webrtc.org changed reviewers: + kjellander@webrtc.org, magjed@webrtc.org
I realize I don't know this code very well so I'd prefer if Magnus takes the lead on the review. I've asked Magnus to strip down the command line to what's the minimum required to get this working (for a loopback call). Ideally you'll also write a short summary of what you're doing in this CL. I'd also like if you can provide minimal command line for what to run if you have Device A and B and want them to call each other.
On 2016/08/24 12:59:45, kjellander_webrtc wrote: > I realize I don't know this code very well so I'd prefer if Magnus takes the > lead on the review. > I've asked Magnus to strip down the command line to what's the minimum required > to get this working (for a loopback call). > > Ideally you'll also write a short summary of what you're doing in this CL. > I'd also like if you can provide minimal command line for what to run if you > have Device A and B and want them to call each other. Here is a minimal command line for loop back call, where camera input comes from file instead of the camera. The remote screen which show the other end is saved to file also. echo 'am start -a android.intent.action.VIEW -n org.appspot.apprtc/.CallActivity -d https://appr.tc --es org.appspot.apprtc.AUDIOCODEC OPUS --ez org.appspot.apprtc.LOOPBACK true --es org.appspot.apprtc.VIDEOCODEC VP8 --ez org.appspot.apprtc.CAPTURETOTEXTURE false --ez org.appspot.apprtc.CAMERA2 false --es org.appspot.apprtc.VIDEO_FILE_AS_CAMERA "/storage/emulated/0/nv21_1280x720.yuv" --ei org.appspot.apprtc.VIDEO_FILE_AS_CAMERA_WIDTH 1280 --ei org.appspot.apprtc.VIDEO_FILE_AS_CAMERA_HEIGHT 720 --es org.appspot.apprtc.SAVE_REMOTE_VIDEO_TO_FILE "/storage/emulated/0/output.y4m" --ei org.appspot.apprtc.SAVE_REMOTE_VIDEO_TO_FILE_WIDTH 1280 --ei org.appspot.apprtc.SAVE_REMOTE_VIDEO_TO_FILE_HEIGHT 720 --es org.appspot.apprtc.ROOMID man1' > cmd.sh && adb push cmd.sh /data/local/tmp/. && adb shell sh /data/local/tmp/cmd.sh To make a loopback call, you can create a bash script like this (start_inp_out.sh): #!/bin/bash adevice=$1 echo "$adevice" room=$(uuidgen | cut -d- -f1) #$(uuidgen) echo "roomid: $room" # Input side inpdev=$1 echo "am start -a android.intent.action.VIEW -n org.appspot.apprtc/.CallActivity -d https://appr.tc --es org.appspot.apprtc.AUDIOCODEC OPUS --ez org.appspot.apprtc.LOOPBACK false --es org.appspot.apprtc.VIDEOCODEC VP8 --ez org.appspot.apprtc.CAPTURETOTEXTURE false --ez org.appspot.apprtc.CAMERA2 false --es org.appspot.apprtc.VIDEO_FILE_AS_CAMERA \"/storage/emulated/0/nv21_1280x720.yuv\" --ei org.appspot.apprtc.VIDEO_FILE_AS_CAMERA_WIDTH 1280 --ei org.appspot.apprtc.VIDEO_FILE_AS_CAMERA_HEIGHT 720 --es org.appspot.apprtc.ROOMID $room" > cmd.sh && adb -s $inpdev push cmd.sh /data/local/tmp/. && adb -s $inpdev shell sh /data/local/tmp/cmd.sh # Output side outdev=$2 echo "am start -a android.intent.action.VIEW -n org.appspot.apprtc/.CallActivity -d https://appr.tc --es org.appspot.apprtc.AUDIOCODEC OPUS --ez org.appspot.apprtc.LOOPBACK false --es org.appspot.apprtc.VIDEOCODEC VP8 --ez org.appspot.apprtc.CAPTURETOTEXTURE false --ez org.appspot.apprtc.CAMERA2 false --es org.appspot.apprtc.SAVE_REMOTE_VIDEO_TO_FILE \"/storage/emulated/0/output.y4m\" --ei org.appspot.apprtc.SAVE_REMOTE_VIDEO_TO_FILE_WIDTH 1280 --ei org.appspot.apprtc.SAVE_REMOTE_VIDEO_TO_FILE_HEIGHT 720 --es org.appspot.apprtc.ROOMID $room" > cmd.sh && adb -s $outdev push cmd.sh /data/local/tmp/. && adb -s $outdev shell sh /data/local/tmp/cmd.sh ---------------------- The first argument to start_inp_out.sh is the device that should take video input from file and the second argument is the device that should write video output to file. You run it like this: mandermo@mandermo:~/code/webrtc/src$ adb devices List of devices attached 02157df28cd47001 device 03157df3e30cdd28 device mandermo@mandermo:~/code/webrtc/src$ ~/code/controlphones/start_inp_out.sh 03157df3e30cdd28 02157df28cd47001
On 2016/08/26 08:16:05, mandermo wrote: > On 2016/08/24 12:59:45, kjellander_webrtc wrote: > > I realize I don't know this code very well so I'd prefer if Magnus takes the > > lead on the review. > > I've asked Magnus to strip down the command line to what's the minimum > required > > to get this working (for a loopback call). > > > > Ideally you'll also write a short summary of what you're doing in this CL. > > I'd also like if you can provide minimal command line for what to run if you > > have Device A and B and want them to call each other. > > Here is a minimal command line for loop back call, where camera input comes from > file instead of the camera. The remote screen which show the other end is saved > to file also. > echo 'am start -a android.intent.action.VIEW -n org.appspot.apprtc/.CallActivity > -d https://appr.tc --es org.appspot.apprtc.AUDIOCODEC OPUS --ez > org.appspot.apprtc.LOOPBACK true --es org.appspot.apprtc.VIDEOCODEC VP8 --ez > org.appspot.apprtc.CAPTURETOTEXTURE false --ez org.appspot.apprtc.CAMERA2 false > --es org.appspot.apprtc.VIDEO_FILE_AS_CAMERA > "/storage/emulated/0/nv21_1280x720.yuv" --ei > org.appspot.apprtc.VIDEO_FILE_AS_CAMERA_WIDTH 1280 --ei > org.appspot.apprtc.VIDEO_FILE_AS_CAMERA_HEIGHT 720 --es > org.appspot.apprtc.SAVE_REMOTE_VIDEO_TO_FILE "/storage/emulated/0/output.y4m" > --ei org.appspot.apprtc.SAVE_REMOTE_VIDEO_TO_FILE_WIDTH 1280 --ei > org.appspot.apprtc.SAVE_REMOTE_VIDEO_TO_FILE_HEIGHT 720 --es > org.appspot.apprtc.ROOMID man1' > cmd.sh && adb push cmd.sh /data/local/tmp/. && > adb shell sh /data/local/tmp/cmd.sh > > To make a loopback call, you can create a bash script like this > (start_inp_out.sh): > #!/bin/bash > > adevice=$1 echo "$adevice" > > room=$(uuidgen | cut -d- -f1) #$(uuidgen) > echo "roomid: $room" > > # Input side > inpdev=$1 > echo "am start -a android.intent.action.VIEW -n org.appspot.apprtc/.CallActivity > -d https://appr.tc --es org.appspot.apprtc.AUDIOCODEC OPUS --ez > org.appspot.apprtc.LOOPBACK false --es org.appspot.apprtc.VIDEOCODEC VP8 --ez > org.appspot.apprtc.CAPTURETOTEXTURE false --ez org.appspot.apprtc.CAMERA2 false > --es org.appspot.apprtc.VIDEO_FILE_AS_CAMERA > \"/storage/emulated/0/nv21_1280x720.yuv\" --ei > org.appspot.apprtc.VIDEO_FILE_AS_CAMERA_WIDTH 1280 --ei > org.appspot.apprtc.VIDEO_FILE_AS_CAMERA_HEIGHT 720 --es > org.appspot.apprtc.ROOMID $room" > cmd.sh && adb -s $inpdev push cmd.sh > /data/local/tmp/. && adb -s $inpdev shell sh /data/local/tmp/cmd.sh > > > # Output side > outdev=$2 > echo "am start -a android.intent.action.VIEW -n org.appspot.apprtc/.CallActivity > -d https://appr.tc --es org.appspot.apprtc.AUDIOCODEC OPUS --ez > org.appspot.apprtc.LOOPBACK false --es org.appspot.apprtc.VIDEOCODEC VP8 --ez > org.appspot.apprtc.CAPTURETOTEXTURE false --ez org.appspot.apprtc.CAMERA2 false > --es org.appspot.apprtc.SAVE_REMOTE_VIDEO_TO_FILE > \"/storage/emulated/0/output.y4m\" --ei > org.appspot.apprtc.SAVE_REMOTE_VIDEO_TO_FILE_WIDTH 1280 --ei > org.appspot.apprtc.SAVE_REMOTE_VIDEO_TO_FILE_HEIGHT 720 --es > org.appspot.apprtc.ROOMID $room" > cmd.sh && adb -s $outdev push cmd.sh > /data/local/tmp/. && adb -s $outdev shell sh /data/local/tmp/cmd.sh > > > ---------------------- > > The first argument to start_inp_out.sh is the device that should take video > input from file and the second argument is the device that should write video > output to file. You run it like this: > mandermo@mandermo:~/code/webrtc/src$ adb devices > List of devices attached > 02157df28cd47001 device > 03157df3e30cdd28 device > mandermo@mandermo:~/code/webrtc/src$ ~/code/controlphones/start_inp_out.sh > 03157df3e30cdd28 02157df28cd47001 Thanks for the thorough explanation. Unfortunately only what's written in the description of a CL it put into the commit message. Since these instructions are very long, I suggest you copy all relevant stuff into a new section "Using prerecorded video instead of camera and writing output to disk" in webrtc/examples/androidapp/README. Also update the CL description to not include the verbose example - refer to webrtc/examples/androidapp/README instead for usage.
On 2016/08/26 13:15:32, kjellander_webrtc wrote: > On 2016/08/26 08:16:05, mandermo wrote: > > On 2016/08/24 12:59:45, kjellander_webrtc wrote: > > > I realize I don't know this code very well so I'd prefer if Magnus takes the > > > lead on the review. > > > I've asked Magnus to strip down the command line to what's the minimum > > required > > > to get this working (for a loopback call). > > > > > > Ideally you'll also write a short summary of what you're doing in this CL. > > > I'd also like if you can provide minimal command line for what to run if you > > > have Device A and B and want them to call each other. > > > > Here is a minimal command line for loop back call, where camera input comes > from > > file instead of the camera. The remote screen which show the other end is > saved > > to file also. > > echo 'am start -a android.intent.action.VIEW -n > org.appspot.apprtc/.CallActivity > > -d https://appr.tc --es org.appspot.apprtc.AUDIOCODEC OPUS --ez > > org.appspot.apprtc.LOOPBACK true --es org.appspot.apprtc.VIDEOCODEC VP8 --ez > > org.appspot.apprtc.CAPTURETOTEXTURE false --ez org.appspot.apprtc.CAMERA2 > false > > --es org.appspot.apprtc.VIDEO_FILE_AS_CAMERA > > "/storage/emulated/0/nv21_1280x720.yuv" --ei > > org.appspot.apprtc.VIDEO_FILE_AS_CAMERA_WIDTH 1280 --ei > > org.appspot.apprtc.VIDEO_FILE_AS_CAMERA_HEIGHT 720 --es > > org.appspot.apprtc.SAVE_REMOTE_VIDEO_TO_FILE "/storage/emulated/0/output.y4m" > > --ei org.appspot.apprtc.SAVE_REMOTE_VIDEO_TO_FILE_WIDTH 1280 --ei > > org.appspot.apprtc.SAVE_REMOTE_VIDEO_TO_FILE_HEIGHT 720 --es > > org.appspot.apprtc.ROOMID man1' > cmd.sh && adb push cmd.sh /data/local/tmp/. > && > > adb shell sh /data/local/tmp/cmd.sh > > > > To make a loopback call, you can create a bash script like this > > (start_inp_out.sh): > > #!/bin/bash > > > > adevice=$1 echo "$adevice" > > > > room=$(uuidgen | cut -d- -f1) #$(uuidgen) > > echo "roomid: $room" > > > > # Input side > > inpdev=$1 > > echo "am start -a android.intent.action.VIEW -n > org.appspot.apprtc/.CallActivity > > -d https://appr.tc --es org.appspot.apprtc.AUDIOCODEC OPUS --ez > > org.appspot.apprtc.LOOPBACK false --es org.appspot.apprtc.VIDEOCODEC VP8 --ez > > org.appspot.apprtc.CAPTURETOTEXTURE false --ez org.appspot.apprtc.CAMERA2 > false > > --es org.appspot.apprtc.VIDEO_FILE_AS_CAMERA > > \"/storage/emulated/0/nv21_1280x720.yuv\" --ei > > org.appspot.apprtc.VIDEO_FILE_AS_CAMERA_WIDTH 1280 --ei > > org.appspot.apprtc.VIDEO_FILE_AS_CAMERA_HEIGHT 720 --es > > org.appspot.apprtc.ROOMID $room" > cmd.sh && adb -s $inpdev push cmd.sh > > /data/local/tmp/. && adb -s $inpdev shell sh /data/local/tmp/cmd.sh > > > > > > # Output side > > outdev=$2 > > echo "am start -a android.intent.action.VIEW -n > org.appspot.apprtc/.CallActivity > > -d https://appr.tc --es org.appspot.apprtc.AUDIOCODEC OPUS --ez > > org.appspot.apprtc.LOOPBACK false --es org.appspot.apprtc.VIDEOCODEC VP8 --ez > > org.appspot.apprtc.CAPTURETOTEXTURE false --ez org.appspot.apprtc.CAMERA2 > false > > --es org.appspot.apprtc.SAVE_REMOTE_VIDEO_TO_FILE > > \"/storage/emulated/0/output.y4m\" --ei > > org.appspot.apprtc.SAVE_REMOTE_VIDEO_TO_FILE_WIDTH 1280 --ei > > org.appspot.apprtc.SAVE_REMOTE_VIDEO_TO_FILE_HEIGHT 720 --es > > org.appspot.apprtc.ROOMID $room" > cmd.sh && adb -s $outdev push cmd.sh > > /data/local/tmp/. && adb -s $outdev shell sh /data/local/tmp/cmd.sh > > > > > > ---------------------- > > > > The first argument to start_inp_out.sh is the device that should take video > > input from file and the second argument is the device that should write video > > output to file. You run it like this: > > mandermo@mandermo:~/code/webrtc/src$ adb devices > > List of devices attached > > 02157df28cd47001 device > > 03157df3e30cdd28 device > > mandermo@mandermo:~/code/webrtc/src$ ~/code/controlphones/start_inp_out.sh > > 03157df3e30cdd28 02157df28cd47001 > > Thanks for the thorough explanation. Unfortunately only what's written in the > description of a CL it put into the commit message. > Since these instructions are very long, I suggest you copy all relevant stuff > into a new section "Using prerecorded video instead of camera and writing output > to disk" in webrtc/examples/androidapp/README. > > Also update the CL description to not include the verbose example - refer to > webrtc/examples/androidapp/README instead for usage. Actually, I think it would be even better to just check in the shell script you suggested into webrtc/examples/androidapp/ ?
Description was changed from ========== Support for video file instead of camera and output video out to file When video out to file is enabled the remote video which is recorded is not show on screen. You can use this command line for file input and output: echo 'am start -a android.intent.action.VIEW -n org.appspot.apprtc/.CallActivity -d https://appr.tc --ez org.appspot.apprtc.DISPLAY_HUD false --ei org.appspot.apprtc.VIDEO_BITRATE 0 --ez org.appspot.apprtc.CMDLINE false --ei org.appspot.apprtc.VIDEO_HEIGHT 0 --es org.appspot.apprtc.AUDIOCODEC OPUS --ei org.appspot.apprtc.VIDEO_WIDTH 0 --ez org.appspot.apprtc.HWCODEC true --ei org.appspot.apprtc.VIDEO_FPS 0 --ez org.appsopt.apprtc.VIDEO_CAPTUREQUALITYSLIDER false --ez org.appspot.apprtc.NOAUDIOPROCESSING false --ei org.appspot.apprtc.RUNTIME 0 --ez org.appspot.apprtc.LOOPBACK true --es org.appspot.apprtc.VIDEOCODEC VP8 --ez org.appspot.apprtc.VIDEO_CALL true --ez org.appspot.apprtc.DISABLE_BUILT_IN_AEC false --ez org.appspot.apprtc.CAPTURETOTEXTURE false --ez org.appspot.apprtc.CAMERA2 false --ez org.appspot.apprtc.AECDUMP false --ez org.appspot.apprtc.TRACING false --ei org.appspot.apprtc.AUDIO_BITRATE 0 --ez org.appspot.apprtc.OPENSLES false --es org.appspot.apprtc.VIDEO_FILE_AS_CAMERA "/storage/emulated/0/nv21_1280x720.yuv" --ei org.appspot.apprtc.VIDEO_FILE_AS_CAMERA_WIDTH 1280 --ei org.appspot.apprtc.VIDEO_FILE_AS_CAMERA_HEIGHT 720 --es org.appspot.apprtc.SAVE_REMOTE_VIDEO_TO_FILE "/storage/emulated/0/output.y4m" --ei org.appspot.apprtc.SAVE_REMOTE_VIDEO_TO_FILE_WIDTH 1280 --ei org.appspot.apprtc.SAVE_REMOTE_VIDEO_TO_FILE_HEIGHT 720 --es org.appspot.apprtc.ROOMID man1' > cmd.sh && adb push cmd.sh /data/local/tmp/. && adb shell sh /data/local/tmp/cmd.sh ========== to ========== Support for video file instead of camera and output video out to file When video out to file is enabled the remote video which is recorded is not show on screen. You can use this command line for file input and output: room=$(uuidgen | cut -d- -f1); echo "am start -a android.intent.action.VIEW -n org.appspot.apprtc/.CallActivity -d https://appr.tc --es org.appspot.apprtc.AUDIOCODEC OPUS --ez org.appspot.apprtc.LOOPBACK true --es org.appspot.apprtc.VIDEOCODEC VP8 --ez org.appspot.apprtc.CAPTURETOTEXTURE false --ez org.appspot.apprtc.CAMERA2 false --es org.appspot.apprtc.VIDEO_FILE_AS_CAMERA \"/storage/emulated/0/nv21_1280x720.yuv\" --ei org.appspot.apprtc.VIDEO_FILE_AS_CAMERA_WIDTH 1280 --ei org.appspot.apprtc.VIDEO_FILE_AS_CAMERA_HEIGHT 720 --es org.appspot.apprtc.SAVE_REMOTE_VIDEO_TO_FILE \"/storage/emulated/0/output.y4m\" --ei org.appspot.apprtc.SAVE_REMOTE_VIDEO_TO_FILE_WIDTH 1280 --ei org.appspot.apprtc.SAVE_REMOTE_VIDEO_TO_FILE_HEIGHT 720 --es org.appspot.apprtc.ROOMID $room" > cmd.sh && adb -s 02157df28cd47001 push cmd.sh /data/local/tmp/. && adb -s 02157df28cd47001 shell sh /data/local/tmp/cmd.sh ==========
https://codereview.webrtc.org/2273573003/diff/20001/webrtc/api/android/java/s... File webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java (right): https://codereview.webrtc.org/2273573003/diff/20001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:27: class VideoReaderYuv implements VideoReader { Make this a private class in FileVideoCapturer instead. https://codereview.webrtc.org/2273573003/diff/20001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:36: } You should have an empty line between functions. https://codereview.webrtc.org/2273573003/diff/20001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:45: this.frameSize = width*height*3/2; You need to insert spaces here. https://codereview.webrtc.org/2273573003/diff/20001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:72: class VideoReaderY4M implements VideoReader { Is this class unused? https://codereview.webrtc.org/2273573003/diff/20001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:147: public class FileVideoCapturer implements CameraVideoCapturer { Implement VideoCapturer instead of CameraVideoCapturer. https://codereview.webrtc.org/2273573003/diff/20001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:152: private CameraVideoCapturer physicalCameraCapturer; This is strange, why does the File capturer need a reference to a physicalCameraCapturer? https://codereview.webrtc.org/2273573003/diff/20001/webrtc/api/android/java/s... File webrtc/api/android/java/src/org/webrtc/GlVideoFileDrawer.java (right): https://codereview.webrtc.org/2273573003/diff/20001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/GlVideoFileDrawer.java:16: public class GlVideoFileDrawer implements RendererCommon.GlDrawer { I don't think this is the best approach, I think you should implement VideoRenderer.Callbacks instead. If you want to preserve remote rendering when capturing to a file, you can add multiple renderers to the remote video track. https://codereview.webrtc.org/2273573003/diff/20001/webrtc/examples/androidap... File webrtc/examples/androidapp/AndroidManifest.xml (right): https://codereview.webrtc.org/2273573003/diff/20001/webrtc/examples/androidap... webrtc/examples/androidapp/AndroidManifest.xml:6: android:debuggable="true"> What will this do? https://codereview.webrtc.org/2273573003/diff/20001/webrtc/examples/androidap... webrtc/examples/androidapp/AndroidManifest.xml:51: <intent-filter> Is this needed for starting CallActivity from adb shell? https://codereview.webrtc.org/2273573003/diff/20001/webrtc/examples/androidap... File webrtc/examples/androidapp/src/org/appspot/apprtc/CallActivity.java (right): https://codereview.webrtc.org/2273573003/diff/20001/webrtc/examples/androidap... webrtc/examples/androidapp/src/org/appspot/apprtc/CallActivity.java:40: import org.webrtc.GlVideoFileDrawer; The imports should be put in alphabetical order. https://codereview.webrtc.org/2273573003/diff/20001/webrtc/examples/androidap... webrtc/examples/androidapp/src/org/appspot/apprtc/CallActivity.java:103: public static final String EXTRA_VIDEO_FILE_AS_CAMERA_WIDTH = Why do we have to send width and height separately from the File?
https://codereview.webrtc.org/2273573003/diff/20001/webrtc/api/android/java/s... File webrtc/api/android/java/src/org/webrtc/GlVideoFileDrawer.java (right): https://codereview.webrtc.org/2273573003/diff/20001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/GlVideoFileDrawer.java:34: surfaceTextureHelper = SurfaceTextureHelper.create( The SurfaceTextureHelper will create a SurfaceTexture and some other GL stuff that you don't need. Create a YuvConverter directly instead. https://codereview.webrtc.org/2273573003/diff/20001/webrtc/api/android/java/s... File webrtc/api/android/java/src/org/webrtc/SurfaceTextureHelper.java (right): https://codereview.webrtc.org/2273573003/diff/20001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/SurfaceTextureHelper.java:79: static public class YuvConverter { Move this class to a new file instead.
Have fixed review comments https://codereview.webrtc.org/2273573003/diff/20001/webrtc/api/android/java/s... File webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java (right): https://codereview.webrtc.org/2273573003/diff/20001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:27: class VideoReaderYuv implements VideoReader { On 2016/08/31 13:08:03, magjed_webrtc wrote: > Make this a private class in FileVideoCapturer instead. Done. https://codereview.webrtc.org/2273573003/diff/20001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:36: } On 2016/08/31 13:08:03, magjed_webrtc wrote: > You should have an empty line between functions. Done. Does it apply to interfaces? https://codereview.webrtc.org/2273573003/diff/20001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:45: this.frameSize = width*height*3/2; On 2016/08/31 13:08:03, magjed_webrtc wrote: > You need to insert spaces here. Done. https://codereview.webrtc.org/2273573003/diff/20001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:72: class VideoReaderY4M implements VideoReader { On 2016/08/31 13:08:03, magjed_webrtc wrote: > Is this class unused? Used earlier, removed now. https://codereview.webrtc.org/2273573003/diff/20001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:147: public class FileVideoCapturer implements CameraVideoCapturer { On 2016/08/31 13:08:03, magjed_webrtc wrote: > Implement VideoCapturer instead of CameraVideoCapturer. PeerConnectionClient uses switchCamera(), which is defined in CameraVideoCapturer. https://codereview.webrtc.org/2273573003/diff/20001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:152: private CameraVideoCapturer physicalCameraCapturer; On 2016/08/31 13:08:03, magjed_webrtc wrote: > This is strange, why does the File capturer need a reference to a > physicalCameraCapturer? Refactored with a timer thread controlling when to send frames. A physical camera was to get similar load to normal program and use same timing as physical camera. The frames from the physical camera were discarded. https://codereview.webrtc.org/2273573003/diff/20001/webrtc/api/android/java/s... File webrtc/api/android/java/src/org/webrtc/GlVideoFileDrawer.java (right): https://codereview.webrtc.org/2273573003/diff/20001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/GlVideoFileDrawer.java:34: surfaceTextureHelper = SurfaceTextureHelper.create( On 2016/08/31 14:10:46, magjed_webrtc wrote: > The SurfaceTextureHelper will create a SurfaceTexture and some other GL stuff > that you don't need. Create a YuvConverter directly instead. Done. https://codereview.webrtc.org/2273573003/diff/20001/webrtc/examples/androidap... File webrtc/examples/androidapp/AndroidManifest.xml (right): https://codereview.webrtc.org/2273573003/diff/20001/webrtc/examples/androidap... webrtc/examples/androidapp/AndroidManifest.xml:6: android:debuggable="true"> On 2016/08/31 13:08:03, magjed_webrtc wrote: > What will this do? Removed, seams only the <intent-filter> was needed. https://codereview.webrtc.org/2273573003/diff/20001/webrtc/examples/androidap... File webrtc/examples/androidapp/src/org/appspot/apprtc/CallActivity.java (right): https://codereview.webrtc.org/2273573003/diff/20001/webrtc/examples/androidap... webrtc/examples/androidapp/src/org/appspot/apprtc/CallActivity.java:40: import org.webrtc.GlVideoFileDrawer; On 2016/08/31 13:08:03, magjed_webrtc wrote: > The imports should be put in alphabetical order. Done. https://codereview.webrtc.org/2273573003/diff/20001/webrtc/examples/androidap... webrtc/examples/androidapp/src/org/appspot/apprtc/CallActivity.java:103: public static final String EXTRA_VIDEO_FILE_AS_CAMERA_WIDTH = On 2016/08/31 13:08:03, magjed_webrtc wrote: > Why do we have to send width and height separately from the File? Video container .yuv does not contain width and height
https://codereview.webrtc.org/2273573003/diff/20001/webrtc/api/android/java/s... File webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java (right): https://codereview.webrtc.org/2273573003/diff/20001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:36: } On 2016/09/16 12:32:29, mandermo wrote: > On 2016/08/31 13:08:03, magjed_webrtc wrote: > > You should have an empty line between functions. > > Done. > > Does it apply to interfaces? No. https://codereview.webrtc.org/2273573003/diff/20001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:147: public class FileVideoCapturer implements CameraVideoCapturer { On 2016/09/16 12:32:30, mandermo wrote: > On 2016/08/31 13:08:03, magjed_webrtc wrote: > > Implement VideoCapturer instead of CameraVideoCapturer. > > PeerConnectionClient uses switchCamera(), which is defined in > CameraVideoCapturer. We talked about it offline, but I think the right approach is to implement VideoCapturer instead, and generalize PeerConnectionClient to work with VideoCapturer. I would recommend just using 'isinstanceof CameraVideoCapturer' in all places that needs a CameraVideoCapturer in PeerConnectionClient. https://codereview.webrtc.org/2273573003/diff/20001/webrtc/examples/androidap... File webrtc/examples/androidapp/src/org/appspot/apprtc/CallActivity.java (right): https://codereview.webrtc.org/2273573003/diff/20001/webrtc/examples/androidap... webrtc/examples/androidapp/src/org/appspot/apprtc/CallActivity.java:103: public static final String EXTRA_VIDEO_FILE_AS_CAMERA_WIDTH = On 2016/09/16 12:32:30, mandermo wrote: > On 2016/08/31 13:08:03, magjed_webrtc wrote: > > Why do we have to send width and height separately from the File? > > Video container .yuv does not contain width and height Ok, then I prefer if we use a container that contains width and height, even if you have to convert from I420 to NV12 at runtime. https://codereview.webrtc.org/2273573003/diff/80001/webrtc/api/android/java/s... File webrtc/api/android/java/src/org/webrtc/FileVideoCapturerTimer.java (right): https://codereview.webrtc.org/2273573003/diff/80001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/FileVideoCapturerTimer.java:18: import java.io.File; Some of these imports are unused. Please remove all unused imports. https://codereview.webrtc.org/2273573003/diff/80001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/FileVideoCapturerTimer.java:23: public class FileVideoCapturerTimer implements CameraVideoCapturer { Why do we need both FileVideoCapturer and FileVideoCapturerTimer? To sync frame delivery with a physical camera? I'm not sure we will ever need that, and then it would be easier to just merge these classes. https://codereview.webrtc.org/2273573003/diff/80001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/FileVideoCapturerTimer.java:28: private volatile boolean isCapturing = false; This variable is unused. Please remove all unused variables. https://codereview.webrtc.org/2273573003/diff/80001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/FileVideoCapturerTimer.java:50: timer.schedule(tickTask, 0, 33); This should be based on the framerate. https://codereview.webrtc.org/2273573003/diff/80001/webrtc/api/android/java/s... File webrtc/api/android/java/src/org/webrtc/GlVideoFileRenderer.java (right): https://codereview.webrtc.org/2273573003/diff/80001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/GlVideoFileRenderer.java:50: Logging.d(TAG, "Failed to open file to print video out"); I guess we don't want to continue here. https://codereview.webrtc.org/2273573003/diff/80001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/GlVideoFileRenderer.java:69: int oesTextureId = frame.textureId; We need to be able to handle ByteBuffer frames as well. https://codereview.webrtc.org/2273573003/diff/80001/webrtc/examples/androidap... File webrtc/examples/androidapp/src/org/appspot/apprtc/CallActivity.java (right): https://codereview.webrtc.org/2273573003/diff/80001/webrtc/examples/androidap... webrtc/examples/androidapp/src/org/appspot/apprtc/CallActivity.java:38: import org.webrtc.GlRectDrawer; This import is unused.
Some refactoring of the error handling.
I focused on the .py and .gn, I'm leaving the .java to magjed. https://codereview.webrtc.org/2273573003/diff/100001/webrtc/examples/androida... File webrtc/examples/androidapp/start_loopback_stubbed_camera_saved_video_out.py (right): https://codereview.webrtc.org/2273573003/diff/100001/webrtc/examples/androida... webrtc/examples/androidapp/start_loopback_stubbed_camera_saved_video_out.py:9: import string Sort imports alphabetically. Tip: you can run gpylint / pylint on your script to verify it's passing the lint checks. https://codereview.webrtc.org/2273573003/diff/100001/webrtc/examples/androida... webrtc/examples/androidapp/start_loopback_stubbed_camera_saved_video_out.py:15: devname = sys.argv[2] Since this is python and managing flags is so easy with the argparse module, I suggest you implement these as flags instead of using sys.argv. Then you can also print a nice usage message if the wrong arguments or -h/--help is provided. https://codereview.webrtc.org/2273573003/diff/100001/webrtc/examples/androida... webrtc/examples/androidapp/start_loopback_stubbed_camera_saved_video_out.py:15: devname = sys.argv[2] Put everything in a main method and invoke it at the end of the script: http://dev.chromium.org/chromium-os/python-style-guidelines#TOC-Main-Modules https://codereview.webrtc.org/2273573003/diff/100001/webrtc/examples/androida... webrtc/examples/androidapp/start_loopback_stubbed_camera_saved_video_out.py:20: for _ in range(8)) -1 space indent. https://codereview.webrtc.org/2273573003/diff/100001/webrtc/examples/androida... webrtc/examples/androidapp/start_loopback_stubbed_camera_saved_video_out.py:24: '/storage/emulated/0/output.y4m']) indent with previous line's parenthesis https://google.github.io/styleguide/pyguide.html#Indentation Notice that Chromium and WebRTC only has 2 space indent though: http://dev.chromium.org/chromium-os/python-style-guidelines https://codereview.webrtc.org/2273573003/diff/100001/webrtc/examples/androida... webrtc/examples/androidapp/start_loopback_stubbed_camera_saved_video_out.py:25: #sys.exit() Please remove this debugging line. https://codereview.webrtc.org/2273573003/diff/100001/webrtc/examples/androida... webrtc/examples/androidapp/start_loopback_stubbed_camera_saved_video_out.py:30: 'org.appspot.apprtc.AUDIOCODEC': 'OPUS', Indent these lines with 4 spaces, not more. https://codereview.webrtc.org/2273573003/diff/100001/webrtc/examples/androida... webrtc/examples/androidapp/start_loopback_stubbed_camera_saved_video_out.py:36: '/storage/emulated/0/nv21_1280x720.yuv', Create a flag for this and use this value as the default value (if no value is mitted). https://codereview.webrtc.org/2273573003/diff/100001/webrtc/examples/androida... webrtc/examples/androidapp/start_loopback_stubbed_camera_saved_video_out.py:37: 'org.appspot.apprtc.VIDEO_FILE_AS_CAMERA_WIDTH': 1280, I'd prefer to extract width and height as parameters to the script as well (with 1280x720 as default values) https://codereview.webrtc.org/2273573003/diff/100001/webrtc/examples/androida... webrtc/examples/androidapp/start_loopback_stubbed_camera_saved_video_out.py:40: '/storage/emulated/0/output.y4m', Another flag. https://codereview.webrtc.org/2273573003/diff/100001/webrtc/examples/androida... webrtc/examples/androidapp/start_loopback_stubbed_camera_saved_video_out.py:47: device.startActivity(data='https://appr.tc', action='ACTION_VIEW', We want to have a flag for this address as well, since we plan to run it locally at localhost:9999, right? https://codereview.webrtc.org/2273573003/diff/100001/webrtc/examples/androida... webrtc/examples/androidapp/start_loopback_stubbed_camera_saved_video_out.py:48: component='org.appspot.apprtc/.CallActivity', extras=extras) indent either with 4 spaces or align with the above line's parenthesis. https://codereview.webrtc.org/2273573003/diff/100001/webrtc/examples/androida... webrtc/examples/androidapp/start_loopback_stubbed_camera_saved_video_out.py:50: time.sleep(10) Please add a flag for the call length, then create a loop where you print each second, so the user running the script knows what's going on. Something like this: print 'Running a call for %d seconds', % opts.call_length for _ in xrange(opts.call_length): sys.stdout.write('.') time.sleep(1) print '\nEnding call.' https://codereview.webrtc.org/2273573003/diff/100001/webrtc/examples/androida... webrtc/examples/androidapp/start_loopback_stubbed_camera_saved_video_out.py:52: # Press back to end the call. Will end on both sides nit: End comments with punctation. https://codereview.webrtc.org/2273573003/diff/100001/webrtc/examples/androida... webrtc/examples/androidapp/start_loopback_stubbed_camera_saved_video_out.py:55: subprocess.Popen(['adb', '-s', devname, 'pull', Do we know that the call has ended, teardown has completed and such, or do we risk starting to pull the video before it has been finished writing it to the sdcard? https://codereview.webrtc.org/2273573003/diff/100001/webrtc/examples/androida... webrtc/examples/androidapp/start_loopback_stubbed_camera_saved_video_out.py:56: '/storage/emulated/0/output.y4m', videoout]) Use the same flag as earlier here, for the putput file.
Changes based on review comments https://codereview.webrtc.org/2273573003/diff/20001/webrtc/api/android/java/s... File webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java (right): https://codereview.webrtc.org/2273573003/diff/20001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:147: public class FileVideoCapturer implements CameraVideoCapturer { On 2016/08/31 13:08:03, magjed_webrtc wrote: > Implement VideoCapturer instead of CameraVideoCapturer. Done. https://codereview.webrtc.org/2273573003/diff/20001/webrtc/examples/androidap... File webrtc/examples/androidapp/src/org/appspot/apprtc/CallActivity.java (right): https://codereview.webrtc.org/2273573003/diff/20001/webrtc/examples/androidap... webrtc/examples/androidapp/src/org/appspot/apprtc/CallActivity.java:103: public static final String EXTRA_VIDEO_FILE_AS_CAMERA_WIDTH = On 2016/09/16 13:46:25, magjed_webrtc wrote: > On 2016/09/16 12:32:30, mandermo wrote: > > On 2016/08/31 13:08:03, magjed_webrtc wrote: > > > Why do we have to send width and height separately from the File? > > > > Video container .yuv does not contain width and height > > Ok, then I prefer if we use a container that contains width and height, even if > you have to convert from I420 to NV12 at runtime. Change to .y4m with width and height and conversion from I420 to NV21 at runtime. https://codereview.webrtc.org/2273573003/diff/80001/webrtc/api/android/java/s... File webrtc/api/android/java/src/org/webrtc/FileVideoCapturerTimer.java (right): https://codereview.webrtc.org/2273573003/diff/80001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/FileVideoCapturerTimer.java:23: public class FileVideoCapturerTimer implements CameraVideoCapturer { On 2016/09/16 13:46:25, magjed_webrtc wrote: > Why do we need both FileVideoCapturer and FileVideoCapturerTimer? To sync frame > delivery with a physical camera? I'm not sure we will ever need that, and then > it would be easier to just merge these classes. To be able to late do timing based on physical camera. Now merge because it is unlikely we would need timing based on physical camera. https://codereview.webrtc.org/2273573003/diff/80001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/FileVideoCapturerTimer.java:28: private volatile boolean isCapturing = false; On 2016/09/16 13:46:25, magjed_webrtc wrote: > This variable is unused. Please remove all unused variables. Done. https://codereview.webrtc.org/2273573003/diff/80001/webrtc/api/android/java/s... webrtc/api/android/java/src/org/webrtc/FileVideoCapturerTimer.java:50: timer.schedule(tickTask, 0, 33); On 2016/09/16 13:46:25, magjed_webrtc wrote: > This should be based on the framerate. Based on framerate argument now https://codereview.webrtc.org/2273573003/diff/80001/webrtc/examples/androidap... File webrtc/examples/androidapp/src/org/appspot/apprtc/CallActivity.java (right): https://codereview.webrtc.org/2273573003/diff/80001/webrtc/examples/androidap... webrtc/examples/androidapp/src/org/appspot/apprtc/CallActivity.java:38: import org.webrtc.GlRectDrawer; On 2016/09/16 13:46:25, magjed_webrtc wrote: > This import is unused. Done. https://codereview.webrtc.org/2273573003/diff/100001/webrtc/examples/androida... File webrtc/examples/androidapp/start_loopback_stubbed_camera_saved_video_out.py (right): https://codereview.webrtc.org/2273573003/diff/100001/webrtc/examples/androida... webrtc/examples/androidapp/start_loopback_stubbed_camera_saved_video_out.py:15: devname = sys.argv[2] On 2016/09/19 05:27:29, kjellander_webrtc wrote: > Put everything in a main method and invoke it at the end of the script: > http://dev.chromium.org/chromium-os/python-style-guidelines#TOC-Main-Modules Done. https://codereview.webrtc.org/2273573003/diff/100001/webrtc/examples/androida... webrtc/examples/androidapp/start_loopback_stubbed_camera_saved_video_out.py:20: for _ in range(8)) On 2016/09/19 05:27:28, kjellander_webrtc wrote: > -1 space indent. Done. https://codereview.webrtc.org/2273573003/diff/100001/webrtc/examples/androida... webrtc/examples/androidapp/start_loopback_stubbed_camera_saved_video_out.py:24: '/storage/emulated/0/output.y4m']) On 2016/09/19 05:27:29, kjellander_webrtc wrote: > indent with previous line's parenthesis > https://google.github.io/styleguide/pyguide.html#Indentation > > Notice that Chromium and WebRTC only has 2 space indent though: > http://dev.chromium.org/chromium-os/python-style-guidelines Done. https://codereview.webrtc.org/2273573003/diff/100001/webrtc/examples/androida... webrtc/examples/androidapp/start_loopback_stubbed_camera_saved_video_out.py:25: #sys.exit() On 2016/09/19 05:27:29, kjellander_webrtc wrote: > Please remove this debugging line. Done. https://codereview.webrtc.org/2273573003/diff/100001/webrtc/examples/androida... webrtc/examples/androidapp/start_loopback_stubbed_camera_saved_video_out.py:30: 'org.appspot.apprtc.AUDIOCODEC': 'OPUS', On 2016/09/19 05:27:28, kjellander_webrtc wrote: > Indent these lines with 4 spaces, not more. Done. https://codereview.webrtc.org/2273573003/diff/100001/webrtc/examples/androida... webrtc/examples/androidapp/start_loopback_stubbed_camera_saved_video_out.py:36: '/storage/emulated/0/nv21_1280x720.yuv', On 2016/09/19 05:27:29, kjellander_webrtc wrote: > Create a flag for this and use this value as the default value (if no value is > mitted). Done. https://codereview.webrtc.org/2273573003/diff/100001/webrtc/examples/androida... webrtc/examples/androidapp/start_loopback_stubbed_camera_saved_video_out.py:37: 'org.appspot.apprtc.VIDEO_FILE_AS_CAMERA_WIDTH': 1280, On 2016/09/19 05:27:29, kjellander_webrtc wrote: > I'd prefer to extract width and height as parameters to the script as well (with > 1280x720 as default values) Done. https://codereview.webrtc.org/2273573003/diff/100001/webrtc/examples/androida... webrtc/examples/androidapp/start_loopback_stubbed_camera_saved_video_out.py:40: '/storage/emulated/0/output.y4m', On 2016/09/19 05:27:28, kjellander_webrtc wrote: > Another flag. Done. https://codereview.webrtc.org/2273573003/diff/100001/webrtc/examples/androida... webrtc/examples/androidapp/start_loopback_stubbed_camera_saved_video_out.py:47: device.startActivity(data='https://appr.tc', action='ACTION_VIEW', On 2016/09/19 05:27:29, kjellander_webrtc wrote: > We want to have a flag for this address as well, since we plan to run it locally > at localhost:9999, right? Will do that in separate CL when we run locally https://codereview.webrtc.org/2273573003/diff/100001/webrtc/examples/androida... webrtc/examples/androidapp/start_loopback_stubbed_camera_saved_video_out.py:48: component='org.appspot.apprtc/.CallActivity', extras=extras) On 2016/09/19 05:27:29, kjellander_webrtc wrote: > indent either with 4 spaces or align with the above line's parenthesis. Done. https://codereview.webrtc.org/2273573003/diff/100001/webrtc/examples/androida... webrtc/examples/androidapp/start_loopback_stubbed_camera_saved_video_out.py:50: time.sleep(10) On 2016/09/19 05:27:28, kjellander_webrtc wrote: > Please add a flag for the call length, then create a loop where you print each > second, so the user running the script knows what's going on. Something like > this: > > print 'Running a call for %d seconds', % opts.call_length > for _ in xrange(opts.call_length): > sys.stdout.write('.') > time.sleep(1) > print '\nEnding call.' Done. https://codereview.webrtc.org/2273573003/diff/100001/webrtc/examples/androida... webrtc/examples/androidapp/start_loopback_stubbed_camera_saved_video_out.py:55: subprocess.Popen(['adb', '-s', devname, 'pull', On 2016/09/19 05:27:29, kjellander_webrtc wrote: > Do we know that the call has ended, teardown has completed and such, or do we > risk starting to pull the video before it has been finished writing it to the > sdcard? Not had any problems yet, but added small sleep https://codereview.webrtc.org/2273573003/diff/100001/webrtc/examples/androida... webrtc/examples/androidapp/start_loopback_stubbed_camera_saved_video_out.py:56: '/storage/emulated/0/output.y4m', videoout]) On 2016/09/19 05:27:29, kjellander_webrtc wrote: > Use the same flag as earlier here, for the putput file. Done.
magjed@webrtc.org changed reviewers: + sakal@webrtc.org
I don't have much time to review, so I'm adding Sami as a reviewer. I can take a final look once you have his approval. There are a lot of style nits you need to fix and removal of redundant and unused code. My only remaining high level issues are: - I think 'VideoCapturer videoCapturer' should be added as a parameter to PeerConnectionParameters instead of useCamera2, captureToTexture, and videoFileAsCamera. - Update the CL description and make it more descriptive of what you do in this CL. You also need to file a bug that tracks this work. Remove the big command line comment from the CL description since you are adding that in a file instead. You might also consider landing FileVideoCapturer.java and GlVideoFileRenderer.java in separate CL:s before landing this change. - I would like to see some unittests for FileVideoCapturer and GlVideoFileRenderer. Is it possible to mock IO with robolectric? https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... File webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java (right): https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:31: class VideoReaderY4M implements VideoReader { Make this class static. https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:33: private int frameWidth; make these variables final https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:40: final String Y4M_FRAME_DELIMETER = "FRAME"; make this private static https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:54: byte[] startData = new byte[1000]; This is a bit ugly. Is there no elegant way of finding the first occurrence of a string in a RandomAccessFile? https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:60: //throw new RuntimeException("Error reading file"); You should probably throw here. https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:82: frameSize = w * h * 3 / 2; have you tried frames with odd width? I think you need to update this code to 'w * h + 2 * ((w + 1) / 2) * ((h + 1) / 2)' https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:89: if (mediaFileStream.skipBytes(Y4M_FRAME_DELIMETER.length()+1) < this doesn't follow the style guide https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:111: Logging.d(TAG, "Problem closing file"); Log error here instead, i.e. Logging.e. https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:157: videoReader = new VideoReaderY4M(file); just inline this in the ctor https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:160: Logging.d(TAG, "Could not open video file: " + file); We already log in FileVideoCapturer.create so you don't need this one as well. https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... File webrtc/api/android/java/src/org/webrtc/GlVideoFileRenderer.java (right): https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/GlVideoFileRenderer.java:120: int outputFrameSize = outputFileWidth * outputFileHeight * 3 / 2; You already have this variable in |frameSize|. Also, you should handle odd widths properly. https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/GlVideoFileRenderer.java:128: videoOutFile.write(outputBuffer.array(), outputBuffer.arrayOffset(), outputFrameSize); You are not calling VideoRenderer.renderFrameDone in this path, so you will be leaking all frames. Have you tried exercising this path in a local test? https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... File webrtc/api/android/java/src/org/webrtc/SurfaceTextureHelper.java (right): https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/SurfaceTextureHelper.java:349: public YuvConverter getYuvConverter() { Make this private again. https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/jni/p... File webrtc/api/android/jni/peerconnection_jni.cc (right): https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/jni/p... webrtc/api/android/jni/peerconnection_jni.cc:2010: //size_t src_size = jni->GetDirectBufferCapacity(j_src_buffer); Clean up and remove unused code. https://codereview.webrtc.org/2273573003/diff/140001/webrtc/examples/androida... File webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java (right): https://codereview.webrtc.org/2273573003/diff/140001/webrtc/examples/androida... webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java:169: public final String videoFileAsCamera; This doesn't really scale. We now have useCamera2, captureToTexture, and videoFileAsCamera as parameters. Soon useScreenCapturer will be added as well. I think it would be better to have a single 'VideoCapturer videoCapturer' as parameter instead. https://codereview.webrtc.org/2273573003/diff/140001/webrtc/examples/androida... webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java:1061: if (videoCapturer instanceof CameraVideoCapturer) { Maybe you should log when this is not the case. Maybe add this check together with the other checks in the first if-statement.
https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... File webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java (right): https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:9: */ nit: add empty line https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:18: nit: remove empty line https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:23: public class FileVideoCapturer implements VideoCapturer { nit: it would be nice to have a simple javadoc about what this class does https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:31: class VideoReaderY4M implements VideoReader { On 2016/09/26 11:40:01, magjed_webrtc wrote: > Make this class static. And private? https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:42: private RandomAccessFile mediaFileStream; Make this final. https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:52: VideoReaderY4M(String file) throws IOException { public? https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:82: frameSize = w * h * 3 / 2; The length of the frame depends on the color space. Maybe we should assert that the file has the correct color space? https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:95: mediaFileStream.readFully(frame); Frames can also have parameters. We should take that into account. https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:97: nativeI420ToNV21( Please add comment why it is called nv12Frame when we use nativeI420ToNV21. I know in some places we use some trickery to convert between the two. Is it the case here as well? https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:101: catch (IOException e) { nit: move to the same line with the closing bracket https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... File webrtc/api/android/java/src/org/webrtc/GlVideoFileRenderer.java (right): https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/GlVideoFileRenderer.java:25: private SurfaceTextureHelper.YuvConverter yuvConverter; nit: Please remove excessive empty lines. https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/GlVideoFileRenderer.java:39: sharedContext); nit: Does this fit on the same line? https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/GlVideoFileRenderer.java:70: RendererCommon.rotateTextureMatrix(frame.samplingMatrix, frame.rotationDegree); nit: indentation https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/GlVideoFileRenderer.java:75: int frameSize = outputFileWidth*outputFileHeight*3/2; nit: spaces How does this work with uneven resolutions? https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/GlVideoFileRenderer.java:76: ByteBuffer frameBytes = ByteBuffer.allocateDirect(frameSize); Can we allocate this in the constructor? Java documentation: The buffers returned by this method typically have somewhat higher allocation and deallocation costs than non-direct buffers. The contents of direct buffers may reside outside of the normal garbage-collected heap, and so their impact upon the memory footprint of an application might not be obvious. It is therefore recommended that direct buffers be allocated primarily for large, long-lived buffers that are subject to the underlying system's native I/O operations. In general it is best to allocate direct buffers only when they yield a measureable gain in program performance. https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/GlVideoFileRenderer.java:100: videoOutFile.write(data, offset, width*height); nit: spaces https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/GlVideoFileRenderer.java:103: for (int r = height; r<height*3/2; ++r) { nit: spaces https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/GlVideoFileRenderer.java:108: for (int r = height; r<height*3/2; ++r) { nit: spaces https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/GlVideoFileRenderer.java:116: else { nit: move on the same line with the closing bracket https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... File webrtc/api/android/java/src/org/webrtc/SurfaceTextureHelper.java (right): https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/SurfaceTextureHelper.java:79: static public class YuvConverter { Maybe package visibility would be appropriate? https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/jni/p... File webrtc/api/android/jni/peerconnection_jni.cc (right): https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/jni/p... webrtc/api/android/jni/peerconnection_jni.cc:2050: jobject j_src_buffer_y, jint j_src_stride_y, These are also jbyteArrays, right? Can you use the type? https://codereview.webrtc.org/2273573003/diff/140001/webrtc/examples/androida... File webrtc/examples/androidapp/AndroidManifest.xml (right): https://codereview.webrtc.org/2273573003/diff/140001/webrtc/examples/androida... webrtc/examples/androidapp/AndroidManifest.xml:50: <intent-filter> There is already an intent in the ConnectActivity. Can we keep using that one or move all code to this intent? https://codereview.webrtc.org/2273573003/diff/140001/webrtc/examples/androida... File webrtc/examples/androidapp/start_loopback_stubbed_camera_saved_video_out.py (right): https://codereview.webrtc.org/2273573003/diff/140001/webrtc/examples/androida... webrtc/examples/androidapp/start_loopback_stubbed_camera_saved_video_out.py:1: # Copyright 2016 The WebRTC Project Authors. All rights reserved. nit: Can you add the shebang line? https://codereview.webrtc.org/2273573003/diff/140001/webrtc/examples/androida... webrtc/examples/androidapp/start_loopback_stubbed_camera_saved_video_out.py:7: from com.android.monkeyrunner import MonkeyRunner, MonkeyDevice Move group below standard imports. Google Python style guide: Imports are always put at the top of the file, just after any module comments and doc strings and before module globals and constants. Imports should be grouped with the order being most generic to least generic: standard library imports third-party imports application-specific imports https://codereview.webrtc.org/2273573003/diff/140001/webrtc/examples/androida... webrtc/examples/androidapp/start_loopback_stubbed_camera_saved_video_out.py:81: 'org.appspot.apprtc.AUDIOCODEC': 'OPUS', nit: Indent with 4 spaces
Whoa, already a centi-comment CL. I think almost all outstanding comments are pretty easy to fix luckily. I added a comment about the refactoring request, which I think can wait to a future CL. Regarding testing I'd like to see something exercising the code paths in GlVideoFileRenderer.java at least. More extensive unit testing can probably wait to a follow-up CL since this is already getting out of hand. https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... File webrtc/api/android/java/src/org/webrtc/GlVideoFileRenderer.java (right): https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/GlVideoFileRenderer.java:128: videoOutFile.write(outputBuffer.array(), outputBuffer.arrayOffset(), outputFrameSize); On 2016/09/26 11:40:02, magjed_webrtc wrote: > You are not calling VideoRenderer.renderFrameDone in this path, so you will be > leaking all frames. Have you tried exercising this path in a local test? That sounds bad. Can we please get a basic unit test for each of these two code paths? https://codereview.webrtc.org/2273573003/diff/140001/webrtc/examples/androida... File webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java (right): https://codereview.webrtc.org/2273573003/diff/140001/webrtc/examples/androida... webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java:169: public final String videoFileAsCamera; On 2016/09/26 11:40:02, magjed_webrtc wrote: > This doesn't really scale. We now have useCamera2, captureToTexture, and > videoFileAsCamera as parameters. Soon useScreenCapturer will be added as well. I > think it would be better to have a single 'VideoCapturer videoCapturer' as > parameter instead. I think this can wait. This CL is adding new functionality and shouldn't attempt to do refactorings in the same CL. Let's address the refactoring in a separate CL.
Description was changed from ========== Support for video file instead of camera and output video out to file When video out to file is enabled the remote video which is recorded is not show on screen. You can use this command line for file input and output: room=$(uuidgen | cut -d- -f1); echo "am start -a android.intent.action.VIEW -n org.appspot.apprtc/.CallActivity -d https://appr.tc --es org.appspot.apprtc.AUDIOCODEC OPUS --ez org.appspot.apprtc.LOOPBACK true --es org.appspot.apprtc.VIDEOCODEC VP8 --ez org.appspot.apprtc.CAPTURETOTEXTURE false --ez org.appspot.apprtc.CAMERA2 false --es org.appspot.apprtc.VIDEO_FILE_AS_CAMERA \"/storage/emulated/0/nv21_1280x720.yuv\" --ei org.appspot.apprtc.VIDEO_FILE_AS_CAMERA_WIDTH 1280 --ei org.appspot.apprtc.VIDEO_FILE_AS_CAMERA_HEIGHT 720 --es org.appspot.apprtc.SAVE_REMOTE_VIDEO_TO_FILE \"/storage/emulated/0/output.y4m\" --ei org.appspot.apprtc.SAVE_REMOTE_VIDEO_TO_FILE_WIDTH 1280 --ei org.appspot.apprtc.SAVE_REMOTE_VIDEO_TO_FILE_HEIGHT 720 --es org.appspot.apprtc.ROOMID $room" > cmd.sh && adb -s 02157df28cd47001 push cmd.sh /data/local/tmp/. && adb -s 02157df28cd47001 shell sh /data/local/tmp/cmd.sh ========== to ========== Support for video file instead of camera and output video out to file When video out to file is enabled the remote video which is recorded is not show on screen. You can use this command line for file input and output: monkeyrunner ./webrtc/examples/androidapp/start_loopback_stubbed_camera_saved_video_out.py --devname 02157df28cd47001 --videoin /storage/emulated/0/reference_video_1280x720_30fps.y4m --videoout /storage/emulated/0/output.y4m --videoout_width 1280 --videoout_height 720 --videooutsave /tmp/out.y4m ==========
Updated from review comments, except unit testing https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... File webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java (right): https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:31: class VideoReaderY4M implements VideoReader { On 2016/09/26 11:40:01, magjed_webrtc wrote: > Make this class static. Made it private static https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:31: class VideoReaderY4M implements VideoReader { On 2016/09/27 07:54:27, sakal wrote: > On 2016/09/26 11:40:01, magjed_webrtc wrote: > > Make this class static. > > And private? Done. https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:33: private int frameWidth; On 2016/09/26 11:40:02, magjed_webrtc wrote: > make these variables final Done. https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:40: final String Y4M_FRAME_DELIMETER = "FRAME"; On 2016/09/26 11:40:02, magjed_webrtc wrote: > make this private static Done. https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:52: VideoReaderY4M(String file) throws IOException { On 2016/09/27 07:54:27, sakal wrote: > public? Done. https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:54: byte[] startData = new byte[1000]; On 2016/09/26 11:40:02, magjed_webrtc wrote: > This is a bit ugly. Is there no elegant way of finding the first occurrence of a > string in a RandomAccessFile? Done. https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:54: byte[] startData = new byte[1000]; On 2016/09/26 11:40:02, magjed_webrtc wrote: > This is a bit ugly. Is there no elegant way of finding the first occurrence of a > string in a RandomAccessFile? Done. https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:60: //throw new RuntimeException("Error reading file"); On 2016/09/26 11:40:01, magjed_webrtc wrote: > You should probably throw here. Done. https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:60: //throw new RuntimeException("Error reading file"); On 2016/09/26 11:40:01, magjed_webrtc wrote: > You should probably throw here. Done. https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:82: frameSize = w * h * 3 / 2; On 2016/09/26 11:40:01, magjed_webrtc wrote: > have you tried frames with odd width? I think you need to update this code to 'w > * h + 2 * ((w + 1) / 2) * ((h + 1) / 2)' https://wiki.multimedia.cx/index.php?title=YUV4MPEG2 says that you can compute frame size this way width * height * 3 / 2 for I420. It does not appear to me that the container support odd sized frames. Should I do it anyway? https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:95: mediaFileStream.readFully(frame); On 2016/09/27 07:54:27, sakal wrote: > Frames can also have parameters. We should take that into account. Checks that the frame does not contain any parameters now. https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:97: nativeI420ToNV21( On 2016/09/27 07:54:27, sakal wrote: > Please add comment why it is called nv12Frame when we use nativeI420ToNV21. I > know in some places we use some trickery to convert between the two. Is it the > case here as well? Renamed to NV21. Thought they where in NV12 format before. https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:111: Logging.d(TAG, "Problem closing file"); On 2016/09/26 11:40:01, magjed_webrtc wrote: > Log error here instead, i.e. Logging.e. Done. https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:111: Logging.d(TAG, "Problem closing file"); On 2016/09/26 11:40:01, magjed_webrtc wrote: > Log error here instead, i.e. Logging.e. Done. https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:157: videoReader = new VideoReaderY4M(file); On 2016/09/26 11:40:01, magjed_webrtc wrote: > just inline this in the ctor Done. https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:160: Logging.d(TAG, "Could not open video file: " + file); On 2016/09/26 11:40:02, magjed_webrtc wrote: > We already log in FileVideoCapturer.create so you don't need this one as well. Done. https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... File webrtc/api/android/java/src/org/webrtc/GlVideoFileRenderer.java (right): https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/GlVideoFileRenderer.java:25: private SurfaceTextureHelper.YuvConverter yuvConverter; On 2016/09/27 07:54:28, sakal wrote: > nit: Please remove excessive empty lines. Done. https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/GlVideoFileRenderer.java:39: sharedContext); On 2016/09/27 07:54:27, sakal wrote: > nit: Does this fit on the same line? Done. https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/GlVideoFileRenderer.java:75: int frameSize = outputFileWidth*outputFileHeight*3/2; On 2016/09/27 07:54:27, sakal wrote: > nit: spaces > > How does this work with uneven resolutions? Have added a check in the constructor and throws an exception if it is odd. https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/GlVideoFileRenderer.java:76: ByteBuffer frameBytes = ByteBuffer.allocateDirect(frameSize); On 2016/09/27 07:54:28, sakal wrote: > Can we allocate this in the constructor? > > Java documentation: > The buffers returned by this method typically have somewhat higher allocation > and deallocation costs than non-direct buffers. The contents of direct buffers > may reside outside of the normal garbage-collected heap, and so their impact > upon the memory footprint of an application might not be obvious. It is > therefore recommended that direct buffers be allocated primarily for large, > long-lived buffers that are subject to the underlying system's native I/O > operations. In general it is best to allocate direct buffers only when they > yield a measureable gain in program performance. Moved allocation to constructor and renamed to outputFrameBuffer. https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/GlVideoFileRenderer.java:100: videoOutFile.write(data, offset, width*height); On 2016/09/27 07:54:27, sakal wrote: > nit: spaces Done. https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/GlVideoFileRenderer.java:103: for (int r = height; r<height*3/2; ++r) { On 2016/09/27 07:54:27, sakal wrote: > nit: spaces Done. https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/GlVideoFileRenderer.java:108: for (int r = height; r<height*3/2; ++r) { On 2016/09/27 07:54:28, sakal wrote: > nit: spaces Done. https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/GlVideoFileRenderer.java:120: int outputFrameSize = outputFileWidth * outputFileHeight * 3 / 2; On 2016/09/26 11:40:02, magjed_webrtc wrote: > You already have this variable in |frameSize|. Also, you should handle odd > widths properly. Using frameSize now. See my response for odd widths in FileVideoCapturer. https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/GlVideoFileRenderer.java:128: videoOutFile.write(outputBuffer.array(), outputBuffer.arrayOffset(), outputFrameSize); On 2016/09/26 11:40:02, magjed_webrtc wrote: > You are not calling VideoRenderer.renderFrameDone in this path, so you will be > leaking all frames. Have you tried exercising this path in a local test? Missed that call. Have that path with an instance of this class as a localRenderer for CallActivity. Did not play enough to get a crash from the leak. https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... File webrtc/api/android/java/src/org/webrtc/SurfaceTextureHelper.java (right): https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/SurfaceTextureHelper.java:79: static public class YuvConverter { On 2016/09/27 07:54:28, sakal wrote: > Maybe package visibility would be appropriate? Done. https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/SurfaceTextureHelper.java:349: public YuvConverter getYuvConverter() { On 2016/09/26 11:40:02, magjed_webrtc wrote: > Make this private again. Done. https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/jni/p... File webrtc/api/android/jni/peerconnection_jni.cc (right): https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/jni/p... webrtc/api/android/jni/peerconnection_jni.cc:2010: //size_t src_size = jni->GetDirectBufferCapacity(j_src_buffer); On 2016/09/26 11:40:02, magjed_webrtc wrote: > Clean up and remove unused code. Done. https://codereview.webrtc.org/2273573003/diff/140001/webrtc/api/android/jni/p... webrtc/api/android/jni/peerconnection_jni.cc:2050: jobject j_src_buffer_y, jint j_src_stride_y, On 2016/09/27 07:54:28, sakal wrote: > These are also jbyteArrays, right? Can you use the type? It is ByteBuffers not byte[]. https://codereview.webrtc.org/2273573003/diff/140001/webrtc/examples/androida... File webrtc/examples/androidapp/start_loopback_stubbed_camera_saved_video_out.py (right): https://codereview.webrtc.org/2273573003/diff/140001/webrtc/examples/androida... webrtc/examples/androidapp/start_loopback_stubbed_camera_saved_video_out.py:1: # Copyright 2016 The WebRTC Project Authors. All rights reserved. On 2016/09/27 07:54:28, sakal wrote: > nit: Can you add the shebang line? You have to run the program with monkeyrunner and not python. https://codereview.webrtc.org/2273573003/diff/140001/webrtc/examples/androida... webrtc/examples/androidapp/start_loopback_stubbed_camera_saved_video_out.py:7: from com.android.monkeyrunner import MonkeyRunner, MonkeyDevice On 2016/09/27 07:54:28, sakal wrote: > Move group below standard imports. > > Google Python style guide: > Imports are always put at the top of the file, just after any module comments > and doc strings and before module globals and constants. Imports should be > grouped with the order being most generic to least generic: > > standard library imports > third-party imports > application-specific imports Done. https://codereview.webrtc.org/2273573003/diff/140001/webrtc/examples/androida... webrtc/examples/androidapp/start_loopback_stubbed_camera_saved_video_out.py:81: 'org.appspot.apprtc.AUDIOCODEC': 'OPUS', On 2016/09/27 07:54:28, sakal wrote: > nit: Indent with 4 spaces Done.
It's starting to look good. A few more comments from me. https://codereview.webrtc.org/2273573003/diff/160001/webrtc/api/android/java/... File webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java (right): https://codereview.webrtc.org/2273573003/diff/160001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:72: String[] headerTokens = header.split("[\n ]"); Header cannot contain \n because we break on it, right? Can't we just split on spaces? https://codereview.webrtc.org/2273573003/diff/160001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:111: //mediaFileStream.seek(videoStart + Y4M_FRAME_DELIMETER.length() + 1); Please remove commented out code. https://codereview.webrtc.org/2273573003/diff/160001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:144: private VideoReader videoReader; final https://codereview.webrtc.org/2273573003/diff/160001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:145: Please reduce the amount of empty lines. https://codereview.webrtc.org/2273573003/diff/160001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:148: private Timer timer = new Timer(); final https://codereview.webrtc.org/2273573003/diff/160001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:150: private TimerTask tickTask = new TimerTask() { final https://codereview.webrtc.org/2273573003/diff/160001/webrtc/api/android/java/... File webrtc/api/android/java/src/org/webrtc/VideoFileRenderer.java (right): https://codereview.webrtc.org/2273573003/diff/160001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/VideoFileRenderer.java:25: private SurfaceTextureHelper.YuvConverter yuvConverter; final https://codereview.webrtc.org/2273573003/diff/160001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/VideoFileRenderer.java:26: private HandlerThread renderThread; final https://codereview.webrtc.org/2273573003/diff/160001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/VideoFileRenderer.java:27: private Handler renderThreadHandler; final https://codereview.webrtc.org/2273573003/diff/160001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/VideoFileRenderer.java:28: private FileOutputStream videoOutFile; final https://codereview.webrtc.org/2273573003/diff/160001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/VideoFileRenderer.java:29: private int outputFileWidth; final https://codereview.webrtc.org/2273573003/diff/160001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/VideoFileRenderer.java:30: private int outputFileHeight; final https://codereview.webrtc.org/2273573003/diff/160001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/VideoFileRenderer.java:31: private int outputFrameSize; final https://codereview.webrtc.org/2273573003/diff/160001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/VideoFileRenderer.java:32: private ByteBuffer outputFrameBuffer; final https://codereview.webrtc.org/2273573003/diff/160001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/VideoFileRenderer.java:80: int oesTextureId = frame.textureId; nit: I don't see why you need this variable https://codereview.webrtc.org/2273573003/diff/160001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/VideoFileRenderer.java:92: int width = outputFileWidth; nit: I'd prefer removing these width/height variables. They are not really needed. https://codereview.webrtc.org/2273573003/diff/160001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/VideoFileRenderer.java:105: for (int r = height; r<height * 3 / 2; ++r) { nit: why r instead of more standard i? https://codereview.webrtc.org/2273573003/diff/160001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/VideoFileRenderer.java:105: for (int r = height; r<height * 3 / 2; ++r) { Add spaces around < https://codereview.webrtc.org/2273573003/diff/160001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/VideoFileRenderer.java:110: for (int r = height; r<height * 3 / 2; ++r) { ditto https://codereview.webrtc.org/2273573003/diff/160001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/VideoFileRenderer.java:114: Logging.d(TAG, "Failed to write to file for video out"); Logging.e https://codereview.webrtc.org/2273573003/diff/160001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/VideoFileRenderer.java:143: Logging.d(TAG, "Error closing output video file"); Logging.e https://codereview.webrtc.org/2273573003/diff/160001/webrtc/examples/androida... File webrtc/examples/androidapp/src/org/appspot/apprtc/CallActivity.java (left): https://codereview.webrtc.org/2273573003/diff/160001/webrtc/examples/androida... webrtc/examples/androidapp/src/org/appspot/apprtc/CallActivity.java:213: // Get Intent parameters. nit: move comment https://codereview.webrtc.org/2273573003/diff/160001/webrtc/examples/androida... File webrtc/examples/androidapp/src/org/appspot/apprtc/CallActivity.java (right): https://codereview.webrtc.org/2273573003/diff/160001/webrtc/examples/androida... webrtc/examples/androidapp/src/org/appspot/apprtc/CallActivity.java:112: "org.appspot.apprtc.VIDEO_FILE_AS_CAMERA"; Please correct the indentation on this and following lines. https://codereview.webrtc.org/2273573003/diff/160001/webrtc/examples/androida... webrtc/examples/androidapp/src/org/appspot/apprtc/CallActivity.java:155: private List<VideoRenderer.Callbacks> remoteRenders = new ArrayList<VideoRenderer.Callbacks>(); final https://codereview.webrtc.org/2273573003/diff/160001/webrtc/examples/androida... webrtc/examples/androidapp/src/org/appspot/apprtc/CallActivity.java:228: // When saveRemoveToFile is set we save the video from the remote to a file. saveRemoveToFile -> saveRemoteVideoToFile https://codereview.webrtc.org/2273573003/diff/160001/webrtc/examples/androida... webrtc/examples/androidapp/src/org/appspot/apprtc/CallActivity.java:356: private VideoCapturer createCapturer(CameraEnumerator enumerator) { nit: rename to createCameraCapturer https://codereview.webrtc.org/2273573003/diff/160001/webrtc/examples/androida... File webrtc/examples/androidapp/src/org/appspot/apprtc/ConnectActivity.java (right): https://codereview.webrtc.org/2273573003/diff/160001/webrtc/examples/androida... webrtc/examples/androidapp/src/org/appspot/apprtc/ConnectActivity.java:397: String resolution = sharedPref.getString( Resolution is still always get from the settings, why? https://codereview.webrtc.org/2273573003/diff/160001/webrtc/examples/androida... webrtc/examples/androidapp/src/org/appspot/apprtc/ConnectActivity.java:417: if (!useValuesFromIntent || cameraFps == 0) { !useValuesFromIntent is not needed here https://codereview.webrtc.org/2273573003/diff/160001/webrtc/examples/androida... File webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java (right): https://codereview.webrtc.org/2273573003/diff/160001/webrtc/examples/androida... webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java:1007: else { move else to the line above https://codereview.webrtc.org/2273573003/diff/160001/webrtc/examples/androidt... File webrtc/examples/androidtests/src/org/appspot/apprtc/test/PeerConnectionClientTest.java (left): https://codereview.webrtc.org/2273573003/diff/160001/webrtc/examples/androidt... webrtc/examples/androidtests/src/org/appspot/apprtc/test/PeerConnectionClientTest.java:294: final boolean useCamera2 = captureToTexture && Camera2Enumerator.isSupported(); isSupported now takes context as a parameter, please rebase https://codereview.webrtc.org/2273573003/diff/160001/webrtc/examples/androidt... File webrtc/examples/androidtests/src/org/appspot/apprtc/test/PeerConnectionClientTest.java (right): https://codereview.webrtc.org/2273573003/diff/160001/webrtc/examples/androidt... webrtc/examples/androidtests/src/org/appspot/apprtc/test/PeerConnectionClientTest.java:383: boolean decodeToTexure) nit: can you fix the typo in the parameter name
Fixed review comments https://codereview.webrtc.org/2273573003/diff/160001/webrtc/api/android/java/... File webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java (right): https://codereview.webrtc.org/2273573003/diff/160001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:72: String[] headerTokens = header.split("[\n ]"); On 2016/10/05 13:28:40, sakal wrote: > Header cannot contain \n because we break on it, right? Can't we just split on > spaces? Done. https://codereview.webrtc.org/2273573003/diff/160001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:111: //mediaFileStream.seek(videoStart + Y4M_FRAME_DELIMETER.length() + 1); On 2016/10/05 13:28:40, sakal wrote: > Please remove commented out code. Done. https://codereview.webrtc.org/2273573003/diff/160001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:144: private VideoReader videoReader; On 2016/10/05 13:28:40, sakal wrote: > final Done. https://codereview.webrtc.org/2273573003/diff/160001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:145: On 2016/10/05 13:28:40, sakal wrote: > Please reduce the amount of empty lines. Done. https://codereview.webrtc.org/2273573003/diff/160001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:148: private Timer timer = new Timer(); On 2016/10/05 13:28:40, sakal wrote: > final Done. https://codereview.webrtc.org/2273573003/diff/160001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:150: private TimerTask tickTask = new TimerTask() { On 2016/10/05 13:28:40, sakal wrote: > final Done. https://codereview.webrtc.org/2273573003/diff/160001/webrtc/api/android/java/... File webrtc/api/android/java/src/org/webrtc/VideoFileRenderer.java (right): https://codereview.webrtc.org/2273573003/diff/160001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/VideoFileRenderer.java:80: int oesTextureId = frame.textureId; On 2016/10/05 13:28:41, sakal wrote: > nit: I don't see why you need this variable Removed and using frame.textureId instead of oesTextureId. https://codereview.webrtc.org/2273573003/diff/160001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/VideoFileRenderer.java:92: int width = outputFileWidth; On 2016/10/05 13:28:41, sakal wrote: > nit: I'd prefer removing these width/height variables. They are not really > needed. Done. https://codereview.webrtc.org/2273573003/diff/160001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/VideoFileRenderer.java:105: for (int r = height; r<height * 3 / 2; ++r) { On 2016/10/05 13:28:41, sakal wrote: > nit: why r instead of more standard i? It is r for row. Should I change to i or add comment? https://codereview.webrtc.org/2273573003/diff/160001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/VideoFileRenderer.java:110: for (int r = height; r<height * 3 / 2; ++r) { On 2016/10/05 13:28:41, sakal wrote: > ditto Done. https://codereview.webrtc.org/2273573003/diff/160001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/VideoFileRenderer.java:143: Logging.d(TAG, "Error closing output video file"); On 2016/10/05 13:28:41, sakal wrote: > Logging.e Done. https://codereview.webrtc.org/2273573003/diff/160001/webrtc/examples/androida... File webrtc/examples/androidapp/src/org/appspot/apprtc/CallActivity.java (left): https://codereview.webrtc.org/2273573003/diff/160001/webrtc/examples/androida... webrtc/examples/androidapp/src/org/appspot/apprtc/CallActivity.java:213: // Get Intent parameters. On 2016/10/05 13:28:41, sakal wrote: > nit: move comment Moved to before String roomId = intent.getStringExtra(EXTRA_ROOMID); Right? https://codereview.webrtc.org/2273573003/diff/160001/webrtc/examples/androida... File webrtc/examples/androidapp/src/org/appspot/apprtc/CallActivity.java (right): https://codereview.webrtc.org/2273573003/diff/160001/webrtc/examples/androida... webrtc/examples/androidapp/src/org/appspot/apprtc/CallActivity.java:112: "org.appspot.apprtc.VIDEO_FILE_AS_CAMERA"; On 2016/10/05 13:28:41, sakal wrote: > Please correct the indentation on this and following lines. Done. https://codereview.webrtc.org/2273573003/diff/160001/webrtc/examples/androida... webrtc/examples/androidapp/src/org/appspot/apprtc/CallActivity.java:155: private List<VideoRenderer.Callbacks> remoteRenders = new ArrayList<VideoRenderer.Callbacks>(); On 2016/10/05 13:28:41, sakal wrote: > final Made remoteRenders final and changed name to remoteRenderers. The other two fields are initialized in onCreate(). https://codereview.webrtc.org/2273573003/diff/160001/webrtc/examples/androida... webrtc/examples/androidapp/src/org/appspot/apprtc/CallActivity.java:228: // When saveRemoveToFile is set we save the video from the remote to a file. On 2016/10/05 13:28:41, sakal wrote: > saveRemoveToFile -> saveRemoteVideoToFile Done. https://codereview.webrtc.org/2273573003/diff/160001/webrtc/examples/androida... webrtc/examples/androidapp/src/org/appspot/apprtc/CallActivity.java:356: private VideoCapturer createCapturer(CameraEnumerator enumerator) { On 2016/10/05 13:28:41, sakal wrote: > nit: rename to createCameraCapturer Done. Change name of createVideoCapturer in PeerConnectionClientTests.java to createCameraCapturer to be consistent. https://codereview.webrtc.org/2273573003/diff/160001/webrtc/examples/androida... File webrtc/examples/androidapp/src/org/appspot/apprtc/ConnectActivity.java (right): https://codereview.webrtc.org/2273573003/diff/160001/webrtc/examples/androida... webrtc/examples/androidapp/src/org/appspot/apprtc/ConnectActivity.java:397: String resolution = sharedPref.getString( On 2016/10/05 13:28:41, sakal wrote: > Resolution is still always get from the settings, why? Did misstake. Added so it can be provided on command line. https://codereview.webrtc.org/2273573003/diff/160001/webrtc/examples/androida... webrtc/examples/androidapp/src/org/appspot/apprtc/ConnectActivity.java:417: if (!useValuesFromIntent || cameraFps == 0) { On 2016/10/05 13:28:41, sakal wrote: > !useValuesFromIntent is not needed here Done. https://codereview.webrtc.org/2273573003/diff/160001/webrtc/examples/androida... File webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java (right): https://codereview.webrtc.org/2273573003/diff/160001/webrtc/examples/androida... webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java:1007: else { On 2016/10/05 13:28:42, sakal wrote: > move else to the line above Done. https://codereview.webrtc.org/2273573003/diff/160001/webrtc/examples/androidt... File webrtc/examples/androidtests/src/org/appspot/apprtc/test/PeerConnectionClientTest.java (right): https://codereview.webrtc.org/2273573003/diff/160001/webrtc/examples/androidt... webrtc/examples/androidtests/src/org/appspot/apprtc/test/PeerConnectionClientTest.java:383: boolean decodeToTexure) On 2016/10/05 13:28:42, sakal wrote: > nit: can you fix the typo in the parameter name Done.
lgtm In the future, please make a rebase in a separate patchset. It makes reviewing easier. https://codereview.webrtc.org/2273573003/diff/180001/webrtc/examples/androidt... File webrtc/examples/androidtests/src/org/appspot/apprtc/test/PeerConnectionClientTest.java (right): https://codereview.webrtc.org/2273573003/diff/180001/webrtc/examples/androidt... webrtc/examples/androidtests/src/org/appspot/apprtc/test/PeerConnectionClientTest.java:415: createCameraCapturer(false), false); nit: add comments to literals in this file, like createCameraCapturer(false /* captureToTexture */), false /* decodeToTexture */);
Only nit comments left. Please run 'git cl format' before uploading the next patch set. It should take care of formatting for you. https://codereview.webrtc.org/2273573003/diff/180001/webrtc/api/android/java/... File webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java (right): https://codereview.webrtc.org/2273573003/diff/180001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:19: import java.io.File; unused import https://codereview.webrtc.org/2273573003/diff/180001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:117: Logging.d(TAG, "frameDelim: '" + frameDelimStr + "'"); Remove this log and include the frameDelimStr information in the exception below. https://codereview.webrtc.org/2273573003/diff/180001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:133: Logging.e(TAG, "Problem closing file"); Print exception information. You can add it like: Logging.e(TAG, "Problem closing file", e); https://codereview.webrtc.org/2273573003/diff/180001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:167: public static FileVideoCapturer create(String inputFile) { This function looks unnecessary, why not use the ctor directly? https://codereview.webrtc.org/2273573003/diff/180001/webrtc/api/android/java/... File webrtc/api/android/java/src/org/webrtc/VideoFileRenderer.java (right): https://codereview.webrtc.org/2273573003/diff/180001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/VideoFileRenderer.java:20: * Can be used to saves the video frames to file. nit spelling: save https://codereview.webrtc.org/2273573003/diff/180001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/VideoFileRenderer.java:68: float frameAspectRatio = (frame.rotationDegree % 180 == 0) ? (float) frame.width / frame.height Write it like this instead: final float frameAspectRatio = frame.rotatedWidth() / frame.rotatedHeight(); https://codereview.webrtc.org/2273573003/diff/180001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/VideoFileRenderer.java:77: if (!frame.yuvFrame) { Change the order of the if-statement and the try-statement so you can reuse the common code: try { videoOutFile.write("FRAME\n".getBytes()); if (!frame.yuvFrame) { ... } else { ... } } catch (IOException e) { Logging.e(TAG, "Failed to write to file for video out", e); throw new RuntimeException(e); } finally { VideoRenderer.renderFrameDone(frame); } https://codereview.webrtc.org/2273573003/diff/180001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/VideoFileRenderer.java:90: Logging.d(TAG, "arrayOffset(): " + outputFrameBuffer.arrayOffset() + " hasArray: " Remove the hasArray stuff. If it does not have an array, it will throw an exception when you call .array() above. https://codereview.webrtc.org/2273573003/diff/180001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/VideoFileRenderer.java:129: Logging.d(TAG, "Error closing output video file"); Include exception in log, add it as the last argument. https://codereview.webrtc.org/2273573003/diff/180001/webrtc/api/android/jni/p... File webrtc/api/android/jni/peerconnection_jni.cc (right): https://codereview.webrtc.org/2273573003/diff/180001/webrtc/api/android/jni/p... webrtc/api/android/jni/peerconnection_jni.cc:2015: RTC_CHECK(src_size >= src_stride * height * 3 / 2) Change to RTC_CHECK_GE and remove the manual log. https://codereview.webrtc.org/2273573003/diff/180001/webrtc/api/android/jni/p... webrtc/api/android/jni/peerconnection_jni.cc:2017: RTC_CHECK(dst_size >= dst_stride * height * 3 / 2) ditto: Change to RTC_CHECK_GE and remove the manual log. https://codereview.webrtc.org/2273573003/diff/180001/webrtc/api/android/jni/p... webrtc/api/android/jni/peerconnection_jni.cc:2052: RTC_CHECK(src_size_y >= j_src_stride_y * height) ditto: Change to RTC_CHECK_GE and remove the manual log. Same for the lines under.
Fixed review comments https://codereview.webrtc.org/2273573003/diff/180001/webrtc/api/android/java/... File webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java (right): https://codereview.webrtc.org/2273573003/diff/180001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:19: import java.io.File; On 2016/10/08 10:46:04, magjed_webrtc wrote: > unused import Done. https://codereview.webrtc.org/2273573003/diff/180001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:117: Logging.d(TAG, "frameDelim: '" + frameDelimStr + "'"); On 2016/10/08 10:46:04, magjed_webrtc wrote: > Remove this log and include the frameDelimStr information in the exception > below. Done. https://codereview.webrtc.org/2273573003/diff/180001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:133: Logging.e(TAG, "Problem closing file"); On 2016/10/08 10:46:04, magjed_webrtc wrote: > Print exception information. You can add it like: Logging.e(TAG, "Problem > closing file", e); Done. https://codereview.webrtc.org/2273573003/diff/180001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:167: public static FileVideoCapturer create(String inputFile) { On 2016/10/08 10:46:04, magjed_webrtc wrote: > This function looks unnecessary, why not use the ctor directly? Fixed. Catches exception in CallActivity instead of checking for null. https://codereview.webrtc.org/2273573003/diff/180001/webrtc/api/android/java/... File webrtc/api/android/java/src/org/webrtc/VideoFileRenderer.java (right): https://codereview.webrtc.org/2273573003/diff/180001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/VideoFileRenderer.java:77: if (!frame.yuvFrame) { On 2016/10/08 10:46:04, magjed_webrtc wrote: > Change the order of the if-statement and the try-statement so you can reuse the > common code: > try { > videoOutFile.write("FRAME\n".getBytes()); > if (!frame.yuvFrame) { > ... > } else { > ... > } > } catch (IOException e) { > Logging.e(TAG, "Failed to write to file for video out", e); > throw new RuntimeException(e); > } finally { > VideoRenderer.renderFrameDone(frame); > } Done. https://codereview.webrtc.org/2273573003/diff/180001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/VideoFileRenderer.java:90: Logging.d(TAG, "arrayOffset(): " + outputFrameBuffer.arrayOffset() + " hasArray: " On 2016/10/08 10:46:04, magjed_webrtc wrote: > Remove the hasArray stuff. If it does not have an array, it will throw an > exception when you call .array() above. Done. https://codereview.webrtc.org/2273573003/diff/180001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/VideoFileRenderer.java:129: Logging.d(TAG, "Error closing output video file"); On 2016/10/08 10:46:05, magjed_webrtc wrote: > Include exception in log, add it as the last argument. Done. https://codereview.webrtc.org/2273573003/diff/180001/webrtc/api/android/jni/p... File webrtc/api/android/jni/peerconnection_jni.cc (right): https://codereview.webrtc.org/2273573003/diff/180001/webrtc/api/android/jni/p... webrtc/api/android/jni/peerconnection_jni.cc:2015: RTC_CHECK(src_size >= src_stride * height * 3 / 2) On 2016/10/08 10:46:05, magjed_webrtc wrote: > Change to RTC_CHECK_GE and remove the manual log. Done. https://codereview.webrtc.org/2273573003/diff/180001/webrtc/api/android/jni/p... webrtc/api/android/jni/peerconnection_jni.cc:2017: RTC_CHECK(dst_size >= dst_stride * height * 3 / 2) On 2016/10/08 10:46:05, magjed_webrtc wrote: > ditto: Change to RTC_CHECK_GE and remove the manual log. Done. https://codereview.webrtc.org/2273573003/diff/180001/webrtc/api/android/jni/p... webrtc/api/android/jni/peerconnection_jni.cc:2052: RTC_CHECK(src_size_y >= j_src_stride_y * height) On 2016/10/08 10:46:05, magjed_webrtc wrote: > ditto: Change to RTC_CHECK_GE and remove the manual log. Same for the lines > under. Done. https://codereview.webrtc.org/2273573003/diff/180001/webrtc/examples/androidt... File webrtc/examples/androidtests/src/org/appspot/apprtc/test/PeerConnectionClientTest.java (right): https://codereview.webrtc.org/2273573003/diff/180001/webrtc/examples/androidt... webrtc/examples/androidtests/src/org/appspot/apprtc/test/PeerConnectionClientTest.java:415: createCameraCapturer(false), false); On 2016/10/07 11:57:21, sakal wrote: > nit: add comments to literals in this file, like createCameraCapturer(false /* > captureToTexture */), false /* decodeToTexture */); Done.
Fixed review comments https://codereview.webrtc.org/2273573003/diff/180001/webrtc/api/android/java/... File webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java (right): https://codereview.webrtc.org/2273573003/diff/180001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:19: import java.io.File; On 2016/10/08 10:46:04, magjed_webrtc wrote: > unused import Done. https://codereview.webrtc.org/2273573003/diff/180001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:117: Logging.d(TAG, "frameDelim: '" + frameDelimStr + "'"); On 2016/10/08 10:46:04, magjed_webrtc wrote: > Remove this log and include the frameDelimStr information in the exception > below. Done. https://codereview.webrtc.org/2273573003/diff/180001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:133: Logging.e(TAG, "Problem closing file"); On 2016/10/08 10:46:04, magjed_webrtc wrote: > Print exception information. You can add it like: Logging.e(TAG, "Problem > closing file", e); Done. https://codereview.webrtc.org/2273573003/diff/180001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/FileVideoCapturer.java:167: public static FileVideoCapturer create(String inputFile) { On 2016/10/08 10:46:04, magjed_webrtc wrote: > This function looks unnecessary, why not use the ctor directly? Fixed. Catches exception in CallActivity instead of checking for null. https://codereview.webrtc.org/2273573003/diff/180001/webrtc/api/android/java/... File webrtc/api/android/java/src/org/webrtc/VideoFileRenderer.java (right): https://codereview.webrtc.org/2273573003/diff/180001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/VideoFileRenderer.java:77: if (!frame.yuvFrame) { On 2016/10/08 10:46:04, magjed_webrtc wrote: > Change the order of the if-statement and the try-statement so you can reuse the > common code: > try { > videoOutFile.write("FRAME\n".getBytes()); > if (!frame.yuvFrame) { > ... > } else { > ... > } > } catch (IOException e) { > Logging.e(TAG, "Failed to write to file for video out", e); > throw new RuntimeException(e); > } finally { > VideoRenderer.renderFrameDone(frame); > } Done. https://codereview.webrtc.org/2273573003/diff/180001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/VideoFileRenderer.java:90: Logging.d(TAG, "arrayOffset(): " + outputFrameBuffer.arrayOffset() + " hasArray: " On 2016/10/08 10:46:04, magjed_webrtc wrote: > Remove the hasArray stuff. If it does not have an array, it will throw an > exception when you call .array() above. Done. https://codereview.webrtc.org/2273573003/diff/180001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/VideoFileRenderer.java:129: Logging.d(TAG, "Error closing output video file"); On 2016/10/08 10:46:05, magjed_webrtc wrote: > Include exception in log, add it as the last argument. Done. https://codereview.webrtc.org/2273573003/diff/180001/webrtc/api/android/jni/p... File webrtc/api/android/jni/peerconnection_jni.cc (right): https://codereview.webrtc.org/2273573003/diff/180001/webrtc/api/android/jni/p... webrtc/api/android/jni/peerconnection_jni.cc:2015: RTC_CHECK(src_size >= src_stride * height * 3 / 2) On 2016/10/08 10:46:05, magjed_webrtc wrote: > Change to RTC_CHECK_GE and remove the manual log. Done. https://codereview.webrtc.org/2273573003/diff/180001/webrtc/api/android/jni/p... webrtc/api/android/jni/peerconnection_jni.cc:2017: RTC_CHECK(dst_size >= dst_stride * height * 3 / 2) On 2016/10/08 10:46:05, magjed_webrtc wrote: > ditto: Change to RTC_CHECK_GE and remove the manual log. Done. https://codereview.webrtc.org/2273573003/diff/180001/webrtc/api/android/jni/p... webrtc/api/android/jni/peerconnection_jni.cc:2052: RTC_CHECK(src_size_y >= j_src_stride_y * height) On 2016/10/08 10:46:05, magjed_webrtc wrote: > ditto: Change to RTC_CHECK_GE and remove the manual log. Same for the lines > under. Done. https://codereview.webrtc.org/2273573003/diff/180001/webrtc/examples/androidt... File webrtc/examples/androidtests/src/org/appspot/apprtc/test/PeerConnectionClientTest.java (right): https://codereview.webrtc.org/2273573003/diff/180001/webrtc/examples/androidt... webrtc/examples/androidtests/src/org/appspot/apprtc/test/PeerConnectionClientTest.java:415: createCameraCapturer(false), false); On 2016/10/07 11:57:21, sakal wrote: > nit: add comments to literals in this file, like createCameraCapturer(false /* > captureToTexture */), false /* decodeToTexture */); Done.
Fixed a bug form last upload, which I introduced when refactoring frame aspect ratio calculation for video out.
Found that the synchronization in VideoFileRenderer was bad. Fixed that.
https://codereview.webrtc.org/2273573003/diff/240001/webrtc/api/android/java/... File webrtc/api/android/java/src/org/webrtc/VideoFileRenderer.java (right): https://codereview.webrtc.org/2273573003/diff/240001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/VideoFileRenderer.java:61: synchronized (handlerLock) { What's the purpose of this lock? renderThreadHandler is final so you don't need to lock it. https://codereview.webrtc.org/2273573003/diff/240001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/VideoFileRenderer.java:85: videoOutFile.write("FRAME\n".getBytes()); Move this above the if-statement, and remove the same line in the else-clause. https://codereview.webrtc.org/2273573003/diff/240001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/VideoFileRenderer.java:134: renderThread.quitSafely(); quitSafely was added in API 18, so we can't use it yet. Call quit() inside the Runnable instead.
Fixed comments in #28 https://codereview.webrtc.org/2273573003/diff/240001/webrtc/api/android/java/... File webrtc/api/android/java/src/org/webrtc/VideoFileRenderer.java (right): https://codereview.webrtc.org/2273573003/diff/240001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/VideoFileRenderer.java:61: synchronized (handlerLock) { On 2016/10/11 13:48:16, magjed_webrtc wrote: > What's the purpose of this lock? renderThreadHandler is final so you don't need > to lock it. Now lock is removed. Planned to set renderThreadHandler to null and check for null in renderThread(), therefore I added the lock. Later figured out is was not necessary, but I forgot to remove the lock. https://codereview.webrtc.org/2273573003/diff/240001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/VideoFileRenderer.java:85: videoOutFile.write("FRAME\n".getBytes()); On 2016/10/11 13:48:16, magjed_webrtc wrote: > Move this above the if-statement, and remove the same line in the else-clause. Done. https://codereview.webrtc.org/2273573003/diff/240001/webrtc/api/android/java/... webrtc/api/android/java/src/org/webrtc/VideoFileRenderer.java:134: renderThread.quitSafely(); On 2016/10/11 13:48:16, magjed_webrtc wrote: > quitSafely was added in API 18, so we can't use it yet. Call quit() inside the > Runnable instead. Done.
Fixed I introduced when changing from HandlerThread.quitSafely() to HandlerThread.quit()
lgtm
The CQ bit was checked by mandermo@webrtc.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.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios32_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/11461) ios64_gyp_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gyp_dbg/builds/1373) ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/13702)
The CQ bit was checked by jansson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sakal@webrtc.org Link to the patchset: https://codereview.webrtc.org/2273573003/#ps280001 (title: "Fixed I introduced when changing from HandlerThread.quitSafely() to HandlerThread.quit()")
The CQ bit was unchecked by jansson@chromium.org
The CQ bit was checked by mandermo@webrtc.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.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_asan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/18482)
The CQ bit was checked by mandermo@webrtc.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.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/9196)
The CQ bit was checked by mandermo@google.com 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.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/9255)
The CQ bit was checked by mandermo@google.com 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.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/9257)
Description was changed from ========== Support for video file instead of camera and output video out to file When video out to file is enabled the remote video which is recorded is not show on screen. You can use this command line for file input and output: monkeyrunner ./webrtc/examples/androidapp/start_loopback_stubbed_camera_saved_video_out.py --devname 02157df28cd47001 --videoin /storage/emulated/0/reference_video_1280x720_30fps.y4m --videoout /storage/emulated/0/output.y4m --videoout_width 1280 --videoout_height 720 --videooutsave /tmp/out.y4m ========== to ========== Support for video file instead of camera and output video out to file When video out to file is enabled the remote video which is recorded is not show on screen. You can use this command line for file input and output: monkeyrunner ./webrtc/examples/androidapp/start_loopback_stubbed_camera_saved_video_out.py --devname 02157df28cd47001 --videoin /storage/emulated/0/reference_video_1280x720_30fps.y4m --videoout /storage/emulated/0/output.y4m --videoout_width 1280 --videoout_height 720 --videooutsave /tmp/out.y4m BUG=webrtc:6545 ==========
The CQ bit was checked by mandermo@google.com 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.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/9259)
The CQ bit was checked by mandermo@google.com 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.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/9262)
The CQ bit was checked by mandermo@google.com 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.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mandermo@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from sakal@webrtc.org, magjed@webrtc.org Link to the patchset: https://codereview.webrtc.org/2273573003/#ps320001 (title: "Fixed copyright header for start_loopback_stubbed_camera_saved_video_out.py")
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.
Description was changed from ========== Support for video file instead of camera and output video out to file When video out to file is enabled the remote video which is recorded is not show on screen. You can use this command line for file input and output: monkeyrunner ./webrtc/examples/androidapp/start_loopback_stubbed_camera_saved_video_out.py --devname 02157df28cd47001 --videoin /storage/emulated/0/reference_video_1280x720_30fps.y4m --videoout /storage/emulated/0/output.y4m --videoout_width 1280 --videoout_height 720 --videooutsave /tmp/out.y4m BUG=webrtc:6545 ========== to ========== Support for video file instead of camera and output video out to file When video out to file is enabled the remote video which is recorded is not show on screen. You can use this command line for file input and output: monkeyrunner ./webrtc/examples/androidapp/start_loopback_stubbed_camera_saved_video_out.py --devname 02157df28cd47001 --videoin /storage/emulated/0/reference_video_1280x720_30fps.y4m --videoout /storage/emulated/0/output.y4m --videoout_width 1280 --videoout_height 720 --videooutsave /tmp/out.y4m BUG=webrtc:6545 ==========
Message was sent while issue was closed.
Committed patchset #17 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== Support for video file instead of camera and output video out to file When video out to file is enabled the remote video which is recorded is not show on screen. You can use this command line for file input and output: monkeyrunner ./webrtc/examples/androidapp/start_loopback_stubbed_camera_saved_video_out.py --devname 02157df28cd47001 --videoin /storage/emulated/0/reference_video_1280x720_30fps.y4m --videoout /storage/emulated/0/output.y4m --videoout_width 1280 --videoout_height 720 --videooutsave /tmp/out.y4m BUG=webrtc:6545 ========== to ========== Support for video file instead of camera and output video out to file When video out to file is enabled the remote video which is recorded is not show on screen. You can use this command line for file input and output: monkeyrunner ./webrtc/examples/androidapp/start_loopback_stubbed_camera_saved_video_out.py --devname 02157df28cd47001 --videoin /storage/emulated/0/reference_video_1280x720_30fps.y4m --videoout /storage/emulated/0/output.y4m --videoout_width 1280 --videoout_height 720 --videooutsave /tmp/out.y4m BUG=webrtc:6545 Committed: https://crrev.com/44666997ca912705f8f96c9bd211e719525a3ccc Cr-Commit-Position: refs/heads/master@{#14660} ==========
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/44666997ca912705f8f96c9bd211e719525a3ccc Cr-Commit-Position: refs/heads/master@{#14660}
Message was sent while issue was closed.
A revert of this CL (patchset #17 id:320001) has been created in https://codereview.webrtc.org/2425763003/ by kjellander@webrtc.org. The reason for reverting is: Breaks internal project..
Message was sent while issue was closed.
A revert of this CL (patchset #17 id:320001) has been created in https://codereview.webrtc.org/2431663002/ by mandermo@google.com. The reason for reverting is: The patch breaks downstream. |
