Hi,<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">I see that your patch changes the definition of a function in the C API<br>
(VRTAddComplexSource). Up to now, the GDAL C API has been set in stone. This<br>might change in GDAL 2.0 though, but I'm going to start a specific thread on<br>this to have global feedback from the GDAL community.</blockquote>
<div>Yes, I understand. I hesitated a bit before changing the public interface but I had no choice except to add another function with another name and the same argument ...</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
But even if we are OK to change the C API, I begin to think that this<br>VRTAddComplexSource() function becomes ugly with so many parameters. Perhaps<br>we should add a char** papszOptions to provide additional options without<br>
breaking it each time. Or I'm wondering if the C API is really the best option<br>for doing such complex things, and using the C++ API wouldn't be just better.</blockquote><div>I agree to change the VRTAddComplexSource() function to make it more simple. If you use C++ to do it, I think a decorator pattern can be useful to add fonctionnality to a VRT dataset.</div>
<div><br></div><div>But as I say, the main goal is to build VRT with LUT and filter. Maybe you can envisage it for the gdal 2.0 release?</div><div><br></div><div>Cheers.</div><div><br></div>
<div><br><br><div class="gmail_quote">2012/5/20 Even Rouault <span dir="ltr"><<a href="mailto:even.rouault@mines-paris.org" target="_blank">even.rouault@mines-paris.org</a>></span><br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Le dimanche 20 mai 2012 16:07:07, Saâd HESSANE a écrit :<br>
<div>> Hi Even,<br>
><br>
> The main goal of this proposal is to built using "gdalbuildvrt" VRT files<br>
> containing LUT or filters.<br>
><br>
> The patch that I send allow build VRT with LUT by specifying the -lutin and<br>
> -lutout parameters.<br>
> Ex :<br>
> gdalbuildvrt -lutin "0 1 2" -lutout "4 5 6" test.vrt test.tif<br>
><br>
> We can discuss the fact that define two parameters (-lutin and -lutout) or<br>
> just one parameter (-lut "0,4 1,5 2,6"). This is not a problem.<br>
<br>
</div>Ah, your patch is more extensive than what I thought (I imagined only the<br>
SetLUT() function to be in it).<br>
<br>
I see that your patch changes the definition of a function in the C API<br>
(VRTAddComplexSource). Up to now, the GDAL C API has been set in stone. This<br>
might change in GDAL 2.0 though, but I'm going to start a specific thread on<br>
this to have global feedback from the GDAL community.<br>
<br>
But even if we are OK to change the C API, I begin to think that this<br>
VRTAddComplexSource() function becomes ugly with so many parameters. Perhaps<br>
we should add a char** papszOptions to provide additional options without<br>
breaking it each time. Or I'm wondering if the C API is really the best option<br>
for doing such complex things, and using the C++ API wouldn't be just better.<br>
<div><div><br>
><br>
><br>
> 2012/5/19 Even Rouault <<a href="mailto:even.rouault@mines-paris.org" target="_blank">even.rouault@mines-paris.org</a>><br>
><br>
> > Le samedi 19 mai 2012 18:58:05, Saâd HESSANE a écrit :<br>
> > > Hi Even and thank you for the quick response.<br>
> > ><br>
> > > You should even not consider the VRTComplexSource class to be in the<br>
> ><br>
> > public<br>
> ><br>
> > > > API, so the visibility of its members is not significant. And playing<br>
> > > > with them<br>
> > > > from the outside isn't recommanded at all.<br>
> > ><br>
> > > The fact that you don't consider the class in the public API does not<br>
> > > excuse the fact that this is a mistake encapsulation :)<br>
> ><br>
> > I agree the encapsulation isn't ideal, but unless I'm wrong, this class<br>
> > is *not* in the public API. It is not marked as CPL_DLL exported, so on<br>
> > Windows,<br>
> > you should not be able to access it from the outside of the GDAL lib. On<br>
> > Unix/Linux, as, by default (unless GDAL is configured with<br>
> > --hide-internal- symbols), all symbols are exported, you can technically<br>
> > use it however.<br>
> ><br>
> > > And apart from that, nothing prevents me to use a special driver to do<br>
> > > specific things that are not directly provided by the public API.<br>
> > > Currently I need to build a VRT, and the VRT driver are fine for that.<br>
> ><br>
> > It's<br>
> ><br>
> > > dirrectly use in the gdalbuildvrt utility for example. If the VRT<br>
> > > plugin<br>
> ><br>
> > is<br>
> ><br>
> > > not safe to use, the solution is to correct it.<br>
> ><br>
> > There's always a trade off between exposing API and increasing<br>
> > maintenance burden. The more API you expose, the more difficult it is to<br>
> > make changes afterwards.<br>
> ><br>
> > > To read a VRT file outside from gdal I need to parse an XML file, so I<br>
> ><br>
> > have<br>
> ><br>
> > > to use another dependency (like xerces) to do just a small think.<br>
> ><br>
> > You don't need another dependency. You can use the CPL minixml API (see<br>
> > cpl_minixml.h) that is used by the VRT driver itself for example.<br>
> ><br>
> > > Another argument is the VRTKernelFiltredSource. To set a kernel filtre<br>
> > > we don't have to set the attributes nKernelSize, padfKernelCoefs and<br>
> > > bNormalized, because we can't (the attributes are protected). But the<br>
> > > API offers a setKernel that do exactly the same think that I hope the<br>
> > > setLUT method do in the VRTComplexSource.<br>
> ><br>
> > If you can come with a patch, I'll consider including it, but I still<br>
> > believe<br>
> > you should not rely on that.<br>
</div></div></blockquote></div><br></div>