Just another note UpdateDisplay should go into the while loop in order
to render the itermediary stages in the sample code, like:
while (ctx-> status != IO_COMPLETE)
{
ctx->RasterIO(buffer, options);
UpdateDisplay(buffer);
// check has the view updated, is the request cancelled
if (bCancel || viewChanged)
{
update = false;
ctx -> CancelIO();
break;
}
}
2008/9/5 Tamas Szekeres <[EMAIL PROTECTED]>:
> 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 <[EMAIL PROTECTED]>:
>> 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:[EMAIL PROTECTED]
>> Sent: Wednesday, September 03, 2008 4:42 PM
>> To: Norman Barker
>> Cc: [email protected]
>> 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:[EMAIL PROTECTED]
>>> Sent: Wednesday, September 03, 2008 10:59 AM
>>> To: Norman Barker
>>> Cc: [email protected]
>>> Subject: Re: [gdal-dev] RE: progressive rendering
>>>
>>> 2008/9/3 Norman Barker <[EMAIL PROTECTED]>:
>>> > <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
>>> [email protected]
>>> http://lists.osgeo.org/mailman/listinfo/gdal-dev
>>
>>
>> _______________________________________________
>> gdal-dev mailing list
>> [email protected]
>> http://lists.osgeo.org/mailman/listinfo/gdal-dev
>>
>
_______________________________________________
gdal-dev mailing list
[email protected]
http://lists.osgeo.org/mailman/listinfo/gdal-dev
