react-juce icon indicating copy to clipboard operation
react-juce copied to clipboard

Open question: Image/BinaryData Support

Open nick-thompson opened this issue 6 years ago • 10 comments

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 avatar Jun 26 '19 16:06 nick-thompson

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.

nick-thompson avatar Jun 10 '20 13:06 nick-thompson

@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?

JoshMarler avatar Aug 18 '20 13:08 JoshMarler

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 avatar Aug 18 '20 22:08 nick-thompson

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.

nick-thompson avatar Oct 03 '20 14:10 nick-thompson

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

stfufane avatar Jan 09 '21 23:01 stfufane

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

nick-thompson avatar Jan 10 '21 17:01 nick-thompson

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 :

  • I split ImageView in .h and .cpp, it was becoming big and having only the .h had CMake recompile everything at every change, which was annoying.
  • I've created a separate class for the download task listener. I learnt that thread managing is a pain in the ass in the process :)
  • There's an onLoad prop I have to call when the download is successful. I also think an onError prop would be nice and could be used on other image formats as well instead of throwing an exception. It would leave more flexibility to the user on error management.
  • @JoshMarler suggested a lock_guard for the drawable, I'd be happy to add that but didn't fully understand where that would happen and if that would affect every line where drawable is written to. I didn't find how to make it crash this way but I'll let you judge if that's necessary or not.
  • I removed a few lines from the example for my tests and forgot to put them back but realized it after the commit, I'll restore that when we validate the changes

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

Cheers!

stfufane avatar Jan 13 '21 10:01 stfufane

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.

nick-thompson avatar Jan 13 '21 13:01 nick-thompson

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?

stfufane avatar Jan 13 '21 13:01 stfufane

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

nick-thompson avatar Jan 13 '21 14:01 nick-thompson