<div dir="ltr"><div><div>Hi Even,<br><br></div>Another alternative that will have less impact on existing code and behaviour may be to leave GetDefatultRAT and SetDefaultRAT as they are (ie returning an in-memory representation of the attribute table - GDALRasterAttributeTable). This means existing code can Clone() and Serialize() etc.<br>

<br></div>We could then have another function on GDALRasterBand (eg GetRAT()) that returns the virtual base class version ('GDALIORasterAttributeTable'), if supported by the driver. We don't need a SetRAT() as the intention is that all modifications to the returned object are written to disc. Applications could call GetRAT() first to see if they can use the 'new' way, or drop back to GetDefaultRAT if they can't (memory permitting).<br>

<div><div class="gmail_extra"><br></div><div class="gmail_extra">GDALRasterAttributeTable doesn't need to be derived from GDALIORasterAttributeTable (although it could be). We could have a way of converting a GDALRasterAttributeTable to a GDALIORasterAttributeTable (maybe an alternate constructor of GDALRasterAttributeTable).<br>

<br></div><div class="gmail_extra">Would this be a more acceptable proposal?<br><br>Sam.<br></div><div class="gmail_extra"><br><div class="gmail_quote">On 3 May 2013 11:32, Even Rouault <span dir="ltr"><<a href="mailto:even.rouault@mines-paris.org" target="_blank">even.rouault@mines-paris.org</a>></span> wrote:<br>

