search

google / earthenterprise

Google Earth Enterprise - Open Source
Apache License 2.0
2.67k stars 889 forks source link

Fix imagery project transparency #1794

Closed duvifn closed 4 years ago

duvifn commented 4 years ago

Fixes #1785. I'm also a bit busy now, but I checked the Allow edits by maintainers checkbox, so if I'm unresponsive feel free to change whatever you want before merging. Thank you very much

googlebot commented 4 years ago

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

:memo: Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

duvifn commented 4 years ago

@googlebot I signed it!

googlebot commented 4 years ago

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

tst-lsavoie commented 4 years ago

I'm following the steps you posted in the issue and I'm still not able to reproduce this. Every time I add the clipped image as a new resource and build it it ends up being masked correctly. Is there any else you're doing that might be different? I've uploaded the image I'm using to the link below in case that's helpful.

https://drive.google.com/file/d/1IWjQvGDgGNRlGRVuRNImyfuAazGXN6xm/view?usp=sharing

The reason I'm being so careful with this is because we've been bitten by changes like this before - we'll fix something and then find out a couple of releases later that we also broke something else. This code is more or less unchanged since GEE was open sourced, so there's no one working on it that understands it completely.

duvifn commented 4 years ago

The resource does masked properly for me, too. The built imagery project, however, doesn't reflect this, at least in my configuration.

duvifn commented 4 years ago

Perhaps the following use case would be clearer: Build two flat DBs: Layer 1- Only with blue-marble Layer 2- As I mentioned in the issue. Then publish them with WMS support and load them one on top of the other in wms client (like qgis). With this issue- Layer 1 can't be seen when layer 2 is on top of it (transparent areas appear as black ). Without this issue transparent areas appear as transparent so you can see through them.

tst-lsavoie commented 4 years ago

Ok, with those instructions I can reproduce it. I've never seen that use case before. I'll have to think about how to handle it.

duvifn commented 4 years ago

The reason I'm being so careful with this is because we've been bitten by changes like this before - we'll fix something and then find out a couple of releases later that we also broke something else. This code is more or less unchanged since GEE was open sourced, so there's no one working on it that understands it completely.

Ah, of course. What I know (hope I understood & recall correctly) from debugging this: Apparently alpha tiles got into the code in version 5.x. Until then imagery project contained only jpeg tiles, so no-data areas appeared as black. The alpha channel is currently stored as a png buffer in a different field in the protobuf, separately from the rgb which are stored as jpeg. When client ask the server for a tile the server check if alpha channel exists and if so it composites and returns png file. Otherwise it returns jpeg. I think it's done this way to preserve back compatibility but it also save a lot of storage space (jpeg is far more efficient than png).

Since we need to merge layers we have to take into account all insets (resources) that below current inset in the project. Calculating the exact alpha value for all of the levels (X all the insets below) is a waste of time if we can use calculations from previous levels and layers, so it's get cached. So, for one inset- we compute only the max level (based on resource's kmp) and loop over lower levels and define if they has cached alpha from higher levels (and if so we put it). The problem is that at this point the packlevel assetver is not yet processed so it's actually doesn't exist on the disk, and also, its paths are not populated yet (output_file_name is empty). As a result, the maximum level's alpha is computed properly (since it's using resource's kmp as the source of the alpha) but in lower levels it does't: this if is always false (since (inset->cached_blend_reader is null pointer) so fusion decides this is an opaque tile and doesn't compute its alpha.

tst-lsavoie commented 4 years ago

Based on your comments and my code inspection it seems safe. Let me suggest an alternate implementation that seems a bit cleaner to me.

  1. In PacketGen.src, on the two checks for cache_alpha.pack (currently lines 262 and 293), we don't check for the existence of the file. We just use blendVer.Ref() (or prevLevelVer.Ref()) unconditionally whether or not it exists.
  2. In RasterMerger.cpp we add an existence check before creating the cached_blend_alpha_reader on line 163. We'd also need to move the comment about 4.x to this point so devs know why the file might not exist, and we should add a log statement any time we decide not to create an alpha reader because the file doesn't exist, probably at the INFO level.

What do you think? Do you want to make those changes? I can do it as well, but I don't want to take your chance to get some commits in the codebase.

duvifn commented 4 years ago

This does look cleaner to me.

Do you want to make those changes? I can do it as well, but I don't want to take your chance to get some commits in the codebase.

Thanks but this is your solution and I don't want to take credit for it (but I appreciate it!). I hope I will contribute in the future (I currently have some features, which I hope to contribute when I have the time to bring them to desired level).

tst-lsavoie commented 4 years ago

Replaced by PR #1836 based on the discussion above.