Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2576283004:
Video collected by VideoFileRenderer is first saved on the native heap, then saved to disk during r… (Closed)
DescriptionVideo collected by VideoFileRenderer is first saved on the native heap, then saved to disk during release.
BUG=webrtc:6545
Review-Url: https://codereview.webrtc.org/2576283004
Cr-Commit-Position: refs/heads/master@{#16167}
Committed: https://chromium.googlesource.com/external/webrtc/+/eef94d99952f64e543075a417e54d476067f32d2
Patch Set 1 #
Total comments: 6
Patch Set 2 : Fixed review comments #
Total comments: 5
Patch Set 3 : Merge branch 'master' into native_bytebuffer #Patch Set 4 : Better logging and new debug log #Patch Set 5 : Buffering of YUV frames also #Patch Set 6 : Format change for peerconnection_jni.cc as suggested by git cl format #
Total comments: 8
Patch Set 7 : Fixing review comments #
Total comments: 2
Patch Set 8 : position(0) to rewind() #Patch Set 9 : Moved add to under if #
Messages
Total messages: 41 (18 generated)
mandermo@webrtc.org changed reviewers: + jansson@webrtc.org
CL for video saved in RAM.
mandermo@webrtc.org changed reviewers: + kjellander@webrtc.org
Please add reference to the bug as well as an actual owner of this code. https://codereview.webrtc.org/2576283004/diff/1/webrtc/sdk/android/api/org/we... File webrtc/sdk/android/api/org/webrtc/VideoFileRenderer.java (right): https://codereview.webrtc.org/2576283004/diff/1/webrtc/sdk/android/api/org/we... webrtc/sdk/android/api/org/webrtc/VideoFileRenderer.java:116: videoOutFile.write("FRAME\n".getBytes()); Please move this to the line before line 120 instead. https://codereview.webrtc.org/2576283004/diff/1/webrtc/sdk/android/api/org/we... webrtc/sdk/android/api/org/webrtc/VideoFileRenderer.java:152: videoOutFile.write(data); Can't you just use videoOutFile.write(buffer.array()); ? According to https://docs.oracle.com/javase/7/docs/api/java/nio/ByteBuffer.html#array() it returns the backing byte array. Or maybe it doesn't exist on Android? https://codereview.webrtc.org/2576283004/diff/1/webrtc/sdk/android/api/org/we... webrtc/sdk/android/api/org/webrtc/VideoFileRenderer.java:156: videoOutFile.close(); Please log something on the INFO level that confirms everything was written correctly as well. It might be useful to also log the number of frames written, the dimensions of them, the total bytes written and the filename it was written into as well.
Description was changed from ========== Video collected by VideoFileRenderer is first saved on the native heap, then saved to disk during release. BUG= ========== to ========== Video collected by VideoFileRenderer is first saved on the native heap, then saved to disk during release. BUG=webrtc:6545 ==========
Fixed review comments. https://codereview.webrtc.org/2576283004/diff/1/webrtc/sdk/android/api/org/we... File webrtc/sdk/android/api/org/webrtc/VideoFileRenderer.java (right): https://codereview.webrtc.org/2576283004/diff/1/webrtc/sdk/android/api/org/we... webrtc/sdk/android/api/org/webrtc/VideoFileRenderer.java:116: videoOutFile.write("FRAME\n".getBytes()); On 2016/12/20 13:21:46, kjellander_webrtc wrote: > Please move this to the line before line 120 instead. Done. https://codereview.webrtc.org/2576283004/diff/1/webrtc/sdk/android/api/org/we... webrtc/sdk/android/api/org/webrtc/VideoFileRenderer.java:152: videoOutFile.write(data); On 2016/12/20 13:21:46, kjellander_webrtc wrote: > Can't you just use videoOutFile.write(buffer.array()); ? > According to > https://docs.oracle.com/javase/7/docs/api/java/nio/ByteBuffer.html#array() it > returns the backing byte array. Or maybe it doesn't exist on Android? Depends how you construct it if you can use array(). In that case you would also need to take care of the offset(). In this case it will throw UnsupportedOperationException if you use array(). https://codereview.webrtc.org/2576283004/diff/1/webrtc/sdk/android/api/org/we... webrtc/sdk/android/api/org/webrtc/VideoFileRenderer.java:156: videoOutFile.close(); On 2016/12/20 13:21:46, kjellander_webrtc wrote: > Please log something on the INFO level that confirms everything was written > correctly as well. > It might be useful to also log the number of frames written, the dimensions of > them, the total bytes written and the filename it was written into as well. Done, except file size. There is no Logging.i, so I use Logging.d. Should I use Log.i instead?
mandermo@webrtc.org changed reviewers: + magjed@webrtc.org
+magjed
lgtm but you need magjed to have a look as well. https://codereview.webrtc.org/2576283004/diff/20001/webrtc/sdk/android/api/or... File webrtc/sdk/android/api/org/webrtc/VideoFileRenderer.java (right): https://codereview.webrtc.org/2576283004/diff/20001/webrtc/sdk/android/api/or... webrtc/sdk/android/api/org/webrtc/VideoFileRenderer.java:165: Logging.e(TAG, "Error closing output video file"); The error could be something other than closing the file, like an error on writing etc. I suggest making the message more generic and also include the exception in the logging method call, since most such methods have an overloaded version accepting a Throwable be passed to it.
https://codereview.webrtc.org/2576283004/diff/20001/webrtc/sdk/android/api/or... File webrtc/sdk/android/api/org/webrtc/VideoFileRenderer.java (right): https://codereview.webrtc.org/2576283004/diff/20001/webrtc/sdk/android/api/or... webrtc/sdk/android/api/org/webrtc/VideoFileRenderer.java:122: videoOutFile.write( Why not buffer these frames as well?
https://codereview.webrtc.org/2576283004/diff/20001/webrtc/sdk/android/api/or... File webrtc/sdk/android/api/org/webrtc/VideoFileRenderer.java (right): https://codereview.webrtc.org/2576283004/diff/20001/webrtc/sdk/android/api/or... webrtc/sdk/android/api/org/webrtc/VideoFileRenderer.java:122: videoOutFile.write( On 2017/01/11 14:49:05, magjed_webrtc wrote: > Why not buffer these frames as well? I can fix it if you want. The code with the performance problem did not use YUV frames, so did not feel there was any need for buffering in this case, but probably the code will be a more consistent with buffering in both cases. https://codereview.webrtc.org/2576283004/diff/20001/webrtc/sdk/android/api/or... webrtc/sdk/android/api/org/webrtc/VideoFileRenderer.java:165: Logging.e(TAG, "Error closing output video file"); On 2016/12/21 12:45:51, kjellander_webrtc OOO to Jan19 wrote: > The error could be something other than closing the file, like an error on > writing etc. I suggest making the message more generic and also include the > exception in the logging method call, since most such methods have an overloaded > version accepting a Throwable be passed to it. Done.
https://codereview.webrtc.org/2576283004/diff/20001/webrtc/sdk/android/api/or... File webrtc/sdk/android/api/org/webrtc/VideoFileRenderer.java (right): https://codereview.webrtc.org/2576283004/diff/20001/webrtc/sdk/android/api/or... webrtc/sdk/android/api/org/webrtc/VideoFileRenderer.java:122: videoOutFile.write( On 2017/01/12 10:22:53, mandermo wrote: > On 2017/01/11 14:49:05, magjed_webrtc wrote: > > Why not buffer these frames as well? > > I can fix it if you want. The code with the performance problem did not use YUV > frames, so did not feel there was any need for buffering in this case, but > probably the code will be a more consistent with buffering in both cases. Yeah, I think it's best to be consistent. The bottleneck must be IO here, so YUV frames will have the same problem as texture frames.
Fixed buffering for YUV frames also. Agree consistency is good. Agree that IO is the bottleneck and not the conversion, but we only used non-YUV frames where we had the performance problem.
Format change for peerconnection_jni.cc as suggested by git cl format
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: This issue passed the CQ dry run.
Looks good now, but I have some final comments before I stamp the CL. https://codereview.webrtc.org/2576283004/diff/100001/webrtc/sdk/android/api/o... File webrtc/sdk/android/api/org/webrtc/VideoFileRenderer.java (right): https://codereview.webrtc.org/2576283004/diff/100001/webrtc/sdk/android/api/o... webrtc/sdk/android/api/org/webrtc/VideoFileRenderer.java:101: ByteBuffer buffer = nativeCreateNativeByteBuffer(outputFrameSize); Put this before the if-statement since it's the same for both yuv and non-yuv. https://codereview.webrtc.org/2576283004/diff/100001/webrtc/sdk/android/api/o... webrtc/sdk/android/api/org/webrtc/VideoFileRenderer.java:116: buffer.position(0); I think it's slightly more clear to use buffer.rewind() than buffer.position(0). Also, move this below the if-statement since it's shared for both yuv and non-yuv. https://codereview.webrtc.org/2576283004/diff/100001/webrtc/sdk/android/api/o... webrtc/sdk/android/api/org/webrtc/VideoFileRenderer.java:118: rawFrames.add(buffer); Same here, move below if-statement.
https://codereview.webrtc.org/2576283004/diff/100001/webrtc/sdk/android/api/o... File webrtc/sdk/android/api/org/webrtc/VideoFileRenderer.java (right): https://codereview.webrtc.org/2576283004/diff/100001/webrtc/sdk/android/api/o... webrtc/sdk/android/api/org/webrtc/VideoFileRenderer.java:116: buffer.position(0); On 2017/01/16 13:23:12, magjed_webrtc wrote: > I think it's slightly more clear to use buffer.rewind() than buffer.position(0). > Also, move this below the if-statement since it's shared for both yuv and > non-yuv. Done. https://codereview.webrtc.org/2576283004/diff/100001/webrtc/sdk/android/api/o... webrtc/sdk/android/api/org/webrtc/VideoFileRenderer.java:118: rawFrames.add(buffer); On 2017/01/16 13:23:12, magjed_webrtc wrote: > Same here, move below if-statement. Done.
https://codereview.webrtc.org/2576283004/diff/100001/webrtc/sdk/android/api/o... File webrtc/sdk/android/api/org/webrtc/VideoFileRenderer.java (right): https://codereview.webrtc.org/2576283004/diff/100001/webrtc/sdk/android/api/o... webrtc/sdk/android/api/org/webrtc/VideoFileRenderer.java:118: rawFrames.add(buffer); On 2017/01/16 17:16:55, mandermo wrote: > On 2017/01/16 13:23:12, magjed_webrtc wrote: > > Same here, move below if-statement. > > Done. It's not done. https://codereview.webrtc.org/2576283004/diff/120001/webrtc/sdk/android/api/o... File webrtc/sdk/android/api/org/webrtc/VideoFileRenderer.java (right): https://codereview.webrtc.org/2576283004/diff/120001/webrtc/sdk/android/api/o... webrtc/sdk/android/api/org/webrtc/VideoFileRenderer.java:125: buffer.position(0); Can you change this to buffer.rewind()?
https://codereview.webrtc.org/2576283004/diff/120001/webrtc/sdk/android/api/o... File webrtc/sdk/android/api/org/webrtc/VideoFileRenderer.java (right): https://codereview.webrtc.org/2576283004/diff/120001/webrtc/sdk/android/api/o... webrtc/sdk/android/api/org/webrtc/VideoFileRenderer.java:125: buffer.position(0); On 2017/01/17 08:29:12, magjed_webrtc wrote: > Can you change this to buffer.rewind()? Done.
https://codereview.webrtc.org/2576283004/diff/100001/webrtc/sdk/android/api/o... File webrtc/sdk/android/api/org/webrtc/VideoFileRenderer.java (right): https://codereview.webrtc.org/2576283004/diff/100001/webrtc/sdk/android/api/o... webrtc/sdk/android/api/org/webrtc/VideoFileRenderer.java:118: rawFrames.add(buffer); On 2017/01/17 08:29:12, magjed_webrtc wrote: > On 2017/01/16 17:16:55, mandermo wrote: > > On 2017/01/16 13:23:12, magjed_webrtc wrote: > > > Same here, move below if-statement. > > > > Done. > > It's not done. This is still not done, can you please fix it?
https://codereview.webrtc.org/2576283004/diff/100001/webrtc/sdk/android/api/o... File webrtc/sdk/android/api/org/webrtc/VideoFileRenderer.java (right): https://codereview.webrtc.org/2576283004/diff/100001/webrtc/sdk/android/api/o... webrtc/sdk/android/api/org/webrtc/VideoFileRenderer.java:118: rawFrames.add(buffer); On 2017/01/17 10:49:58, magjed_webrtc wrote: > On 2017/01/17 08:29:12, magjed_webrtc wrote: > > On 2017/01/16 17:16:55, mandermo wrote: > > > On 2017/01/16 13:23:12, magjed_webrtc wrote: > > > > Same here, move below if-statement. > > > > > > Done. > > > > It's not done. > > This is still not done, can you please fix it? Sorry for mistake, fixed now.
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: linux_tsan2 on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/18864)
The CQ bit was checked by mandermo@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kjellander@webrtc.org Link to the patchset: https://codereview.webrtc.org/2576283004/#ps160001 (title: "Moved add to under if")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_msan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_msan/builds/16779)
On 2017/01/17 14:48:30, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_msan on master.tryserver.webrtc (JOB_FAILED, > http://build.chromium.org/p/tryserver.webrtc/builders/linux_msan/builds/16779) Looks like a test flake, just retry that bot (or just CQ the CL again)
The CQ bit was checked by jansson@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1484840690421640,
"parent_rev": "3626865be216309d99321998af38202985c69da5", "commit_rev":
"eef94d99952f64e543075a417e54d476067f32d2"}
Message was sent while issue was closed.
Description was changed from ========== Video collected by VideoFileRenderer is first saved on the native heap, then saved to disk during release. BUG=webrtc:6545 ========== to ========== Video collected by VideoFileRenderer is first saved on the native heap, then saved to disk during release. BUG=webrtc:6545 Review-Url: https://codereview.webrtc.org/2576283004 Cr-Commit-Position: refs/heads/master@{#16167} Committed: https://chromium.googlesource.com/external/webrtc/+/eef94d99952f64e543075a417... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/external/webrtc/+/eef94d99952f64e543075a417... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
