[gdal-dev] RE: progressive rendering

[Norman Barker]

Hi Even,

Firstly, Tamas is right ReadBlock, RasterIO are identical - stupid mistake on my part :-) - a user can still optimize the remote reading by picking the correct view to match the image metadata from the server (if they so wish).  

Comments inline and I have also updated the wiki.

I would like to propose this for a vote, where we are voting to approve the interface for submission into GDAL pending an implementation.  I have initial approval (as in from my employer) to do the implementation - also I am contacting other jpip vendors to make sure we get an interoperable driver; we will of course be testing against kakadu as well.  I wish to get the interface approved before proceeding to code the engine.  

Comments as always appreciated, apologies if I have not done the vote part correctly - it of course has a +1 from me!

My comments inline below;

-----Original Message-----
From: Even Rouault [mailto:even.rouault at mines-paris.org] 
Sent: Wednesday, September 03, 2008 4:42 PM
To: Norman Barker
Cc: gdal-dev at lists.osgeo.org
Subject: Re: [gdal-dev] RE: progressive rendering

Norman,

just a very quick review of the latest state of the RFC after a quick reading 
of it... 

- do we really need to make new classes GDALAsyncDataset and 
GDALAsyncRasterBand. I've raised that point on previous email. --> potential 
problem for the corresponding C API, but, I agree, not critical. We just need 
to be aware of it.

<ncb>
I agree with your comments about the C API, by making GDALAsyncDataset and GDALAsyncRasterBand it just makes the code base cleaner.  I am 50:50 on which way to go.  Am I right in thinking that if we added the virtual methods to GDALDataset then we could still have the same methods declared as virtual on GDALASyncXxxx and everything would work the same?  If so I propose we go as we are, and add the virtual methods later if required for the C API.
</ncb>

- if we allow classical RasterIO to be mixed with CreateRasterIOContext, or 
not, should be clearly stated. Or maybe just state that it is "left to the 
implementation driver. RTFM of the driver where it is specified and check 
well the returned error codes !"
- same remark with 2 or more CreateRasterIOContext being used "at the same 
time"

<ncb>
I think it is going to be RTFM for the user.  JPIP for example does allow single shot stateless requests to the server where you get back an image for a particular view - this would correspond to classical RasterIO.

I think we will be supporting 2 or more CreateRasterIOContext(s), e.g. thumbnail and main image - but this will be per the documentation of the format driver, and if not supported an error code should be raised.
</ncb>

- I don't really see how "static GDALDataset *GDALAsyncDatasetOpen" can be 
used by GDALOpen(). The current GDALOpen method iterates on each DRIVER 
object and calls  pfnOpen() pointer function on it. It has only knowledge of 
the driver object, so it couldn't make any use of GDALAsyncDatasetOpen(). Do 
we need to open the dataset in a particular way for the progressive/async 
rendering ? Can't arguments just be provided as the dataset name. Currently 
most drivers opening datasets that are not real files have a syntax 
like "DRIVER_NAME:host=XXXXX:dbname=XXXXX:user=XXXX:". This could be a way of 
initiating async io mode if necessary. Or maybe the papszOptions should be 
used at the CreateRasterIOContext() level. That's a convenient way of 
extending stuff, but we should avoid abusing it too... I guess your knowledge 
and experience of JPIP can guide the choice. My feeling is that it would be 
more appropriate to add it into CreateRasterIOContext(), as this will define 
a new transaction with the server. It seems unlikely that we would need to 
change the parameters of this transaction during it. This is just an 
intuition.

<ncb>
GDALAsyncDatasetOpen is called at the dataset level to open a session to the JPIP server and gets all the image metadata etc - this metadata is required before CreateRasterIOContext is called.
</ncb>

- I don't understand how SetView() interacts with the corresponding parameters 
in the initial CreateRasterIOContext().

<ncb>
The view and resolution will change a lot during the session initiated in the GDALAsyncDatasetOpen since the data streamed is interactive with the user input.  Currently SetView works for the currently set resolution level in CreateRasterIOContext, I have just updated the code on the wiki to allow setting the window level (resolution) as well with SetView rather than having to create a new context.
</ncb>
- Really detail question... but when and by whom is the GDALRasterIOContext * 
freed ? I think one of my proposal was to add a symetrical method to 
CreateRasterIOContext. Something like
"void the_good_class_name::DeleteRasterIOContext(GDALRasterIOContext *);" Then 
CancelIO() would be unnecessary.


