JoshMarler / react-juce

Write cross-platform native apps with React.js and JUCE
https://docs.react-juce.dev
MIT License
763 stars 79 forks source link

Open question: Image/BinaryData Support #14

Open nick-thompson opened 5 years ago

nick-thompson commented 5 years ago

Handling image data is an interesting challenge, and there are many different ways to do it.

Currently, Blueprint implements an <Image> element that accepts a source property expecting a value that is an SVG string, which on the backend is parsed by a juce::Drawable and drawn at paint time. This works nicely with Webpack's SVG file loader because it automatically reads require('path/to/svg.svg') statements and satisfies them by embedding the raw svg string from the file.

However, this maybe won't scale for larger, more complicated SVGs, and it totally ignores the other common image use case, which is big PNG files either read from disk or carried in BinaryData. One possible path forward is to support BinaryData directly with some kind of C++ interface that lets you register a key/value pair which is a name and a pointer to some specific binary data, and allow that pair to be referenced by the key on the javascript side. <BinaryDataImage sourceKey={'background'} />. I'm not convinced this is a great idea though.

On the flip side, this might be a good case for a problem that Blueprint intentionally does not solve, and leaves open to the user. For example, if you have a big PNG strip for drawing a knob and you use the juce ImageCache to speed that up, maybe the best approach for interfacing with that is to write a custom KnobView and register that view with the ReactApplicationRoot (#8). Then there's no reason for Blueprint to concern itself with binary data, and you can still take full advantage of the techniques that JUCE already has for interacting with it.

nick-thompson commented 4 years ago

Thinking about this a little more, I think our <Image> component should just support a source prop pretty much identical to React Native's source: http://reactnative.dev/docs/image#source

Currently <Image> only supports a source value which is an SVG string, so the first step will be a breaking change to change that to support data: URIs (and passing those SVG strings as proper data URIs), and then implementing local file loading and remote file loading.

Then the only remaining question is if and how we want to support juce's BinaryData. It seems like an easy add that won't break our API in any other way. I'll propose something simple: since we'll support data: URIs, let's just add another unique scheme that we support bindat: wherein you name the resource you're trying to load.

<Image source="bindat:WaveAnimationBackground_png" />

Then in the implementation, if we see a bindat: scheme we just look up the binary data using the BinaryData::getNamedResource API and render the image that way.

JoshMarler commented 4 years ago

@nick-thompson, You game for me to have a crack at this? Following a URI pattern seems like it shouldn't be too hard. If it's a file path like URI load via file system, if it's URL follow a different model. Can start with file path and go from there.

Binary assets seem like they should be reasonably straight forward too. Maybe go for local URI's first and then support remote file loading as second step?

nick-thompson commented 4 years ago

Definitely! Yea, let's go for local first and defer remote. So first step, then, is supporting data:, file:, and a custom bindat:? Or maybe just start with data: and file:.

data: should roughly match the Data URL spec https://developer.mozilla.org/en-US/docs/Web/HTTP/Basics_of_HTTP/Data_URIs, and will have to edit the GainPlugin example to adapt the way it currently sends svg strings (I think can just prepend the data: with the mime type) file:<filepath> maybe we only support absolute file paths for now? What would a relative file path be relative to? Wondering about things like iOS bundles.... maybe a "todo later" on that bindat:<name> would be just a name-based lookup on BinaryData.

nick-thompson commented 3 years ago

Realizing as I'm coming back to this just now, I definitely want to add http(s): support, so that you can write something like

let background = 'https://images.unsplash.com/photo-1593642703055-4b72c180d9b5';

<Image source={background} />

just as you would expect with <img src="https://images.unsplash.com/photo-1593642703055-4b72c180d9b5" /> in HTML, or as with React Native's Image.

Thinking it probably makes the most sense to focus on data/file/http schemes and come back to the bindat idea later.

stfufane commented 3 years ago

Hey there, as we discussed I'll give a go at the http(s): support, I'll let you know if I encounter a difficulty

nick-thompson commented 3 years ago

Great, thank you. I've assigned your name here as well!

stfufane commented 3 years ago

This was a big one and it required lots of new things to understand for me but I finally got something working. There are lots of changes so before I submit a rotten PR, I'll let you check on my forked repo if the approach sounds right.

https://github.com/stfufane/blueprint/commit/4ba6fb41e03c020ebcd29575ada44fcb8a852db2

A few notes :

Let me know if I have to start from scratch or if there's something good in there :p

Cheers!

nick-thompson commented 3 years ago

I left a few comments inline on your branch.

Generally looks great! A couple things in there we can simplify. Also, you won't need a lock_guard if we use that juce::MessageManager::callAsync trick for getting the actual juce::DrawableImage swap to happen on the main thread.

stfufane commented 3 years ago

Cool, thanks! Just read the comments, looks easier indeed :D I'll change that, should be straight forward this time. I actually had complications because of the thread in a thread due to the use of DownloadTask... I stuck to this comment https://github.com/nick-thompson/blueprint/pull/117#issue-507445831 and didn't realize it would cause this kind of trouble in my mind ^^'

What do you think about the use of onLoad/onError? They're native properties in javascript when loading an image or a script so I thought they'd feel natural in this context. What's the behaviour when using std::runtime_error?

nick-thompson commented 3 years ago

Yea, I think this way is simpler :)

Yes +1 for onLoad, and in keeping with the spec in web (https://developer.mozilla.org/en-US/docs/Web/HTML/Element/Img#image_loading_errors) I say +1 for onError too! Then we can skip the throw runtime_error bit and instead call the onError callback if it's present