<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Le vendredi 03 mai 2013 00:23:48, Sam Gillingham a écrit :<br>
<div>> Hi Evan,<br>
><br>
> Response to your points below:<br>
><br>
> 1) Good point about bool not being available in C. How about making<br>
> this an 8 bit int instead? The idea was to be able to express<br>
> True/False values without using a whole integer, thus saving space. We<br>
> included it in case future drivers support it.<br>
<br>
</div>According to <a href="http://stackoverflow.com/questions/4897844/is-sizeofbool-implementation-defined-in-c" target="_blank">http://stackoverflow.com/questions/4897844/is-sizeofbool-<br>
implementation-defined-in-c</a> , sizeof(bool) is not necessarily 1 (even if I<br>
guess that 99% of the compilers define it as such). If you want the 1 byte<br>
guarantee, then perhaps use the CPL GByte type (which resolves to unsigned<br>
char) for both the C and C++ parts.<br>
<div><br>
><br>
> 2) I was meaning the CSL* family of functions from cpl_string.h since<br>
> the CPLString class won't be available from C so the 'char<br>
> **papszStrList' parameter will be manipulated with these functions.<br>
<br>
</div>Ah ok, but that wouldn't have worked for the read case. You would have needed<br>
to pass a pointer to **papszStrList. So a ***ppapszStrList. Kind of ugly.<br>
<div><br>
> Your suggestion of only the list being allocated and ValuesIO()<br>
> allocating the strings themselves make sense and will be incorporated<br>
> in the RFC.<br>
><br>
> 3) Yes this would be cleaner - I will modify the RFC to include<br>
> GDALDefaultRasterAttributeTable.<br>
<br>
</div>This point is certainly the most important change that would be done with this<br>
RFC.<br>
<div><br>
><br>
> 4,5) regarding Clone() and Serialize(). Maybe these functions should<br>
> be unsupported for GDALRasterAttributeTable?<br>
<br>
</div>You mean defined in the interface and return NULL pointers, or perhaps pure<br>
virutal methods ?<br>
<div><br>
> We are trying to get the<br>
> RAT support on a similar footing to bands (eg. reading/writing the<br>
> data on demand and not holding all data in memory, plus being able to<br>
> access whole chunks at once). AFAIK the Band object does not support<br>
> Clone and Serialize and our feeling is that neither should the RAT<br>
> class.<br>
<br>
</div>So that would mean that the translation of a HFA with a RAT to another format<br>
only supporting PAM RAT would fail, even for modest size source HFA RATs ?<br>
That could be seen as a functional regression. Not that I care that much<br>
personnaly about RATs, but perhaps others do. Actually that would go against<br>
the effort lead in <a href="http://trac.osgeo.org/gdal/ticket/4903" target="_blank">http://trac.osgeo.org/gdal/ticket/4903</a>. This might require<br>
some coordination with Robert and Toby.<br>
<div><br>
> Perhaps the proposed derived GDALDefaultRasterAttributeTable<br>
> would have Clone() and Serialize() to support existing code (and it<br>
> can because it holds all the data in memory)?<br>
<br>
</div>Agreed<br>
<div><br>
><br>
> 6) Yes this is a good opportunity to remove unimplemented functions.<br>
><br>
> 7) The wording is incorrect and will be changed.<br>
><br>
> 8) Agreed.<br>
<br>
</div>Another point I'm thinking is that, if your RFC is the first one implemented<br>
for GDAL 2.0, we should initiate a MIGRATION_GUIDE.TXT file to list all the<br>
source incompatibilities with the 1.X versions and how to address them. For<br>
example, for that RFC, that GDALRasterAttributeTable is no longer<br>
instanciable, and that GDALDefaultRasterAttributeTable should be used to get<br>
the same functionality as of pre 2.0 version, etc..<br>
<div><div><br>
><br>
> We will make the changes to the RFC.<br>
><br>
> Sam and Pete<br>
><br>
><br>
> ------------------------------<br>
><br>
> Message: 2<br>
> Date: Wed, 1 May 2013 11:52:30 +0200<br>
> From: Even Rouault <<a href="mailto:even.rouault@mines-paris.org" target="_blank">even.rouault@mines-paris.org</a>><br>
> To: <a href="mailto:gdal-dev@lists.osgeo.org" target="_blank">gdal-dev@lists.osgeo.org</a><br>
> Subject: Re: [gdal-dev] GDAL Raster Attribute Tables.<br>
> Message-ID: <<a href="mailto:201305011152.30779.even.rouault@mines-paris.org" target="_blank">201305011152.30779.even.rouault@mines-paris.org</a>><br>
> Content-Type: Text/Plain;  charset="utf-8"<br>
><br>
> Le mercredi 01 mai 2013 01:29:30, Peter Bunting a ?crit :<br>
> > Hi All,<br>
> ><br>
> > Sam and I have prepared an RFC for our proposal for improving raster<br>
> > attribute table (RAT) support and put it on the trac.<br>
> ><br>
> > <a href="http://trac.osgeo.org/gdal/wiki/rfc40_enhanced_rat_support" target="_blank">http://trac.osgeo.org/gdal/wiki/rfc40_enhanced_rat_support</a><br>
> ><br>
> > We'd be keen to here feedback on our proposal.<br>
><br>
> Peter,<br>
><br>
> Here are my observations and questions :<br>
><br>
> * Is the boolean data type really necessary ? There will be a difficulty<br>
> with the associated C bindings since 'bool' isn't a C type, but a C++ one.<br>
><br>
> * I'm not sure about what you meant with "The CSLString type will be used<br>
> for reading and writing strings". You probably meant the CPLString C++<br>
> class right ? I'm not sure how this would fly with ValuesIO(GDALRWFlag<br>
> eRWFlag, int iField, int iStartRow, int iLength, char **papszStrList).<br>
> This is probably just an implementation detail.<br>
> Related to that, when eRWFlag == GF_Read, you will probably need to<br>
> document that only the papszStrList array must be allocated with a size of<br>
> iLength * sizeof(char*) bytes, and that the papszStrList[i] values that<br>
> will be set in it will be allocated with the CPL allocation routines. So<br>
> users will be responsible to clear the values with CPLFree().<br>
><br>
> * For the HFA specialized implementation, I guess you would override almost<br>
> all methods of GDALRasterAttributeTable, and that you wouldn't use the<br>
> "std::vector<<br>
> GDALRasterAttributeField> aoFields" member variable for example<br>
> since you don't want the size of anValues/adfValues/aosValues arrays of<br>
> GDALRasterAttributeField to go out of control.<br>
> So perhaps a better design would be to completely remove the private<br>
> members of GDALRasterAttributeTable to make it an abstract class. The<br>
> current implementation, with the private member variables, would go to a<br>
> GDALDefaultRasterAttributeTable that would extend GDALRasterAttributeTable<br>
> (like HFARasterAttributeTable would do).<br>
> This would have of course some backward compatibility implications,<br>
> especially for C++ users, since GDALRasterAttributeTable would be no<br>
> longer instanciable (GDALDefaultRasterAttributeTable would), but I don't<br>
> believe this would cause much churn in existing user code bases, and would<br>
> have only some limited implication in GDAL code base. The C function<br>
> GDALCreateRasterAttributeTable() could be preserved and return an instance<br>
> of GDALDefaultRasterAttributeTable.<br>
><br>
> * If your purpose is to address huge RATs, I'm not sure about what we can<br>
> do with the Serialize() method, which is for example used by the PAM<br>
> mechanism when translating datasets to formats that don't support natively<br>
> RATs. Perhaps in the case the RAT is detected to be large to be reasonably<br>
> serialized (for example 10 MB ?), this method could just return NULL (some<br>
> check should be done in the existing GDAL code base to see if we are<br>
> robust to NULL being returned. I can see at least that CPLAddXMLChild(<br>
> psTree, psPam->poDefaultRAT-<br>
><br>
> >Serialize() ) at line 209 of gdalpamrasterband.cpp would need some care)<br>
><br>
> * In a similar situation (huge RAT), I'm not sure what the Clone() method<br>
> would do. GDALPamRasterBand::SetDefaultRAT() uses Clone() on that passed<br>
> RAT object.<a href="http://trac.osgeo.org/gdal/attachment/ticket/4903/4903.4.diff" target="_blank">http://trac.osgeo.org/gdal/attachment/ticket/4903/4903.4.diff</a><br>
> would do the same on VRTRasterBand::SetDefaultRAT().<br>
> IdrisiRasterBand::SetDefaultRAT() does that too. Perhaps that<br>
> HFARasterAttributeTable::Clone() could return a self-contained immutable<br>
> copy of itself (i.e. it could survive the destruction of the HFADataset<br>
> object of the source RAT). Perhaps only in the huge RAT case. In other<br>
> cases, it could return a GDALDefaultRasterAttributeTable object with all<br>
> columns and rows in memory, that could be potentially modified by calling<br>
> code. Hum, that seems to be complicated...<br>
><br>
> In the SetDefaultRAT() use case, the use of Clone() is not meant at getting<br>
> a free-standing copy of the source object that can be altered, but rather<br>
> to be independant from the life cycle of the passed object. Would be<br>
> probably better addressed with some shared pointer mechanism, but the<br>
> implication would be really too huge and go well beyond changes in RAT<br>
> management...<br>
><br>
> * Not linked with your proposal, but I noticed that gdal_rat.h exposes<br>
> methods that are not implemented, like GetRowMin(), GetRowMax(),<br>
> GetColorOfValue(). Your work could be the opportunity to clean that by<br>
> removing them.<br>
><br>
> * Just a detail: In the current state of your proposal, "The proposed<br>
> additions will have an impact on C binary compatibility because they change<br>
> the API" would be better rephrased as "The proposed additions will extend<br>
> the C API" (there's no compatibility problem since it is backward<br>
> compatible).<br>
><br>
> * I'd like to see a mention of extending the Python autotest suite to test<br>
> the new API, both for their default implementation and specialized<br>
> implementation in the HFA driver.<br>
><br>
> Best regards,<br>
><br>
> Even<br>
</div></div>> _______________________________________________<br>
> gdal-dev mailing list<br>
> <a href="mailto:gdal-dev@lists.osgeo.org" target="_blank">gdal-dev@lists.osgeo.org</a><br>
> <a href="http://lists.osgeo.org/mailman/listinfo/gdal-dev" target="_blank">http://lists.osgeo.org/mailman/listinfo/gdal-dev</a><br>
</blockquote></div><br></div></div></div>