Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Piping frames to ffmpeg #15

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Conversation

tienthanh1993
Copy link

@tienthanh1993 tienthanh1993 commented May 13, 2023

Summary

This pull request updates a broken link and rewrites some code to make it compatible with piping frames.

Changes

  • Updated the broken link in the README file.
  • Rewrite some code to make it compatible with piping frames. This means that images can now be piped directly to ffmpeg without being converted to PNG files and written to disk (This change requires a lot of changes to RenderProcessor, but there may be a different way to achieve this compatibility. I am new to Dart, so I am not sure what the best approach is).

Testing

I have tested these changes locally and they appear to work as expected. Please let me know if you have any questions or concerns.

Thanks,

Thanh Pham

This is just a prototype, but I hope it helps you save some time!
Feel free to change!

Copy link
Owner

@polarby polarby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your efforts! Great work, we are getting there!

I have added some comments, where I did not understand or it needs a bit of work. Please clarify, and i will make sure to approve the pull asap.

lib/src/process.dart Show resolved Hide resolved
lib/src/service/notifier.dart Show resolved Hide resolved
lib/src/core.dart Show resolved Hide resolved
lib/src/formats/abstract.dart Show resolved Hide resolved
polarby

This comment was marked as duplicate.

polarby

This comment was marked as duplicate.

Copy link
Owner

@polarby polarby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! It looks good now. I will do some check-ups on the device just so we didn't miss something before merging. Thanks a lot for your contribution so far! Any limitations that you have noticed?

@tienthanh1993
Copy link
Author

tienthanh1993 commented May 14, 2023

It looks good to me now. There are some optimizations that can be done with MotionFormat, such as allowing users to set presets and tune to balance between video size and render speed. However, I will handle these changes in another pull request.

@polarby polarby self-assigned this May 14, 2023
@polarby polarby added enhancement New feature or request render processing issues that involve post capturing processing labels May 14, 2023
@polarby
Copy link
Owner

polarby commented May 14, 2023

(I think) I have improved some things:

  • Removing the storage of each ui.Image as it is no longer needed
    final List<ui.Image> _unhandledCaptures = [];
  • Removing simultaneousCaptureHandlers now seems to be obsolete, as piping can be simultaneously

I couldn't help but notice, that the piping process sometimes (almost randomly) takes significantly longer than storing each frame - the old process approach. Although piping should be the better way, I cannot see a major improvement...?! For web on the other hand this feature might be essential?!
I am not too sure if it is good to merge this for the whole plugin, as although the process is now cleaner, it takes longer most of the time. Let's discuss it! How can we improve this?! You said you noticed this too, right?

This was linked to issues May 14, 2023
@tienthanh1993
Copy link
Author

Yes, there was no major improvement because it's a trade-off between capturing and handling images in parallel, and then batch processing them at the final stage (old approach). On the other hand, the new approach involves piping images in a queue, converting them, and processing them simultaneously in ffmpeg. I believe that for short durations, there may not be a significant impact, but for recording over a long period, it can help save memory, storage, and time as well.

Regarding the web, I have conducted some research but have not yet found any way to implement piping.

@polarby
Copy link
Owner

polarby commented Jun 10, 2023

Let's keep this in mind though! Although web such as using ffmpeg_wasm might not require piping, long-period rendering might require this. If we could separate normal capturing under let's say 5-10min and longer period capturing + recording, this reasonable and working solution may be merged.

@polarby polarby mentioned this pull request Jul 7, 2023
@polarby
Copy link
Owner

polarby commented Jul 7, 2023

@tienthanh1993 Thinking about giving your piping contribution a new chance in life. Check out #6

@marvin-kolja
Copy link

@polarby I wanted to check if piping resolves some RAM issues I have on older devices such as the iPhone 8. I cannot get this PR to run, though. For some reason, the closing of the IOSink (https://github.com/polarby/render/pull/15/files#diff-e4e775396876f43bd88ff1f2de5bfced24f4d39a9b582f880231095ee5f68a25R317) results in very weird behaviour. Nothing after that referenced line is executed. It doesn't throw an error or whatsoever. Can you check if this works for you?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request render processing issues that involve post capturing processing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Web support Optimizing render time: Piping frames to ffmpeg
3 participants