[gdal-dev] RE: progressive rendering

Thu Sep 4 18:30:06 EDT 2008

Call to vote withdrawn - too keen on my part.

I will take in all the comments I receive (thanks Even and Tamas for the extra comments) and start coding the format driver.  

I will then pull the interface from the implementation driver and put this in the RFC 24.  I will try to code to the currently proposed interface, and the comments I have just received in response to my earlier mail.

Many thanks for the help in shaping this RFC and I will be in contact shortly.

Norman

-----Original Message-----
From: Tamas Szekeres [mailto:szekerest at gmail.com] 
Sent: Thursday, September 04, 2008 4:12 PM
To: Norman Barker
Cc: Even Rouault; gdal-dev at lists.osgeo.org
Subject: Re: [gdal-dev] RE: progressive rendering

Norman,

I'd require at least one more roundtrip to reach the final form before
placing a vote on this. I'm afraid not all of the comments have been
taken into account and this one seems more complicated that in could
eventually be.

1. I don't see introducing a new AsyncDataSet call would be more
beneficial than disadvantageous here. It would require some further
action for the user at the C API to decide whether a DataSet supports
this new interface or not. You should refer to GDALOpen which returns
a generic DataSet class to the caller.
I think it's not too bad to allow to implement CreateRasterIO context
for the exising datasets as well. IMO we should simply add
CreateRasterIOContext to the current GDALDataSet class.

2. I'm hesitant to think that adding this SetView method is necessary,
why should we change the raster window within the same iocontext at
all. Is it unconfortable to create a new context in case it the view
is changing?

3. I still see the deprecated ReadNextBlock still exists in the proposal.

4. It seems unreasonable to add DeleteContext as a new method to the
dataset. Why don't we delete the context object itself? The destructor
could call the CancelIO implicitly if needed. With regard to the
garbage collected SWIG bindings like Java and C# they all support a
deterministic finalizer method like finalize(), delete() or Dispose()
to allow the user to clear the things up manually if needed.
We could eventually keep CancelIO however, in order to make sure that
the operation was successfully canceled o not, by setting the state of
the IO accordingly.

By considering these I think it would be in a good shape to start
creating a reference implementation around the concept. I think we all
have some doubts that a real implementation wouldn't bring in further
things that haven't been addressed yet in this RFC.

Best regards,

Tamas

2008/9/4 Norman Barker <nbarker at ittvis.com>:
> 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
>
>
> _______________________________________________
> gdal-dev mailing list
> gdal-dev at lists.osgeo.org
> http://lists.osgeo.org/mailman/listinfo/gdal-dev
>