RedApparat / Fotoapparat

Making Camera for Android more friendly. 📸
Apache License 2.0
3.82k stars 405 forks source link

Frame processor produces frames with tearing #200

Closed Raenar4k closed 6 years ago

Raenar4k commented 6 years ago

Frames from frame processor have tearing when there are movement: https://i.imgur.com/p0JtMBT.jpg

If instead Camera1 .setOneShotPreviewCallback is used then resulting frames will be without any tearing: https://i.imgur.com/3n0ycr5.jpg

I assume tearing comes from the fact that the frames are using same buffer, so there isn't much that can be done about it. However, if oneShotPreviewCallback is used, there is no tearing at the cost of some short, but visible lag.

Is there any other way to get frame from preview without tearing? If it is not possible otherwise - i have submitted a PR https://github.com/Fotoapparat/Fotoapparat/pull/199 that add method to set this callback.

It could be helpful for others that need to process frames without frame tearing.

Diolor commented 6 years ago

Hey, so we discussed the topic here. So first of all the FrameProcessor runs in a background thread, we do that for you. The bytearray in the Frame object is also mutable. We speculate that you try to directly write the bytearray to a file. However this operation takes more time than the next frame comes. That will make parts of the file being written with different frames.

If you want to save it, you should copy the bytearray first. We don't do that by default because this will slow operations for many users

Raenar4k commented 6 years ago

Yep, that was exactly it - thanks for making that clear! I feel stupid now for missing such an obvious thing.

/**
 * Performs processing on preview frames.
 *
 * Frame processors are called from worker thread (aka non-UI thread). After
 * [.processFrame] completes the frame is returned back to the pool where it is reused
 * afterwards. This means that implementations should take special care to not do any operations on
 * frame after method completes.
 */

I assumed it meant not to modify frame itself, but didn't think about the fact reused frame will have same ByteArray object instead of new instance. I guess a little note near Frame.image field about its mutable nature might help others in such case.

        /**
         * Image in NV21 format.
         */
        val image: ByteArray,

Although i do feel a bit dumb for missing it. Thanks for quick response! :)

Raenar4k commented 6 years ago

As for oneShotPreviewCallback - there are concerns about it that i considered:

1) Right now using this callback produces visible lag - not sure yet if it can be mitigated with proper threading or is it implemented that way in Camera1 2) Setting it overrides other previewCallback inside Camera1 - which probably overrides frame processor if it was set (which i don't think is a problem since it probably will be used instead of frame processor)

It is different from frame processor in the way that callback is cleared after one invocation, so it could be useful for some use cases. Apart from lag, i don't see any difference in quality of frames - in my use case i don't see need in clearing callback, so i could use frame processor instead.

So there is a question - will such method for one shot callback be helpful to others?

Diolor commented 6 years ago

Although image is a final object ByteArray's content itself it's not. We can add a reminder in the image docs that you need to copy it to ensure immutability of it's content.

Regarding the oneShotPreviewCallback I think the existing logic will work fine even if you need just one frame. We don't need to add extra logic which is not helpful. So if you are ok with it, I will close the PR; but of course thanks for submitting it.

Raenar4k commented 6 years ago

Yep, i think it is better not to add such method at this moment. A reminder could prove to be helpful in the future :)

As for title of this issue maybe we can change it to "Frame processor produces frames with tearing" or something like that, so it would be easier for someone to find this while searching for this issue?

And again, big thanks to you guys for working for this lib! Awesome to see many improvements and issues resolved!

Diolor commented 6 years ago

Thanks for the recommendation @Raenar4k, I changed the title and will add the kdoc first chance given. Also thanks for your nice words!

brownpixel commented 6 years ago

@Raenar4k @Diolor I'm still seeing some tear even though I'm making a copy of the Frame object itself. any reason why thats the case? Is there some sample code I can look at where you do some work on the frame processing? In the example nothing is being done in the callback.

Awesome library btw!

dmitry-zaitsev commented 6 years ago

@brownpixel could you share the code which you use to copy the frame? If you cam share the whole FrameProcessor that would be just perfect.