thenickdude / webm-writer-js

JavaScript-based WebM video encoder for Google Chrome
272 stars 43 forks source link

FileWriter constructor name is not FileWriter #31

Open McGiverGim opened 3 years ago

McGiverGim commented 3 years ago

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

https://github.com/thenickdude/webm-writer-js/blob/098a057549eb5c4c63db99302263bb57f67ccfe0/src/BlobBuffer.js#L23

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 commented 3 years ago

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

if (destination && "FileWriter" in globalThis && destination instanceof FileWriter) {
McGiverGim commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

I don't know if this helps, but here is how the FileWriter is being adquired:

https://github.com/betaflight/blackbox-log-viewer/blob/ab87fba7d56b62bfb08db27b7a4aa13ec25a6902/js/flightlog_video_renderer.js#L95-L133

It uses the Chrome way inside Node.js.

And here where it is passed to the library:

https://github.com/betaflight/blackbox-log-viewer/blob/ab87fba7d56b62bfb08db27b7a4aa13ec25a6902/js/flightlog_video_renderer.js#L255-L264

guest271314 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

guest271314 commented 3 years ago

As mentioned in https://github.com/thenickdude/webm-writer-js/issues/31#issuecomment-871831497 "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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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

McGiverGim commented 3 years ago

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 commented 3 years ago

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.