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

Some suggestion #13

Closed
jimmywarting opened this issue Jul 10, 2023 · 4 comments · Fixed by #16
Closed

Some suggestion #13

jimmywarting opened this issue Jul 10, 2023 · 4 comments · Fixed by #16
Assignees

Comments

@jimmywarting
Copy link

https://github.com/imgly/background-removal-js/blob/7b074703ac1c322a10fb551dd7d6e46a2705d4e4/src/bundle.ts#L86C1-L128C2

i think this could be better replaced with this:

async function fetchKey(key: string, config: Config) {
  const entry = bundle.get(key)!;
  let url = entry.url;
  if (config.publicPath) {
    url = new URL(url.split('/').pop()!, config.publicPath).toString();
  }

  const response = await fetch(url, config.fetchArgs);

  const chunks = config.progress 
    ? await fetchChunked(response, entry, config, key)
    : [await response.blob()];
  
  const data = new Blob(chunks, { type: entry.mime });
  if (data.size !== entry.size) {
    throw new Error(
      `Failed to fetch ${key} with size ${entry.size} but got ${data.size}`
    );
  }
  return data;
}

async function fetchChunked(
  response: Response,
  entry: any,
  config: Config,
  key: string
) {
  const reader = response.body!.getReader();
  // let contentLength = Number(response.headers.get('Content-Length'));
  const contentLength = entry.size ?? 0;
  let receivedLength = 0;

  let chunks: Uint8Array[] = [];
  while (true) {
    const { done, value } = await reader.read();
    if (done) {
      break;
    }
    chunks.push(value);
    receivedLength += value.length;
    config.progress(`fetch:${key}`, receivedLength, contentLength);
  }
  return chunks;
}

there are some advantages to using native method instead of calling the main thread for each and every chunk while streaming.
calling eg response.blob() instead of using response.arrayBuffer() text or using the response.body.getReader can also have some advantages as it can optimize what it should do with the blob.

@DanielHauschildt
Copy link
Contributor

You are right, this is just needed if we want to report the download progress. Do you know a better way to report progress and not call the main thread on every chunk?

@jimmywarting
Copy link
Author

Nope, sry.
it aint possible yet. give this issue a 👍
you could use old classic XHR. with the progress event listener or use background-fetch

bgFetch.addEventListener('progress', () => {
  // If we didn't provide a total, we can't provide a %.
  if (!bgFetch.downloadTotal) return;

  const percent = Math.round(bgFetch.downloaded / bgFetch.downloadTotal * 100);
  console.log(`Download progress: ${percent}%`);
});

but i don't think i would bother with it...
i don't really think it's up to background-removal-js to be responsible for trying to download something with fetch.
have any network or file system access.

For library author i think it's useful to think more of a onion architecture. Try to develop your package with as little dependencies or knowledge of the platform you are running your code on, try to think as if your module was running on a sandboxed enviorment (or web worker, deno, node or bun.js) with no filesystem or network access, or no access to node or browser API's. The core layer of your application should be like a stdin and stdout. How the consumer reads and saves data should be entirely up to the the developers using your package.

I might not want or need to use every feature a library brings to the table. then it becomes a js fatigue.

It's not hard or annoying to just do the fetching of blob's yourself.

fetch(req)
  .then(res => res.blob())
  .then(imglyRemoveBackground)
  .then(done)

blob's can come from any place like fs.openAsBlob(path), response.blob(), fileHandle.file(), fileInput.files[0], XHR, blob constructor, drag and drop and so on... so i don't think it's a libraries responsibility to support every single possible way you can pass xyz to function x and get a blob out of it... i think they should try to do the most minimal that is required, but this is just my opinion.

@DanielHauschildt
Copy link
Contributor

I kind of agree in general. However, in terms of this library you would have to prefetch the wasm files as well as onnx files all as separate blobs and pass it to the js library.

One workaround would be to pass fetch as an import, or maybe provideBlob which than defaults to fetch.

@jimmywarting
Copy link
Author

should have removed this if check as well

-   if (config.progress)
      config.progress(`fetch:${key}`, receivedLength, contentLength);

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 a pull request may close this issue.

2 participants