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

IfcStreamer fails with Safari at File System API #508

Open
5 tasks done
fethigurcan opened this issue Oct 8, 2024 · 4 comments
Open
5 tasks done

IfcStreamer fails with Safari at File System API #508

fethigurcan opened this issue Oct 8, 2024 · 4 comments
Labels
bug Something isn't working

Comments

@fethigurcan
Copy link

fethigurcan commented Oct 8, 2024

Describe the bug 📝

Hello,

I just upgraded to latest 2.3.0 version. Thanks a lot, still discovering and File System API do the job near perfect.

I have tested and it is ok for Chrome, Firefox, Opera, However it directly fails while opening any Streaming model with Safari and Safari IOS.

According the MDN (https://developer.mozilla.org/en-US/docs/Web/API/File_System_API). There is a lack of support to .createWritable method for these browsers currently.

So, we get these error:
Unhandled Promise Rejection: TypeError: (await (await this.getDir(this.baseDirectory)).getFileHandle(e, { create: !0 })).createWritable is not a function.

I can detect and turn of caching (or library can do it itself) in this condition or if we have an (auto/manuel)option to fallback to the old IndexedDB method until Safari support .createWritable. Or any other way to write these files to the File_System_API without .createWritable?

is it possible? Thanks.

Reproduction ▶️

No response

Steps to reproduce 🔢

loading Any streaming model with streamer.useCache=true option with Safari browser. (it works without cache)

System Info 💻

Binaries:
    Node: 20.14.0 - ~/.nvm/versions/node/v20.14.0/bin/node
    Yarn: 1.22.22 - ~/.nvm/versions/node/v20.14.0/bin/yarn
    npm: 10.7.0 - ~/.nvm/versions/node/v20.14.0/bin/npm
  Browsers:
    Chrome: 129.0.6668.90
    Safari: 18.0
  npmPackages:
    @thatopen/components: ^2.3.0 => 2.3.0

Used Package Manager 📦

npm

Error Trace/Logs 📃

Unhandled Promise Rejection: TypeError: (await (await this.getDir(this.baseDirectory)).getFileHandle(e, {
create: !0
})).createWritable is not a function.

Validations ✅

  • Read the docs.
  • Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
  • Make sure this is a repository issue and not a framework-specific issue. For example, if it's a THREE.js related bug, it should likely be reported to mrdoob/threejs instead.
  • Check that this is a concrete bug. For Q&A join our Community.
  • The provided reproduction is a minimal reproducible example of the bug.
@fethigurcan fethigurcan added the bug Something isn't working label Oct 8, 2024
@fethigurcan
Copy link
Author

fethigurcan commented Oct 8, 2024

Hello again,

As a workaround, I have overwritten the .add method of StreamerFileDb by injecting outside. The good news is, it still uses same file system API, however as my reading, the only way to write a file without .createStream() is .createSyncAccessHandle() which widely supported but it needs dedicated web worker.

I know, there is no webworkers in the current base afaik, so I'am not sure that how to do a good request for a merge.

My current DRAFT patch is very simple at below and works every browser with File System API. I just changed one line regarding to move to web worker actually.

@HoyosJuan @agviegas please, your comments.

It is overwritten on the created instance.

const streamer = this.#components.get(OBCF.IfcStreamer);
const cacheWebWorker = new Worker("./cache-dedicated-web-wroker.js");
streamer.fileDB.add = async (filename, data) => {
  return await new Promise((resolve, reject) => {
    const internalFileName = streamer.fileDB.encodeName(filename);
    cacheWebWorker.postMessage([
      streamer.fileDB.baseDirectory,
      internalFileName,
      data,
    ]);
    cacheWebWorker.onmessage = function (msg) {
      if (msg.data === true) {
        streamer.fileDB.updateLastAccessTime(internalFileName);
        resolve();
      } else {
        reject(msg.data);
      }
    };
  });
};

and the web worker is:

onmessage = async function (msg) {
  try {
    const baseDir = msg.data[0];
    const fileName = msg.data[1];
    const data = msg.data[2];
    const dir = await (
      await navigator.storage.getDirectory()
    ).getDirectoryHandle(baseDir, {
      create: !0,
    });
    const handle = await dir.getFileHandle(fileName, {
      create: !0,
    });
    //const writable = await handle.createWritable();
    const writable = await handle.createSyncAccessHandle({
      mode: "readwrite",
    });
    await writable.write(data);
    await writable.close();
    postMessage(true);
  } catch (e) {
    postMessage(e);
  }
};

@agviegas
Copy link
Contributor

Hi @fethigurcan thank you very much for this! Did you notice any change in the performance? If now, I think we can add this to the library :)

@fethigurcan
Copy link
Author

fethigurcan commented Oct 10, 2024

I did not measure but it seems same or may be feels small performance improvement due to service worker on separate thread.

I have an question according this. do we really need to await for this .add function. .add function caches and other loading processes continue in parallel?

@agviegas
Copy link
Contributor

@fethigurcan maybe not, we can try skipping that and see if it works.

By the way, I'm aware of a bottleneck in streaming that awaits getting the files one by one instead of in parallel. I plan to fix this in this milestone, so together with this, the overall speed of loading and streaming experience should improve notably

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants