[Review request] Image loader API

[Richard Bair]

Hi Lubo,

You might see: http://javafx-jira.kenai.com/browse/RT-18702

I think if I had some class here then ImageLoader could extend from it. (the attached class is old, I'm thinking of an updated design based on these notes I sent previously).

Richard

On Jan 22, 2013, at 5:00 PM, Richard Bair <richard.bair at oracle.com> wrote:

>> first I will try to list some ImageLoader use cases, so there is no confusion where the proposed API is:
> 
> Thanks for these!
> 
> Here is what I'm thinking.
> 
> Image has the ability to load image data in a background thread. Because of this, getPixelReader is spec'd to deal with this situation (return null if the image data isn't loaded yet). Also because of this we have a way to know if an error occurred (isError) and to keep track of the progress. Also there is a "cancel()" method to stop loading of the image in the background.
> 
> Meanwhile, the Media class defines an onError property which, sadly, takes a Runnable and is not a proper event (despite the name). I believe in the media stack the MediaPlayer is the one that does the actual loading, so there isn't really an apples-to-apples comparison between Media and Image (so RT-976 could probably be closed, and what we're really talking about is something different here as we aren't really talking about aligning Image & Media).
> 
> Worker meanwhile defines state related to jobs that occur in the background:
> 	- exception
> 	- message
> 	- progress
> 	- running
> 	- state
> 	- title
> 	- totalWork
> 	- value
> 	- workDone
> 
> Two built in workers exist, Service and Task. Task is a one-shot deal, Service is something that can be restarted. Both Service and Worker add events (onFailed, onCancelled, etc). Service also adds the notion of a "value", although in practice a service could have multiple values if it wanted to.
> 
> A Service could be designed which was always running. A Service could also delegate to multiple independent background threads (because a Task can do anything, and the Task created by a Service can be based on multiple different Tasks running serially, or in parallel).
> 
> Both Service and Task though are really designed around the idea of a thing that executes and ultimately returns a value. In the case of a Task, at the conclusion of its work it is done for good. For a Service, it can be reconfigured and restarted. In this case, what you're really trying to do is to have something which is, in essence, always running (or idle). Because of this you want each event (onCancelled, onRunning, etc) to refer *not* to the state of the loader, but to the state of a particular image being loaded by the loader. The advantage of having the event methods on the loader vs. on each individual image is, ostensibly, the ability to easily add listeners to every image by adding a listener once to the loader, rather than having to add a listener to each image individually. The downside is that you need to use an image loader whereas the current API allows you to just use an image and it would seem nice to be able to just add listeners to an individual image if you wanted to. Whether we like it or not (and I don't like it) we already started down the path of having Images load themselves.
> 
> Which isn't to say that I haven't always wanted an ImageLoader, because actually I have. However the big reason I wanted one was to be able to efficiently handle the job of loading many individual images using a fixed (or whatnot) pool of threads. In fact there was a bunch of code that the NASA WorldWind guys contributed that has so far not been used which handles this very well (and has years of tweaking behind it to get the performance etc just right).
> 
> Anyway, something to keep in mind. But back to Worker. I don't see the API of Worker as being incompatible in this case. Suppose for example, that you have ImageLoader implement Worker. The ImageLoader would start off with state == READY. As soon as the first request for image loading occurs (either synchronous or asynchronous) it changes state to SCHEDULED and then at the proper time (in sync case immediately, in async case when the background thread gets started) to RUNNING. Once it completes, it transitions to SUCCEEDED. When the next call to load an image occurs, it transitions back to READY->SCHEDULED->RUNNING. As long as new calls to load an image occur while it is currently RUNNING, it keeps RUNNING until they all finish processing. You could even have it transition immediately from SUCCEEDED to READY when it completes (so that READY is essentially "idle").
> 
> You could spec it so that it never enters the FAILED state, and only enters CANCELLED if the user explicitly calls cancel (causing all image load operations to be cancelled).
> 
> Since Worker doesn't define the onXXX event handlers, you can redefine them, such that you have a subclass of WorkerStateEvent that defines your image load events, and have all the onXXX event handlers on ImageLoader to be based on those event handlers. getWorker could be defined to return ImageLoader for example.
> 
> This would at least keep the Worker / ImageLoader / Service APIs consistent and tied together without violating the Worker contract.
> 
> The "value" of the ImageLoader could be defined as just being Void.
> 
> But what about the other case, where you just want to use Image and not use ImageLoader? Do you have to use an ImageLoader to get better error handling?
> 
>> Worker doesn't define any event handler and it has one worker to one task / result relationship. I want for ImageLoader and its registered handlers to be able to handle several image loading tasks at once. Which requires at least reference to the corresponding Image in each ImageLoadingEvent. Further the proposed set of events is only an initial set. The last time I looked the toolkit allowed to report some loading warnings as well and to get some metadata when the image file header has been loaded. So we might want to extend the set of ImageLoader events / event fields in the future with such information.
> 
> Hopefully the above (long) explanation helped here -- you really don't' have to have a one-task one-result Worker. One way to have multiple results is for the "value" to be an ObservableList that is populated with results, another is to just have Void as the type and then provide some other API (or none). Also as you noted Worker doesn't define the events (since the events were defined after the interface had already shipped so we could only add them to Worker / Task), so you actually have freedom to implement Worker and define some different (or additional) events.
> 
>>>> * Adds unwanted overhead to images which are constructed directly from application provided pixel data.
>>> I guess I don't understand. It seems like in such cases the LoadWorker could be a singleton instance that is always in the "succeeded" state.
>> 
>> What will the image.getLoadWorker().getValue() return (in the case we don't use Worker<void>)?
> 
> Will come back to this if we go that route. First in the above I took the position "ok we will have ImageLoader, what should it look like". Lets get closure on that. Maybe we can ignore the question of how to get the error when just loading via image and not using image loader (since it is sort of orthogonal question would say).
> 
>>>> * I don't think it is easier to use from FXML. Maybe if the information provided by the Worker interface needs to be displayed, but definitely not if some additional code needs to be executed in response to failed image loading. As to ImageLoader, I already mentioned the possibilities to add additional Image constructors with ImageLoader parameter or to add an imageLoader field to the FXMLLoader class. Even if we don't do that, I found out that FXML uses its own internal builder for Image instances. It will be easy to add support for an optional imageLoader field to this builder.
>>> I think what I was mostly worried about here is not whether the event methods are more/less useful than a Task. It was rather that I should be able to bind a progress indicator (for example) to the progress of the job(s), and that I should be able to kick off an asynchronous load task from FXML. I think this needs to be ironed out still.
>> 
>> We already have a bind-able progress from Image.progress field.
> 
> The difference is that I was thinking about the ImageLoader being a Worker at this point in the email and that the progress of that thing could be the aggregate of multiple background jobs, not just one.
> 
> Richard