[GRASS5] d.photo

[Glynn Clements]

Radim Blazek wrote:

> > Actually, that's not entirely a joke. There is a legitimate argument
> > for sacrificing performance in favour of other properties. In this
> > case, simplicity. You can almost always get performance improvements
> > by replacing generic functions with multiple specific-case versions,
> > but doing so creates maintainence problems, and maintenance is GRASS'
> > biggest problem.
> 
> Can I conclude?: GRASS needs RGB CT but we (you) don't feel to have
> enough human sources for GRASS maintenance and prefer not to increase 
> complexity for now. 

Whether or not the utility of such a feature would justify the
additional complexity is open to debate; I don't have a strong opinion
either way. I'm just saying that there are tradeoffs involved; IOW, it
isn't automatically true that anything which increases performance is
good; the other factors also have to be considered.

> So back to 
> 
> > Given that the speed difference between d.rgb and d.photo appears to
> > be almost entirely due to the additional time spent reading three maps
> > instead of one, it would definitely be worth looking into whether the
> > reading speed can be improved. If it could, that would benefit
> > everything.
> 
> I commented 
>    stat = embed_nulls_nomask(fd, (void *) cell, row, CELL_TYPE, 0);
> in G_get_c_raster_row_nomask() and time fall down to 1.469s from
> original 6.930s. So all evil comes from NULLs.
> 
> embed_nulls()
>   -> G_get_null_value_row()
>     -> G_get_null_value_row_nomask()
> and G_get_null_value_row_nomask() spends most of the time
> (2.161s if commented) 
> 
> It means that G_get_null_value_row() takes more than 230% 
> of the time required for  G_get_c_raster_row() (if NULLs not read).

Riiiight. 

230% seems like a high price for backward compatibility.

> It must be possible to improve performance of G_get_null_value_row()
> but rasters are not my hobby.

get_row.c really needs a complete re-write, but that code is extremely
critical, and possibly quite fragile.

> BTW: It seems that raster values are masked by NULLs in current
> region resolution instead of original raster map resolution. I am
> not sure if it is good. That is probably more general problem of
> conversion of rasters to current region.

Right. My guess is that's a result of the null value support being
"tacked on". The nulls should be embedded before the map is resampled.

-- 
Glynn Clements <glynn.clements at virgin.net>