<ncb>
Thanks for that one, Garbage Collectors in Java spoiled me!

I think CancelIO may still be necessary to implement pauses and resumes to the server from user.  

I have added clean up for the contexts to the wiki.
</ncb>

- Do we really need the GDALAsyncRasterBand class or corresponding 
functionnality ? My feeling is that we don't need it. If the user is only 
interested by one band, he can do that with the CreateRasterIOContext() at 
the dataset level.

<ncb>
It is there for completeness at the moment, after comments from Tamas.  I agree that it is perhaps redundant with the new design, at the CreateRasterIOContext level the bands are specified.
</ncb>

Even

Le Wednesday 03 September 2008 19:24:40 Norman Barker, vous avez écrit :
> Hi Tamas,
>
> Comments inline, thanks again for your thoughts.
>
> -----Original Message-----
> From: Tamas Szekeres [mailto:szekerest at gmail.com]
> Sent: Wednesday, September 03, 2008 10:59 AM
> To: Norman Barker
> Cc: gdal-dev at lists.osgeo.org
> Subject: Re: [gdal-dev] RE: progressive rendering
>
> 2008/9/3 Norman Barker <nbarker at ittvis.com>:
> > <ncb>
> > It would be useful to maintain both ReadNextBlock and RasterIO in the
> > case of JPIP, it can be advantageous (if the client can handle it) to
> > call regions of a server image on tile boundaries -> ReadNextBlock,
>
> but
>
> > in most cases I think RasterIO will be called.
> >
> > If you think it will just be easier to keep RasterIO then I am fine
>
> with
>
> > that.
> > </ncb>
>
> Norman,
>
> I don't see the difference between the functionality of ReadNextBlock
> and RasterIO. I guess it was only a name change when Adam have renamed
> my ReadnextBlock to RasterIO and I didn't object to it.
> Can you explain a possible difference from the aspect of the JPIP
> driver.
> I admit the rasterIO region have already determined when
> CreateRasterIOContext have been called. I think we cannot safely
> change these parameters within the same IO context.
>
> <ncb>
> Isn't there a function in GDAL to get the most appropriate block size,
> it will certainly be possible to advertise this through JPIP. Whereas
> with RasterIO you could specify a region that covers many blocks.  Again
> I defer to you on this, we can make it simple and just keep the one
> function.
> </ncb>
>
> > <ncb>
> > In comparison to a callback function, I think this behaviour is
> > synchronous, we are blocking until a timeout, and then calling again
>
> for
>
> > another time period even if it returns immediately.  However this
>
> isn't
>
> > a bad thing.
> > </ncb>
>
> I consider the blocking will occur until that time when a valid update
> is ready to be rendered by the user. In the meantime the user won't
> really be able to render anything so I guess waiting here is not a big
> issue. However if you would support the user to do any other thing
> within this thread, then implement to return immediately when the
> timeout is set to 0. Then status= IO_PENDING will show that there's
> nothing new can be found in the buffer and RasterIO should be called
> again soon.
>
> <ncb>
> Ok, I have got it now - we have a polling thread on the status - sounds
> good to me.
> </ncb>
>
> > I don't think if the driver would be required to copy padding data
> > into the user buffer since the user will be notified about the update
> > region that should be copied over.
> >
> > <ncb>
> > In the case of JPIP (however unfortunate) the server is allowed to
> > adjust the region of the image returned this may require some padding
> > the requested client buffer.
>
> Hmmm... That's difficult for me to follow. Won't it result in ugly
> effects in the image display?
>
> <ncb>
> It can result in ugly effects on the display, however those 'ugly'
> regions will be filled with subsequent requests to the server.  It is
> part of the JPIP spec. so I mentioned it.
>
> Many thanks.
> </ncb>
>
> Best regards,
>
> Tamas
> _______________________________________________
> gdal-dev mailing list
> gdal-dev at lists.osgeo.org
> http://lists.osgeo.org/mailman/listinfo/gdal-dev