[gdal-dev] RE: progressive rendering

[Even Rouault]

Hi Norman,

My comments :

- I'm wondering if we must really derive a new class GDALAsyncDataset from 
GDALDataset. We could as well add the new methods as new virtual methods in 
GDALDataset, and make a NULL-implementation of them in GDALDataset. The point 
of this remark is for the C API associated to the new C++ methods. We 
currently have only one "C object" for GDALDataset, namely GDALDatasetH. So, 
it means that people should know that this dataset is a GDALAsyncDataset* in 
fact in order to call the new C API. If we add the new methods to 
GDALDataset, they can call the C method without any risk.

- I don't think that Open() must be part of the API. There is already an 
Open() method, but it belongs to the C++ GDAL driver, not the dataset. What 
your proposed Open() method do can be hidden in the implementation of the 
driver. For example in the pfnOpen() method, or at the first 
ProgressiveRasterIO() call

- What is the signature and role of the pfnProgressIO and pProgressUserData 
arguments ? If they are those I proposed yesterday, they don't make sense in 
that pattern based on async messages.

- The SetMessageQueueLimit() is probably something that must not be exposed. 
I'm not convinced myself by my own idea of discarding messages indeed. 
In fact with the latest clarification from Adam about his proposal, I think 
the LockBuffer() / UnlockBuffer() is the right approach. The caller provides 
the buffer in the ProgressiveRasterIO() call and the driver updates it when 
receiving new data and thanks to the messages, notifies the updated area in 
it to the caller. 


In Adam's proposal, there was a GDALAsyncRasterIO* that was returned by the 
initial call and then used to identify it. I think this is a good idea too if 
we want to be able to send several requests at the same time.




So, here are my small adapatations to Adam's proposal :

class GDALDataset
{
	....

public:
	virtual GDALAsyncRasterIO*
			BeginAsyncRasterIO(int xOff, int yOff,
							int xSize, int ySize,
							void * pBuf,
							int bufXSize, int bufYSize,
							GDALDataType bufType,
							int nBandCount, int* bandMap,
							int nPixelSpace, int nLineSpace,
							int nBandSpace,
							char **papszOptions);

	/** @param papszOptions a list of name=value strings with special control 
	 * options.  Normally this is NULL.
	/* The idea of papszOptions is taken from GDALDataset::AdviseRead() */
	/* In fact BeginAsyncRasterIO does pretty much the same thing as */
	/* GDALDataset::AdviseRead */

	virtual void EndAsyncRasterIO(GDALAsyncRasterIO *);
}

class CPL_DLL GDALAsyncRasterIO
{
private:
        GDALDataset* poDS;
	int xOff;
	int yOff;
	int xSize;
	int ySize;
	void * pBuf;
	int bufXSize;
	int bufYSize;
	GDALDataType bufType;
	int nBandCount;
	int* bandMap;
	int nPixelSpace;
	int nLineSpace;
	int nBandSpace;
public:
	... accessors to above things ? ...
	
	/* Returns GARIO_UPDATE, GARIO_NO_MESSAGE (if wait==false and nothing in the 
queue or if wait==true && timeout != 0 and nothing in the queue at the end of 
the timeout), GARIO_COMPLETE, GARIO_ERROR */
	virtual int GetNextUpdatedRegion(bool wait, int timeout,
                                                   int* pnxbufoff,
                                                   int* pnybufoff,
                                                   int* pnxbufsize,
                                                   int* pnybufsize) = 0;
	/* if wait = true, we wait forever if timeout=0, for the timeout time 
otherwise */
	/* if wait = false, we return immediately */
	/* the int* are output values */

	// lock a whole buffer.
	virtual void LockBuffer() = 0;

	// lock only a block
        // the caller must relax a previous lock before asking for a new one
        virtual void LockBuffer(int xbufoff, int ybufoff, int xbufsize, int 
ybufsize) = 0;

