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

Alternative Recording Mode Using Local Recording #532

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

senocak
Copy link

@senocak senocak commented Dec 8, 2023

Summary:

Using local-recording feature instead of ffmpeg to reduce memory usage size to have more instance of jibri. It is tested in our environment and works better than ffmpeg.

Issue(s) addressed:

"N/A"

Testing:

Tested in our environment for performance for a long time, it is working great as expected. I did not write any unit or integration test

Checklist:

[x] I have reviewed the code and tested it thoroughly.
[x] I have followed the coding style guide.
[ ] I have updated the documentation to reflect the changes.
[ ] I have added unit tests for the new code.
[x] I have run the build and test scripts.

Additional notes:

  • --use-fake-device-for-media-stream flag should be added in docker-jitsi-meet application since it is configured to have the data as user input. Also frontend side should be changed reflectively with this release.
  • There is no dependency changes
  • Maybe using this feature can be optional along with ffmpeg

Thanks for reviewing! & Hope this helps

The community ticket: https://community.jitsi.org/t/new-recording-implementation-using-local-recording-instead-of-ffmpeg/128652

@jitsi-jenkins
Copy link

Hi, thanks for your contribution!
If you haven't already done so, could you please make sure you sign our CLA (https://jitsi.org/icla for individuals and https://jitsi.org/ccla for corporations)? We would unfortunately be unable to merge your patch unless we have that piece :(.

@saghul
Copy link
Member

saghul commented Dec 8, 2023

HI there! Thanks for taking the time to put together this PR! This is very intriguing. Some questions:

  1. You say you are reducing memory usage. Can you share what levels you had befor and after?

  2. How are you dealing with long recordings? We have a limit and recording will stop automatically.

  3. In this mode if the browser crashes you get nothing, isn't that a problem for you?

An observation: replacing our current model is not something we can accept. If (and it's a big if, not going to lie) we accept this work it would need to be configurable, and off by default).

@senocak
Copy link
Author

senocak commented Dec 8, 2023

Hi @saghul, thank you for the reply, so the answers;

  1. We had total 25 instances with ffpmeg recording before, using local-recording we've seen 40 at the same time,
  2. So frontend already limited to 1GB(approximately 90/100minutes) so we increased the limit 2GB and we allow users to reecord max 4 hours long
  3. Yes thats the main concerns we have, Tried to explain in the community post with some other problems as well.

:) I understand replacement is a big deal but your suggestion is the same we have now, making it optional

@saghul
Copy link
Member

saghul commented Dec 8, 2023

  1. We had total 25 instances with ffpmeg recording before, using local-recording we've seen 40 at the same time,

Not sure what this means. You mean you can fit more Jibri instances per server?

2. So frontend already limited to 1GB(approximately 90/100minutes) so we increased the limit 2GB and we allow users to reecord max 4 hours long

This is the part I find most puzzing. You said memory usage was a concern, but now the entire recording is stored on RAM vs a small fraction when using ffmpeg. How is that possible? What recording speeds did you observe with ffmpeg? Did you tweak the command line arguments?

I see several additional problems with that approach:

  • There is a limit in recording time, there isn't one now
  • It requires a change in Jitsi Meet as that is not configurable (maybe it should?)
  • Increased RAM usage
  • Loss of data in case of a browser crash

3. Yes thats the main concerns we have, Tried to explain in the community post with some other problems as well.

Oh, I have missed the link, sorry!

:) I understand replacement is a big deal but your suggestion is the same we have now, making it optional

I think this approach weakens the overall recordings mechanism. If the main reason why you did this was to lower memory usage I don't see how that can happen since ffmpeg does not buffer the entire recording. The bug might be elsewhere like the storage being too slow for example.

@emrahcom
Copy link
Contributor

emrahcom commented Dec 8, 2023

Thank you for your effort.

According to my observations, the main problem with ffmpeg is the CPU usage, not RAM... RAM usage increases only when there is not enough CPU power and ffmpeg starts to use buffers more aggressively.

@saghul
Copy link
Member

saghul commented Dec 8, 2023

Thank you for your effort.

According to my observations, the main problem with ffmpeg is the CPU usage, not RAM... RAM usage increases only when there is not enough CPU power and ffmpeg starts to use buffers more aggressively.

Yes, this is what we see too.

@senocak senocak changed the title Replacing ffpmeg capturer with local recording Alternative Recording Mode Using Local Recording Dec 15, 2023
@senocak
Copy link
Author

senocak commented Dec 15, 2023

Hello again @emrahcom @saghul,
I did changed the code with optional "mode" parameter from docker-jitsi-meet project. Default is "ffmpeg" and no behaviour changed.
Here is the pr;
https://github.com/jitsi/docker-jitsi-meet/pull/1671

@saghul
Copy link
Member

saghul commented Dec 15, 2023

I'm still not convinced this is a good idea to do. Let's hear some more voices, @bgrozev thoughts?

@Freddie-m
Copy link

The question in my mind is: how does this differ significantly from the current Local Recording other than creating the additional burden of a separate server? I think the improvements that would make sense to the current model of serverside recording at the very least, would include:

  • Requirement for less CPU demand
  • Ability to run multiple concurrent sessions

If these two are not being addressed, I'm not sure there's any process/efficiency gain.

@jgabarca
Copy link

Alternative implementations of local recording could be explored to avoid buffering the entire recording in memory.

There is FileSystemWritableFileStream.
https://developer.mozilla.org/en-US/docs/Web/API/FileSystemWritableFileStream/write

As well as FileSystemSyncAccessHandle.
https://developer.mozilla.org/en-US/docs/Web/API/FileSystemSyncAccessHandle/write

Both of these APIs allow you to append to a file, multiple buffers at a time.

The FileSystemSyncAccessHandle API has less overhead on each write but is restricted to a file system internal to the browser. So when recording stops the internal file would have to be copied to a user-accessible file with the FileSystemWritableFileStream API.
https://web.dev/articles/origin-private-file-system

Sometime in the future that copy operation could be optimized into a move.
whatwg/fs#114

@saghul
Copy link
Member

saghul commented Dec 21, 2023

Alternative implementations of local recording could be explored to avoid buffering the entire recording in memory.

Using ffmpeg does not requrie that either, so the question remains, why do it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants