Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support grdview -Gimage rather than -grdview -Ggrd_r -Ggrd_g -Ggrd_b #5631

Closed
maxrjones opened this issue Aug 11, 2021 · 8 comments
Closed
Labels
enhancement Improving an existing feature

Comments

@maxrjones
Copy link
Member

Description of the desired feature

Following the conversations in #4044 and #4163, it would be better to support passing an image to grdview -G option rather than red,green,blue bands.

@maxrjones maxrjones added the enhancement Improving an existing feature label Aug 11, 2021
@PaulWessel
Copy link
Member

Fun fact (now that I am looking at grdview.c): Because we need to project and resample that image in order to drape it, we actually end up converting any -Gimage file to three grids for r,g,b...

So while I think we want to deprecate the user option of providing three grids like we did for grdimage (we should not put it on the users to create such grids from an image they want to plot), internally I think we still need to do it via three grids, unless @joa-quim can enlighten me if there is a way for us to do what we do on a GMT_IMAGE instead; it was Joaquim who added this capability in the first place.

@joa-quim
Copy link
Member

That code was written before GDAL has evolved into the librarified mode where we can call GDAL CLI programs as functions. That is what is used by grdgdal and I think the best solution to this would be to call the librarified version of gdalwarp

@PaulWessel
Copy link
Member

PaulWessel commented Aug 12, 2021

Is that what we need, though? I though since we are trying to plot an image onto the undulating topo surface we need to do the scanline rasterization of the topogrid but assign pixel colors from the image. I see we determine the projected i,j pixel bounds of a deformed 3-D gridcell and then we loop over the 4 grid vertices, get r,g,b from z or from the image (via 3 grids r,g,b) and then compute a weighted average of the r,g,b values that depend on distance from the 4 nodes (/* Crude weighted average based on 1/distance to the nearest node */). I guess I am not sure how gdalwarp works and if it project that image into the GMT projected rectangle resulting in the-R -J -p view?

Looking at the code, I do not see what the point was to get red, green, blue grids from the image other than to avoid making some trivial changes in the scanline section (get r,g,b from the image there instead).

@joa-quim
Copy link
Member

Thought you wanted to project the image. I don't remember that code anymore but if it was me, that guy deserves severe punishment. The memory overhead is 12x the image size. An horror.

@PaulWessel
Copy link
Member

I think the trick with a rewrite is that as we visit node ij we need to get the r,g,b for that node in the image and that is when the various image organizations makes things complicated. May need a generic gmt_get_color (struct GMT_IMAGE I*, uint64_t node, unsigned int k) to return r,g,b[alpha] for k=0:3. Maybe we already have bits and pieces of that in grdimage.

@joa-quim
Copy link
Member

That risks to be very inefficient for band interleaved images where r,g,b pixels lay much further away than cache memory. A function that returns image chunks that fit in cache should be much better.

@PaulWessel
Copy link
Member

What is cache memory size and why would that matter when we also keep the entire grid in memory anyway. The 3-byte image is less space than 4-byte grid.

@joa-quim
Copy link
Member

joa-quim commented Aug 12, 2021

You are asking more than I know.

L1 & L2 cache memory is very fast memory. RAM is not. There can be a factor of 10 in accessing data from RAM, that is copied into chunks to L1 & L2. I think L1 (the fastest) is on the order of 64 k, L2 is intermediate. All of them are processor dependent.
When one accesses an Band interleaved image that it's to big to fit in cache chunks of it are copied into cache. But if the seeked element is not in cache, the entire cache is emptied and filled with a new data chunk copied from RAM. This data copy process is one or more orders of magnitude slower than CPU clock so the program halts waiting for the data transaction to finish. That's is part of the reason why accessing a row-major array by rows (or the converse) is so inefficient. If we have a function that gives back chunks of the image that are stored in cache (an OS decision), memory access should be pretty faster.

P.S. If someone who knows more about these matters is reading this, please give us some feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improving an existing feature
Projects
None yet
Development

No branches or pull requests

3 participants