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

Temporary remove FileWriter support when exporting video #516

Merged

Conversation

McGiverGim
Copy link
Member

Workaround for #492

Since some update of our code (I suppose some new Node version or similar), the FileWriter constructor.name does not contain the FileWriter value anymore, now it contains EventTarget. This produces the behaviour of the issue.

I've opened an issue in the library to see if we can get a real fix for that: thenickdude/webm-writer-js#31 because it is a good idea to stream to disk directly and not to memory.

But until then, this PR removes the FileWriter support and uses the "on memory" stream of the video.

@McGiverGim McGiverGim force-pushed the fix_double_dialog_export_video branch from 921d92b to ad125c6 Compare June 3, 2021 09:53
@sonarcloud
Copy link

sonarcloud bot commented Jun 3, 2021

SonarCloud Quality Gate failed.

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell C 2 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@meoso
Copy link

meoso commented Jun 3, 2021

i tested and it technically works, but very weird to ask for filename after processing, IMHO.

@McGiverGim
Copy link
Member Author

@meoso true, I prefer to select the file first, and in this way use less memory. But the problem is outside our code and not depends of us. We can try to migrate to a different library, but I don't know if there are other options out there.

@haslinghuis
Copy link
Member

@github-actions
Copy link

github-actions bot commented Jul 7, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs within a week.

@meoso
Copy link

meoso commented Jul 7, 2021

I presume y'all don't want to go backwards, but version 0.48.4 is the last version that works as expected.
Could potentially use differing NWjs based on OS.

e.g.

    var NWversion;
    switch (os.platform()) {
        case 'linux':
            NWversion ='0.48.4';
            break;
        default:
            NWversion ='0.50.2';
    }

@McGiverGim
Copy link
Member Author

It does not only fail under Linux. Windows has the same problem.

@github-actions github-actions bot removed the Inactive label Jul 8, 2021
Copy link
Member

@ctzsnooze ctzsnooze left a comment

Choose a reason for hiding this comment

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

Approved

@blckmn
Copy link
Member

blckmn commented Sep 3, 2021

AUTOMERGE: (FAIL)

  • github identifies PR as mergeable -> FAIL
  • assigned to a milestone -> PASS
  • cooling off period lapsed -> PASS
  • commit count less or equal to three -> PASS
  • Don't merge label NOT found -> PASS
  • at least one RN: label found -> PASS
  • Tested label found -> PASS
  • assigned to an approver -> PASS
  • approver count at least three -> FAIL

@ctzsnooze
Copy link
Member

@McGiverGim - I have lost track of whether this is needed still?

@McGiverGim
Copy link
Member Author

Yes it is. I've a suggestion of code to test to see if it fixes the real problem, maybe this week I can get some time for it.

@haslinghuis haslinghuis added this to For discussion in Finalizing Firmware 4.3 Release via automation Jan 10, 2022
@haslinghuis haslinghuis moved this from For discussion to Blackbox in Finalizing Firmware 4.3 Release Jan 10, 2022
@haslinghuis haslinghuis merged commit 798f204 into betaflight:master Jan 12, 2022
Finalizing Firmware 4.3 Release automation moved this from Blackbox to Finished (Merged) Jan 12, 2022
@McGiverGim
Copy link
Member Author

This must be reverted. It was an alternate solution. If we merged the other this must be reverted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants