[gdal-dev] RE: progressive rendering
even.rouault at mines-paris.org
Thu Aug 28 19:42:21 EDT 2008
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
- 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 :
BeginAsyncRasterIO(int xOff, int yOff,
int xSize, int ySize,
void * pBuf,
int bufXSize, int bufYSize,
int nBandCount, int* bandMap,
int nPixelSpace, int nLineSpace,
/** @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
void * pBuf;
... 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* pnybufsize) = 0;
/* if wait = true, we wait forever if timeout=0, for the timeout time
/* 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
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.
Le Thursday 28 August 2008 23:05:13, vous avez écrit :
> Hi Adam, Tamas, Even, all
> I have updated the RFC
> 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,
> -----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
More information about the gdal-dev