        virtual void UnlockBuffer() = 0;
}

This should be rather easy to "C"-ify.


In fact I'm thinking that if we want, we could always provide a valid 
implementation of the new interface for all existing drivers, even for 
non-streaming formats. For example, let's assume that you do 
BeginAsyncRasterIO() on a GeoTIFF. The first call to GetNextUpdatedRegion() 
can just fill the first line of the buffer by doing a classic - blocking - 
RasterIO(). (That's just an example of what it can do. It could do more 
clever things depending on if the dataset is organized by strips or tiles, if 
there are overviews, etc etc). Something like the default 
GDALRasterBand::IRasterIO implementation, but in a progressive way.
That could be interesting as, currently, people must do a sequence of 
RasterIO() to get progressive rendering. They must also take into account the 
block size to get the better performances. That part of that work could be 
delegated to the GDALAsyncRasterIO* of the default implementation of 
GDALDataset::BeginAsyncRasterIO()


I'm also a bit worried that there will not be any real immediate 
implementation of the new interface done for this RFC. It makes it a bit hard 
too see if that really works in practice... We should avoid releasing a GDAL 
version with only the interface and then discovering when we implement it the 
first time that there's this little thing missing that prevent it from 
working. That's my "show-me-the-whole-thing-working" side...


As far as the process of adopting a RFC is concerned, 
http://trac.osgeo.org/gdal/wiki/rfc1_pmc gives the formal process.

Before asking for a vote on your proposal, I think you should get a positive 
feedback of your proposition from the people that have already taken part to 
the discussion and then extend it with the C API, as this is very important 
because of the requirement of its stability. I think that the SWIG people 
would be interested by your proposals in that domain too.

Best regards,

Even

Le Thursday 28 August 2008 23:05:13, vous avez écrit :
> Hi Adam, Tamas, Even, all
>
> I have updated the RFC
>
> http://trac.osgeo.org/gdal/wiki/rfc24_progressive_data_support
>
> And completely changed the pattern used to reflect the general consensus
> to use an asynchronous queue for communication between threads.
>
> Can you comment on this, and let me know if it is acceptable?
>
> Can we iterate this is a few times, and then how is this RFC approved
> (or rejected!)?
>
> Many thanks,
>
> Norman
>
> -----Original Message-----
> From: Adam Nowacki [mailto:nowak at xpam.de]
> Sent: Thursday, August 28, 2008 10:50 AM
> To: Even Rouault
> Cc: Norman Barker; gdal-dev at lists.osgeo.org
> Subject: Re: [gdal-dev] RE: progressive rendering
>
> Even Rouault wrote:
> > I don't know JPIP, but I can image that the driver would start a
> > thread when
> > AsyncRasterIO() is called. It communicates with the server and
> > receives the updates with a polling loop. When it has received an
> > update,it put the received data as well as the parameters describing
> > the window, etc... in a structure (let's call it a ticket), pushes
> > that ticket in a stack and goes on pushing tickets, or wait for the
> > ticket to be consumed by the reader (both are possible, even if you
> > can't push continuously new tickets as memory will increase, so the
> > working thread would have to go in idle mode until the queue decreases
> >
> > a bit)
> >
> > The NextAsyncRasterIOMessage() call will check that some message is
> > available and unstack the first ticket. In fact, the LockBuffer() /
> > UnlockBuffer() could probably be avoided at the API level. Of course
> > the implementation of
> > NextAsyncRasterIOMessage() needs an internal mutex to protect the
> > accesses to the queue.
>
> My idea was to update the data buffer given to AsyncRasterIO immediately
> after receiving data and write only window coordinates into the queued
> messages. That way the queue will remain small, a few KB's at most. This
> is also why LockBuffer() / UnlockBuffer() is there, to protect the
> buffer from async updates while we read from it. LockBuffer(xoff, yoff,
> xsize, ysize) allows almost no wait operation if used with coords from
> queue.