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

[Feature] Ability to extract files simultaneously #12

Merged
merged 11 commits into from
Dec 30, 2022

Conversation

AlexSmirnov9107
Copy link
Contributor

Fix or Feature to #10

@ryanheise
Copy link
Owner

Going forward, I wonder whether it would be better to generate a unique ID for each instance rather than use the file name. The plan is to eventually include partial results of the waveform data in the progress so that eventually you might not need the file.

@AlexSmirnov9107
Copy link
Contributor Author

Ok, will change to ID

@ryanheise
Copy link
Owner

Before you do that, I haven't actually thought it through completely, just thinking out aloud. This may be like a job ID.

@@ -121,7 +121,7 @@ - (void)handleMethodCall:(FlutterMethodCall*)call result:(FlutterResult)result {
UInt32 scaledSampleIdx = 0;
int progress = 0;

while (frameCount > 0) {
while (frameCount > 0 && progress < 100) {
Copy link
Owner

Choose a reason for hiding this comment

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

Just curious, what is the scenario where the second condition is required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sometimes the progress never become 100, kind of rare times. may be frame count cause the problem. Its difficult to find

Copy link
Owner

Choose a reason for hiding this comment

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

Your condition wouldn't help if the progress never became 100. Maybe you meant that frameCount never becomes 0?

@Faaatman
Copy link

This looks great! thank you guys for the great work.
@ryanheise if you think this is resource-intensive what do you think about adding a max number of concurrent jobs so that people could limit the number of files being extracted?
if this PR doesn't get merged, do you think we can at least throw an error if a job tries to run while another one is running?
I myself will be implementing a queue interface but I think an exception thrown is much needed.
I will be happy to help. :)

@ryanheise
Copy link
Owner

@ryanheise if you think this is resource-intensive what do you think about adding a max number of concurrent jobs so that people could limit the number of files being extracted?

That is something that can/should be implemented by the app. (I like to keep things simple)

if this PR doesn't get merged, do you think we can at least throw an error if a job tries to run while another one is running? I myself will be implementing a queue interface but I think an exception thrown is much needed. I will be happy to help. :)

At this stage my plan is to eventually merge this PR.

Before I do that, I'm just thinking about whether we want to have a unique ID for each instance generated by uuid or just use the file path as the unique ID. (Again just thinking of simplicity.)

@Faaatman
Copy link

Before I do that, I'm just thinking about whether we want to have a unique ID for each instance generated by uuid or just use the file path as the unique ID. (Again just thinking of simplicity.)

I think we should avoid the path because of duplication. If the same widget is rendered twice this would cause an error. Where using uuid even if there are duplicates the waveform would be extracted.

@ryanheise
Copy link
Owner

I would consider it a misuse of the plugin to simultaneously write to the same path, so the path should be unique among the simultaneous requests.

@Faaatman
Copy link

I would consider it a misuse of the plugin to simultaneously write to the same path, so the path should be unique among the simultaneous requests.

It is a misuse, but I still think UUID would be the better approach since it's not prone to as many problems as the path approach down the line. And it would stop people from opening issues when they misuse the plugin, instead, it would just make their apps a little bit slower.

@ryanheise
Copy link
Owner

This problem is not a a real problem since it can be documented and an exception can be thrown if the same output file is being used in two simultaneous requests. You mentioned there are more problems, though?

@ryanheise
Copy link
Owner

I'll add to the above that using a UUID does not actually solve the problem of using the same output file in simultaneous requests - an exception would be needed.

@ryanheise ryanheise merged commit 64e5dab into ryanheise:master Dec 30, 2022
@ryanheise
Copy link
Owner

This PR has now been merged and released, and I have done some minor cleanup and removed the uuid key in favour of the output filename. Thanks, @AlexSmirnov9107 for the contribution.

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.

3 participants