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

Separate text parsing from binary parsing #177

Open
ghost opened this issue Jul 11, 2019 · 8 comments
Open

Separate text parsing from binary parsing #177

ghost opened this issue Jul 11, 2019 · 8 comments

Comments

@ghost
Copy link

ghost commented Jul 11, 2019

It shouldn't be necessary to load the image files and separate binary buffer when loading the glTF asset – most actions and inspections don't require that data and we only want to load it if and when it is needed.

@syoyo
Copy link
Owner

syoyo commented Jul 12, 2019

Please describe the issue in detail. What is your problem?

@Ybalrid
Copy link
Contributor

Ybalrid commented Jul 12, 2019

I think I understand:

@UX3D-schmithuesen wish there could be a way to either skip or defer image and binary buffer loading.

Currently the behavior of the library is; when you call on of the TinyGLTF::LoadX() functions,

  • JSON metadata is parsed, creating the arrays of nodes, scenes, meshes, accessors, and other kind of glTF objects
  • Binary buffers are loaded (eventually decoded from base64 text) in memory into vectors of bytes
  • Images are also loaded, uncompressed (via, e.g: stb_image) and also loaded into vectors of bytes.

I think Benjamin has a use-case where he wants to use only the result of the parsing of the JSON, and doesn't actually need to load to memory either the binary buffers or the textures. At least not from the get go.

That's actually an interesting idea.

What you could do at the moment is, for the images, you can stub their loading out by providing your own image loader callback. You can do that by providing a function pointer to void TinyGLTF::SetImageLoader(LoadImageDataFunction func, void *user_data) and doing #define TINYGLTF_NO_STB_IMAGE. If your callback doesn't really load the image data, (it just says to tinygltf that it's done) that could already give you a significant speed boost. In my experience, loading the images of a glTF assets is the slowest part of the loading via tinygltf

@syoyo What is you thought about adding a "deferred loading" feature to tinygltf. Something as simple as having buffer and image carry a "loaded/unloadedflag. Use could call aloadData()function to trigger loading. We could keep the current behavior intact, but maybe introduce aTINYGLTF_DEFFERED_LOADING` compile time option that adds this feature. That could be useful. What do you think?

@ghost
Copy link
Author

ghost commented Jul 15, 2019

That is correct, but maybe I just didn't pay enough attention – if I define TINYGLTF_NO_FS, I believe that the tool doesn't read any files which would be exactly what I want.

@syoyo
Copy link
Owner

syoyo commented Jul 15, 2019

TINYGLTF_NO_FS this is not documented in README and rather still for developers.

We may better to clean-up TINYGLTF_NO_FS and TINYGLTF_NO_EXTERNAL_IMAGE(and fs callbacks) and introduce an unified flag not to parse any image/binary assets in the glTF file.

PR is always welcome.

@Mrkol
Copy link

Mrkol commented Dec 15, 2021

I would really like this feature to be implemented in one way or another. I want to load images asynchronously via iocp/io_uring, and that means that providing a function that makes images/buffers immediately available is simply impossible. Maybe make the loader functions return a ternary value, "success", "failure" or "deferred", and then add a simple interface to the model class for providing the deferred file data?

P.S. Reimplementing image parsing is not my intention here, so I would still like tinygltf to use it's default image loading library after I provide the raw file bytes. These concerns should definitely be separate.

@syoyo
Copy link
Owner

syoyo commented Dec 16, 2021

@Mrkol We implemented delayed image loading feature in our private project by using as_is flag

// When this flag is true, data is stored to `image` in as-is format(e.g. jpeg

and providing FS callback function like this:

// Delayed loading of image files since compressed image like jpeg/png takes
// some amount time in decoding. Just add raw image data into queues, then after
// parsing glTF, we'll try to actually decode images in parallel.
static bool DelayedLoadImageData(tinygltf::Image *out, std::string *err,
                                 std::string *warn, int req_width,
                                 int req_height, const unsigned char *bytes,
                                 int bsize, void *arg) {
  (void)err;
  (void)arg;

  if (bsize < 1) {
    if (warn) {
      (*warn) += "Invalid byte length.\n";
    }
    return false;
  }

  out->width = req_width;
  out->height = req_height;
  out->component = 1;  // This will be determined after decoding image data.
  out->image.resize(size_t(bsize));
  memcpy(out->image.data(), bytes, size_t(bsize));
  out->as_is = true;

  return true;
}

...

tinygltf::TinyGLTF gltf;

gltf.SetImageLoader(DelayedLoadImageData, nullptr);

Then, after parsing a glTF scene, scan tinygltf::Model::images and do delayed/async(and multithreaded) image loading where as_is flag is on.

as_is is not intuitive so we could improve API/naming and provide an example of deferred image loading(and also draco-compressed geometry?)

@Mrkol
Copy link

Mrkol commented Dec 16, 2021

@syoyo This is a nice feature, but it only covers deferred image parsing, not file loading. I want to be able to use custom low level (async) IO with the library, which the current interface doesn't really support. Basically separating the file loading into 2 steps: request to load a file, notification that a file was successfully loaded. This is orthogonal to png/jpg parsing.

@syoyo
Copy link
Owner

syoyo commented Dec 17, 2021

@Mrkol Such a feature is not implemented yet. You can contribute PR!(probably by extending custom filesystem callbacks). And will require making RapidJSON context re-entrant: #241

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

No branches or pull requests

3 participants