Max video file size #104

chaitanyaraghav · 2017-11-15T17:16:43Z

Hello,

I had a need to set a max file size limit for my implementation of the camera and added the API to set a limit. I added a new API method setMaxFileSize(long maxFileSizeInBytes) to set a custom maximum file size for a video recording. The camera ignores the max file size if its not set or is invalid.

I added another boolean parameter to the video callback method indicating if the video ended due to a file size limit. I also included a helper getter method to get the status of the current camera video recording. It felt easier as I could rely on cameraView.isRecordingVideo() instead of having my own boolean tracking in my activity.

Thanks!

natario1 · 2017-11-15T17:28:56Z

Nice job, thanks!

I'll take a look now. But we can't change the signature of the listener. That would force everyone to rewrite their code. So I think you should revert that, for now the user can manually check the video size. The rest seems like cool additions!

codecov · 2017-11-15T17:31:33Z

Codecov Report

Merging #104 into master will decrease coverage by 0.3%.
The diff coverage is 11.7%.

Impacted Files	Coverage Δ	Complexity Δ
...ain/java/com/otaliastudios/cameraview/Camera2.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...ain/java/com/otaliastudios/cameraview/Camera1.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
.../java/com/otaliastudios/cameraview/CameraView.java	`67.2% <16.6%> (-0.6%)`	`104 <0> (ø)`
...com/otaliastudios/cameraview/CameraController.java	`25.9% <50%> (+0.2%)`	`23 <0> (ø)`	⬇️
...otaliastudios/cameraview/TextureCameraPreview.java	`65.2% <0%> (-4.4%)`	`4% <0%> (-1%)`

natario1 · 2017-11-15T17:32:10Z

cameraview/src/main/java/com/otaliastudios/cameraview/Camera1.java

+                    switch (i){
+                        case MediaRecorder.MEDIA_RECORDER_INFO_MAX_FILESIZE_REACHED:{
+                            mHasReachedMaxSize = true;
+                            endVideo();


I think endVideoImmediately would work better.

natario1 · 2017-11-15T17:33:18Z

cameraview/src/main/java/com/otaliastudios/cameraview/Camera1.java

+    // Additional helper info
+
+    @Override
+    boolean isRecordingVideo() {


can you move this to CameraController and make it final? So Camera1 just inherits.

natario1 · 2017-11-15T17:34:35Z

cameraview/src/main/java/com/otaliastudios/cameraview/CameraListener.java

     */
    @UiThread
-    public void onVideoTaken(File video) {
+    public void onVideoTaken(File video, boolean hasReachedMaxFileSize) {


As said, I think this is unacceptable at the moment. We can't force everyone to rewrite their code. But in the future I will change the signature with a VideoResult so we can add everything.

natario1 · 2017-11-15T17:35:30Z

cameraview/src/main/java/com/otaliastudios/cameraview/CameraView.java

@@ -1346,7 +1364,7 @@ public boolean getPlaySounds() {
        void onShutter(boolean shouldPlaySound);
        void processImage(byte[] jpeg, boolean consistentWithView, boolean flipHorizontally);
        void processSnapshot(YuvImage image, boolean consistentWithView, boolean flipHorizontally);
-        void dispatchOnVideoTaken(File file);
+        void dispatchOnVideoTaken(File file, boolean hasReachedMaxFileSize);


As above, we can't do this right now.

natario1 · 2017-11-15T17:36:05Z

cameraview/src/main/java/com/otaliastudios/cameraview/CameraView.java

+     *
+     * @param maxFileSizeInBytes The maximum size of videos in bytes
+     */
+    public void setMaxFileSize(long maxFileSizeInBytes){


Can you rename to setVideoMaxSize?

chaitanyaraghav · 2017-11-15T17:38:01Z

@natario1 Ya I agree. I was on the fence about that and did not quite like changing the signature. How about adding a new callback method to notify file size limits? That way only people requiring a notification can override it.

natario1 · 2017-11-15T17:59:02Z

That would be better but I prefer not doing it. You know, there is a lot of video metadata which would be cool to pass around, but we can't have a callback for everything only to deprecate them later.

The good solution is really to change the signature as you did with a VideoResult object but I can only do that in a major release like v2. So, please be patient. You can easily call file.length() for now.

chaitanyaraghav · 2017-11-15T18:02:52Z

@natario1 Ya makes sense to me. I reverted the changes to the method signature. Please take a look!
I'll stick with file.length() for now :)

natario1 · 2017-11-15T18:28:04Z

Looks good to me. I will add tests for this later. Two things,

can you add a cameraVideoMaxSize XML attribute? It's just a few lines in the view constructor
can you add some info in README.md? You can use the Camera Controls table where all other attrs are.

If you don't want to, no problem.

natario1 · 2017-11-15T18:51:32Z

And, since all video API use capturing, can you rename isRecordingVideo to isCapturingVideo? Thanks!

chaitanyaraghav · 2017-11-15T19:09:03Z

@natario1 Sure, I'll make the changes

Johnson145 · 2017-11-16T11:15:09Z

I'm afraid this feature is a bit problematic, isn't it? I was thinking about implementing the same for the setMaxDuration part to get a constant video length as requested in #65. However, according to the docs both events will result in: Stopping happens asynchronously, there is no guarantee that the recorder will have stopped by the time the listener is notified. So we may inform the user about a captured video which isn't available yet, right? Furthermore, your current implementation stops the video a second time, although the media recorder itself stopped it already. This could cause troubles, too, couldn't it?

Johnson145 · 2017-11-16T11:34:26Z

A bit hacky, but maybe you could listen to the MEDIA_RECORDER_INFO_MAX_FILESIZE_APPROACHING event instead? Then we were able to keep control about stopping the video. You may internally scale the max file size that is given by the user such that the MEDIA_RECORDER_INFO_MAX_FILESIZE_APPROACHING event is triggered after 90% of the scaled size has reached, but 100% of the unscaled one.

natario1 · 2017-11-16T11:37:23Z

@Johnson145 I have not tried of course but I think the two issues solve each other.

When we get the callback the recorder is about to auto-stop. We anticipate and stop it, then notify the user. If it stops itself before we do, there should be no problem because our stopping is in a try-catch block.

Does this make sense to you?

The best thing would be to try but I'm not able right now. Maybe after it's merged. If it proves to be flaky, we can do the trick with 90%, seems good! Or @chaitanyaraghav can implement it now if he wants. Just be sure to add some code comments about it.

These APIs do really suck.

Johnson145 · 2017-11-16T11:50:56Z

May work, I don't know. Depends on the question whether the mMediaRecorder.stop() call is thread safe. If one of the two calls is guaranteed to finish its job, you're right. I'm afraid it could be possible that our second call is failing, because the first one is still running (but not yet finished). In that case we would just continue after the try-catch and inform the user about the video to early. In each case, I guess problems are very unlikely, but who knows whether they are impossible..

Sure, we could try it first!

Johnson145 · 2017-11-16T11:53:42Z

Maybe it's even a good idea to give it a try first. If it proves to work, we could implement the MEDIA_RECORDER_INFO_MAX_DURATION_REACHED part, too. Unfortunately, the latter does not have a MEDIA_RECORDER_INFO_MAX_DURATION_APPROACHING version.

natario1 · 2017-11-16T12:00:45Z

Yes, given how unfriendly MediaRecorder is, issues might be likely. The max duration feature would be cool as well if you want to work on it! We could then deprecate startCapturingVideo(File, long).

natario1 · 2017-11-22T10:54:09Z

@chaitanyaraghav are you planning to work on this soon? There's no hurry of course, I want to know if I should release 1.4.1 today or wait for you. This Sunday I'll leave for a while so it will be hard for me to add tests etc. Thanks so much!

chaitanyaraghav · 2017-11-22T17:46:42Z

@natario1 It may be better to push this to the next release. I'm currently out of town and I won't be able to make the changes until I get back this Sunday.

natario1 · 2017-11-22T17:57:47Z

That's fine man 👍 I'll release today then!

…updated the documentation

chaitanyaraghav · 2017-11-28T15:32:25Z

@natario1 Changed the method name, added xml attribute for video size and updated the documentation.

The 90% trick seems good but I have not made that change yet. The current implementation appears to work fine in my testing. If it turns out to be flaky, I'll make the change to scale the max size.

Thanks for the patience!

natario1 · 2017-12-03T22:14:17Z

Thanks!

chaitanyaraghav added 2 commits November 15, 2017 22:01

Added a maximum file size for videos

cd715cf

Merge branch 'master' into max_video_file_size

25092d4

natario1 reviewed Nov 15, 2017

View reviewed changes

Revereted changes to the onVideoTaken callback signature

402b72c

Renamed isRecordingVideo, added xml attribute for video max size and …

d293375

…updated the documentation

Merge branch 'master' into max_video_file_size

8414afb

natario1 merged commit 1bd1816 into natario1:master Dec 3, 2017

lancewoo mentioned this pull request Dec 25, 2021

SIGABRT encountered on writing an invalid file descriptor #1157

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Max video file size #104

Max video file size #104

chaitanyaraghav commented Nov 15, 2017

natario1 commented Nov 15, 2017

codecov bot commented Nov 15, 2017 •

edited

Loading

natario1 Nov 15, 2017

natario1 Nov 15, 2017 •

edited

Loading

natario1 Nov 15, 2017

natario1 Nov 15, 2017

natario1 Nov 15, 2017

chaitanyaraghav commented Nov 15, 2017

natario1 commented Nov 15, 2017

chaitanyaraghav commented Nov 15, 2017

natario1 commented Nov 15, 2017

natario1 commented Nov 15, 2017

chaitanyaraghav commented Nov 15, 2017

Johnson145 commented Nov 16, 2017

Johnson145 commented Nov 16, 2017

natario1 commented Nov 16, 2017 •

edited

Loading

Johnson145 commented Nov 16, 2017

Johnson145 commented Nov 16, 2017 •

edited

Loading

natario1 commented Nov 16, 2017

natario1 commented Nov 22, 2017

chaitanyaraghav commented Nov 22, 2017

natario1 commented Nov 22, 2017

chaitanyaraghav commented Nov 28, 2017

natario1 commented Dec 3, 2017

Max video file size #104

Max video file size #104

Conversation

chaitanyaraghav commented Nov 15, 2017

natario1 commented Nov 15, 2017

codecov bot commented Nov 15, 2017 • edited Loading

Codecov Report

natario1 Nov 15, 2017

Choose a reason for hiding this comment

natario1 Nov 15, 2017 • edited Loading

Choose a reason for hiding this comment

natario1 Nov 15, 2017

Choose a reason for hiding this comment

natario1 Nov 15, 2017

Choose a reason for hiding this comment

natario1 Nov 15, 2017

Choose a reason for hiding this comment

chaitanyaraghav commented Nov 15, 2017

natario1 commented Nov 15, 2017

chaitanyaraghav commented Nov 15, 2017

natario1 commented Nov 15, 2017

natario1 commented Nov 15, 2017

chaitanyaraghav commented Nov 15, 2017

Johnson145 commented Nov 16, 2017

Johnson145 commented Nov 16, 2017

natario1 commented Nov 16, 2017 • edited Loading

Johnson145 commented Nov 16, 2017

Johnson145 commented Nov 16, 2017 • edited Loading

natario1 commented Nov 16, 2017

natario1 commented Nov 22, 2017

chaitanyaraghav commented Nov 22, 2017

natario1 commented Nov 22, 2017

chaitanyaraghav commented Nov 28, 2017

natario1 commented Dec 3, 2017

codecov bot commented Nov 15, 2017 •

edited

Loading

natario1 Nov 15, 2017 •

edited

Loading

natario1 commented Nov 16, 2017 •

edited

Loading

Johnson145 commented Nov 16, 2017 •

edited

Loading