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

FileWriter constructor name is not FileWriter #31

Open
McGiverGim opened this issue Jun 3, 2021 · 16 comments · May be fixed by guest271314/webm-writer-js#5 or #35
Open

FileWriter constructor name is not FileWriter #31

McGiverGim opened this issue Jun 3, 2021 · 16 comments · May be fixed by guest271314/webm-writer-js#5 or #35

Comments

@McGiverGim
Copy link

Hi! The Betaflight Blackbox Explorer has a little bug since some time ago with your library. For some reason, this was working:

if (destination && destination.constructor.name === "FileWriter") {

But since some update of something (Node.js, probably), the value of destination.constructor.name is EventTarget and not FileWriter anymore. If I force this if to be true, all works perfectly, so the FileWriter is valid, but for some reason the name is not valid anymore to verify the FileWriter.
Is there some possibility of patching your library changing this verification? Thanks!

@guest271314
Copy link
Contributor

What is the result of substituting instanceof for destination.constructor.name?

if (destination && "FileWriter" in globalThis && destination instanceof FileWriter) {

@McGiverGim
Copy link
Author

McGiverGim commented Jun 29, 2021

It seems FileWriter does not exist...

(destination && destination instanceof FileWriter)
13:05:39.287 VM348:1 Uncaught ReferenceError: FileWriter is not defined
    at eval (eval at <anonymous> (BlobBuffer.js:23), <anonymous>:1:40)
    at new <anonymous> (BlobBuffer.js:23)
    at new <anonymous> (WebMWriter.js:331)
    at flightlog_video_renderer.js:259

EDIT: But of course destination is a FileWriter:

destination
13:07:34.455 FileWriter {readyState: 2, error: null, position: 0, length: 0, onwritestart: null, …}

EDIT again: More:

destination.toString()
13:09:26.481 "[object FileWriter]"

@guest271314
Copy link
Contributor

FileWriter is an instance of chrome.fileSystem, correct?

Note, Chrome Apps are slated for deprecation, see https://developer.chrome.com/docs/extensions/reference/fileSystem/.

@McGiverGim
Copy link
Author

Betaflight is not more a Chrome app. Is a Node.js app, and it's API is compatible with the Chrome app.
As I said, this changed at some version.
Can it only verify destination and suppose that the user has assigned the correct variable?

@guest271314
Copy link
Contributor

I have not tried to suuply my own FileWriter or pass Node.js fd file descriptor when using webm-writer-js. I am not sure what is being passed by the user. Can you post an example in code of what you are doing?

@McGiverGim
Copy link
Author

@guest271314
Copy link
Contributor

I will look into the code more later. Since you know that you are in fact passing a FileWriter instance you should be able to adjust the if condition to suit the current implementation of your application.

@McGiverGim
Copy link
Author

Yes, I know I can change the code to suit my implementation, I only need to change:

if (destination && destination.constructor.name === "FileWriter") { 

by:

if (destination) { 

But I use the library using npm, so what I want is some kind of fix "directly" in the library. I don't know if removing the second condition can be acceptable or not.

@guest271314
Copy link
Contributor

You can test the local implementation then if that works without side effects you can submit a PR to this repository.

@guest271314
Copy link
Contributor

As mentioned in #31 (comment) "fileSystem" permission is only available in packaged apps, however Chrome packaged apps are slated for deprecation

'fileSystem' is only allowed for packaged apps, but this is a extension.

https://developer.chrome.com/docs/extensions/reference/fileSystem/

chrome.fileSystem
This API is part of the deprecated Chrome Apps platform. Learn more about migrating your app.

Since you are using Node.js have you tried substituting using fd for fileWriter?

An alternative solution in the browser include using File System Access API https://wicg.github.io/file-system-access/.

@McGiverGim
Copy link
Author

McGiverGim commented Jul 2, 2021

Is not as easy... We use Cordova too for Android so it must be compatible with both. Until now it was.

EDIT: Sorry, we only use Cordova for the Configurator, the Blackbox does not use it.

@guest271314
Copy link
Contributor

If internally all that you use is FileWriter then you should be able to use

if (destination) { 

Technically, you can use only a Blob, e.g.,

let blob = new Blob([], {type:'video:/webm'});
// write to blob ...
blob = new Blob([blob, chunks], {type:blob.type});

@guest271314
Copy link
Contributor

@McGiverGim

Betaflight is not more a Chrome app. Is a Node.js app, and it's API is compatible with the Chrome app.

Chrome apps are deprecated, see https://developer.chrome.com/docs/extensions/reference/fileSystem/

This API is part of the deprecated Chrome Apps platform. Learn more about migrating your app.

Following the "migrating your app link" to chrome.fileSystem

chrome.fileSystemNative FileSystem API

Is not as easy... We use Cordova too for Android so it must be compatible with both. Until now it was.

I have not tried Cordova for Android. If that framework internally uses Chrome extension API's File System Access should be exposed to utilize [createWritable()](https://wicg.github.io/file-system-access/#api-filesystemfilehandle-createwritable), if not then --enable-experimental-web-platform-features can be passed as an option to the Chrome instance to enable the API.

I tested substituting passing an instance of WritableStreamDefaultWriter for FileWriter in webm-writer.js on Chromium 93.0.4569.0 on Linux at https://plnkr.co/edit/2Lm3mUbMfKcCnYbe?preview. To run click "Open the previw in a separate window" on the horizontal panel aboe the rendered HTML to avoid

DOMException: Failed to execute 'showSaveFilePicker' on 'Window': Cross origin sub frames aren't allowed to show a file picker.

When you have the time can you kindly test the code in your application and post result here?

webm-writer-js-file-system-access.zip

@guest271314
Copy link
Contributor

@McGiverGim Have you tested the substitution of File System Access API for FileWriter in your code base?

@McGiverGim
Copy link
Author

No sorry, I'm out on holidays, until I return I can't test nothing.
I'm not too sure neither what you want that I test. Add the files you attach in my app and test it? Modify my code to do something similar to what you did?
I suppose you want that I replace when the app asks the user for a file to be writed, by this code of yours, am I right?

fileHandle = await showSaveFilePicker({
...

@guest271314
Copy link
Contributor

I'm not too sure neither what you want that I test.

I included the entire webm-writer.js code with File System Access API substituted for chrome.fileSystem code in the .zip file and at the linked plnkr. The code achieved the expected result here.

At the user side

document.querySelector('p').onclick = async () => {
        fileHandle = await showSaveFilePicker({
          suggestedName: 'webm-writer-filesystem-access.webm',
          startIn: 'videos',
          id: 'webm-writer',
          types: [
            {
              description: 'WebM files',
              accept: {
                'video/webm': ['.webm'],
              },
            },
          ],
          excludeAcceptAllOption: true,
        });
        writable = await fileHandle.createWritable();
        fileWriter = await writable.getWriter();
        videoWriter = new WebMWriter({
          quality: 0.95, // WebM image quality from 0.0 (worst) to 1.0 (best)
          fileWriter, // FileWriter in order to stream to a file instead of buffering to memory (optional)
          fd: null, // Node.js file handle to write to instead of buffering to memory (optional)
          // You must supply one of:
          frameDuration: null, // Duration of frames in milliseconds
          frameRate: 30, // Number of frames per second
          // add support for variable resolution, variable frame duration, data URL representation of WebP input
          variableResolution: true, // frameRate is not used for variable resolution
        });
        // ...
        await videoWriter.complete();
        await fileWriter.close();
        await fileWriter.closed;
});

Since Chrome Apps are deprecated (including chrome.fileSystem) this repository will need to be updated to replace FileWriter.

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