Skip to content

Fix usage of IntCachedColorMap in Indexed PNG encoding - #2075

Merged
echeipesh merged 4 commits into
locationtech:masterfrom
echeipesh:fix/IntCachedColorMap
Mar 20, 2017
Merged

echeipesh merged 4 commits into
locationtech:masterfrom
echeipesh:fix/IntCachedColorMap

Conversation

@echeipesh

Copy link
Copy Markdown
Contributor

IntCachedColorMap implementation was not covered by unit tests and turned out to be incorrect.
This PR adds unit tests for its usage and changes the behavior to something that works.

It looks like this class fell through some cracks in the refactors. In reality it leans on implementation of FastMapHistogram to convert pixels directly to colors, which uses a hash table lookup.

In effect we get eC color encoding instead of Log which is provided by IntColorMap

@echeipesh echeipesh added the bug label Mar 16, 2017
@echeipesh echeipesh added this to the 1.1 milestone Mar 16, 2017
@echeipesh

Copy link
Copy Markdown
Contributor Author

@metasim This is the bug you ran into.
Does the test cover the way you were trying to use this feature?

@fosskers

fosskers commented Mar 16, 2017 •

Copy link
Copy Markdown
Contributor

Right, I recall this not being subject to the BreakMap refactor that the other ColorMap subclasses got.

@metasim

metasim commented Mar 16, 2017

Copy link
Copy Markdown
Member

@echeipesh I believe so. Glad there was actually something behind it.

val r = createTile(arr)
val h = r.histogram
val colorMap = colorMap1.withNoDataColor(0).withBoundaryType(LessThanOrEqualTo).asInstanceOf[IntColorMap].cache(h)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we avoid somehow this runtime cast?

@pomadchin pomadchin left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me, except these two comments.

h.foreachValue(z => ch.setItem(z, map(z)))
new IntCachedColorMap(colors, ch, options)
val cachedColors = h.values()
cfor(0)( _ < cachedColors.length, _ + 1) { i =>

@pomadchin pomadchin Mar 20, 2017 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is h.totalCount would be values count?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

h.totalCount would give me the total number of times any value was seen bytes histogram across all buckets. Not sure if thats useful here. What are you thinking?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eh, nothing smart, that's why i approved it. I tried to avoid somehow calculating length for the second time (as it's already known in fact) and trying to avoid distinct function usage. But no; it's fine as it is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants