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

Init from background #792

Closed

Conversation

deckerst
Copy link
Contributor

@deckerst deckerst commented Aug 5, 2023

Description

Initialize the plugin when attaching to the engine (instead of the activity), so it can now be used from background processes without activity. I believe this fixes #718, #711, #588, #461, #367, #263 (which are currently closed for being stale, not fixed).

Type of Change

  • Bug fix

Checks

  • Changes support all platforms (Android, iOS, Linux, macOS, tvOS)
  • Breaks existing functionality
  • Implementation is completed, not half-done
  • Is there another PR already created for this feature/bug fix

Tests

Use the plugin from a background isolate.

@deckerst deckerst changed the base branch from main to development-flutter August 5, 2023 17:22
@tanersener
Copy link
Collaborator

@deckerst Thanks for creating this PR. We tested the changes but they didn't work for us. Is there a test-application we can use to test it? We have test-applications under the ffmpeg-kit-test repo. Modifying one of those would also work.

@deckerst
Copy link
Contributor Author

Thanks for the feedback. I'll take a look at the test apps from ffmpeg kit.

We tested the changes but they didn't work for us.

Do you mean that it failed to build? Created some regression? Or simply didn't fix the issue as expected?

@tanersener
Copy link
Collaborator

No build issues. Executing commands inside an isolate still doesn't work for us.

@deckerst
Copy link
Contributor Author

@tanersener The goal with arthenica/ffmpeg-kit-test#49 is for you to try the new "Background" tab, comparing running with and without the fix provided here in #792.

Please note that I have only modified test-app-local-dependency and did the bare minimum for demonstration, as I didn't know if you wanted to keep that tab beyond its purpose to support this PR.

@tanersener
Copy link
Collaborator

Thanks @deckerst. Let us test it on our side.

@tanersener
Copy link
Collaborator

tanersener commented Sep 3, 2023

@deckerst Today I had the chance to test this PR with your test-application.

Yes, background tab works fine. However, when we go back to the other tabs e.g. command tab, video tab; they don't work anymore. I see the following error message in them.

W/FlutterJNI(25649): Tried to send a platform message to Flutter, but FlutterJNI was detached from native C++. Could not send. Channel: flutter.arthenica.com/ffmpeg_kit_event. Response ID: 17
W/FlutterJNI(25649): Tried to send a platform message to Flutter, but FlutterJNI was detached from native C++. Could not send. Channel: flutter.arthenica.com/ffmpeg_kit_event. Response ID: 18
W/FlutterJNI(25649): Tried to send a platform message to Flutter, but FlutterJNI was detached from native C++. Could not send. Channel: flutter.arthenica.com/ffmpeg_kit_event. Response ID: 19
W/FlutterJNI(25649): Tried to send a platform message to Flutter, but FlutterJNI was detached from native C++. Could not send. Channel: flutter.arthenica.com/ffmpeg_kit_event. Response ID: 20

Have you seen it in your tests?

@JavierPerezLavadie
Copy link

Hello @tanersener it would be good if this PR could go to the end. Computational operations on ffmpeg are very resource intensive and it is almost impossible to use it correctly with a single thread. Thank you.

@deckerst
Copy link
Contributor Author

deckerst commented Sep 9, 2023

@tanersener I confirm I get the same issue when mixing calls from the activity and from background when using the test app. Strangely, I don't experience this in my own app, where I also mix calls.

That said, in my own app I register handles and do the background plumbing myself, while for the test app I grabbed the package flutter_isolate to do that.

Anyway, I'll take a closer look, but if anybody else sees where I did wrong please feel free to pitch in.

@deckerst
Copy link
Contributor Author

  • at the flutter plugin android level
    Callbacks needed to be updated when the event sink changes.
  • at the flutter initializer level
    Spawning isolates with their own engine somehow was making the activity engine event subscription obsolete. So I updated the initialization code to resubscribe unconditionally. This is light, as all the heavy initialization code is still guarded by the initialized flag. An alternative to that solution was to manually call some kind of reset after being done with a background isolate, but I think that was asking too much from users.

@deckerst
Copy link
Contributor Author

@tanersener does that work for you?

@tanersener
Copy link
Collaborator

@deckerst Flutter plugin v6.0 will be patched again. I'll test the changes as soon as the patch is published.

@tanersener
Copy link
Collaborator

I tested the changes. It seems like the functionality is there.

However, the following lines are printed a lot in the startup. Moreover, I see them each time I execute an ffmpeg-kit command. Do you see them too?

D FFmpegKitFlutterPlugin stopped listening to events.
D FFmpegKitFlutterPlugin com.arthenica.ffmpegkit.flutter.FFmpegKitFlutterPlugin@64e26ae started listening to events on io.flutter.plugin.common.EventChannel$IncomingStreamRequestHandler$EventSinkImplementation@3c9ecd8.

@deckerst
Copy link
Contributor Author

Yes, that's what I explained here:

* at the flutter initializer level
  Spawning isolates with their own engine somehow was making the activity engine event subscription obsolete. So I updated the initialization code to resubscribe unconditionally. This is light, as all the heavy initialization code is still guarded by the `initialized` flag. An alternative to that solution was to manually call some kind of reset after being done with a background isolate, but I think that was asking too much from users.

These log lines are related to the event subscription. The subscription itself is light, so these logs are not a sign of a memory/performance issue. If the existing logs are now too noisy, we could simply remove them, or lower their level to verbose.

@tanersener
Copy link
Collaborator

I'm not worried about the log messages themselves. I'm worried about their meaning. As I said, they are printed every time I call an ffmpeg-kit method. Which means, we are subscribing to events again before calling a method. This is not a good. Additionally, who can guarantee that no events will be lost during unsubscribe/subscribe? What if we run a second command? Remember this is Android only, we will have to implement the same for iOS and macOS before releasing. Do we have another option that doesn't subscribe to events before each method call?

@deckerst
Copy link
Contributor Author

It's all in my comment above.

An alternative to that solution was to manually call some kind of reset after being done with a background isolate, but I think that was asking too much from users.

To be honest, this issue has been fixed for me for ages. I'm happy to stay with my fork. I was willing to give back to this project with this PR, but I can't dedicate more time and effort to it, especially as I don't know the inner workings of isolates and flutter engine. You'll need a smarter person to pick up where I left off.

@tanersener
Copy link
Collaborator

Thanks for your efforts. But we don't want to risk breaking the plugin to add a new feature. Subscribing to events again on each ffmpeg-kit call doesn't look right to me. I can't merge this.

Developing an alternative init/reset method makes more sense to me for this approach. If this initialisation logic is put into that method, then users who want to use the plugin in isolates can manually call that method and use the plugin. And existing users won't be affected from this change.

@Mamena2020
Copy link

Any update of this PR? cause this could be game changer for better performance 🔥. Thank you

Copy link

github-actions bot commented Jan 1, 2024

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days.

Copy link

github-actions bot commented Jan 9, 2024

This PR was closed because it has been stalled for 7 days with no activity.

@github-actions github-actions bot closed this Jan 9, 2024
@fsmdeveloper
Copy link

how can i use your fork version in flutter?

@deckerst
Copy link
Contributor Author

how can i use your fork version in flutter?

Not sure whether you are talking about my fork, but if you do, check this.

@fsmdeveloper
Copy link

fsmdeveloper commented Jan 30, 2024

  ffmpeg_kit_flutter:
    git:
      url: https:/deckerst/ffmpeg-kit.git
      ref: background-lts
      path: flutter/flutter

I've been searching for this solution. I wanted to use FFmpeg in a background service, and the official package didn't work for me. However, your forked version is functioning perfectly in a background service/isolate. Thank you so much..

@mikron123
Copy link

how can i use your fork version in flutter?

Not sure whether you are talking about my fork, but if you do, check this.

How can I make it work with the ffmpeg_kit_flutter_full_gpl version?

@deckerst
Copy link
Contributor Author

deckerst commented Feb 1, 2024

How can I make it work with the ffmpeg_kit_flutter_full_gpl version?

Fork it and change that line. For you I believe it's something like:

implementation 'com.arthenica:ffmpeg-kit-full-gpl:6.0-2.LTS'

@Flajt
Copy link

Flajt commented Feb 15, 2024

Hello,
I'm also currently running with a forked version, which works well for Android, iOS I still have to test (something failing but idk if its due to background work, running in a simulator or something else).

I would like to pick this up, however I can't guarantee that it will be fast since I'm busy at the moment. Also, I'm not very experienced in native code (swift / kotlin).

But I would like to try, maybe we won't be needing these forks afterwards.

If I can get some pointers for what should be done, I would try to get it done.

What I understood was that an init and a reset function would be the way to go, which should deal with some initation. (I'll have to dig into the code to figure out what's going on, I guess)

In terms of platform support, is it a hard requirement to support every platform, or is Android and iOS enough for now?

Cheers

@ShahoodulHassan
Copy link

How can I make it work with the ffmpeg_kit_flutter_full_gpl version?

Fork it and change that line. For you I believe it's something like:

implementation 'com.arthenica:ffmpeg-kit-full-gpl:6.0-2.LTS'

@mikron123 I'm also using ffmpeg_kit_flutter_full_gpl. Have you been able to create a fork as per the above suggestions?

@deckerst deckerst deleted the init-from-background branch May 10, 2024 20:15
@fsmdeveloper
Copy link

fsmdeveloper commented Jun 1, 2024

deckerst I am using your fork version.

ffmpeg_kit_flutter:
git:
url: https:/deckerst/ffmpeg-kit.git
ref: background-lts
path: flutter/flutter

It's working in the foreground service, but if I close the app and then reopen it, I get an error and the FFmpeg process stops.

  error `W/FlutterJNI(16262): Tried to send a platform message to Flutter, but FlutterJNI was detached from native C++. Could not send. Channel: flutter.arthenica.com/ffmpeg_kit_event. Response ID: 529`

can you please fix this issue? or tell me how can I fix that issue?

@deckerst
Copy link
Contributor Author

deckerst commented Jun 1, 2024

@fsmdeveloper sorry I can't dedicate time to fix issues others have with this fork. It works for me, and I suppose you misuse plugins/isolates/engines. In any case, you can check out how I use it in my app, specifically this plugin.

@fsmdeveloper
Copy link

deckerst Ok brother! thank you!

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