Skip to content

Conversation

@197g
Copy link
Member

@197g 197g commented Nov 30, 2025

The argument is summarized in #2355 already. The details about the misfit is expanded on in #2481 (comment).

The main part of its implementation is spent on policy working around
the underlying decoder not really cropping by itself and still yielding
all the data. In this sense it is more like the antipole to what the
trait really should be. That is of course part of the issue with the
trait; none of our implementations provide the real benefit over the
naive 'throw-away-data' strategy and that happens by them using what is,
really, a pitfall to be quite frank. You really should not be using this
except in the circumstance that you can't avoid it. And performant
decoding, in general, should not work by 'scanlines' (apart from png
where the filter/compression depends on this concept and hence the
previous line is always necessary).

This held true for both of the removed decoder bindings. In BMP it is quite suspiciously so as that format is meant to be mmap'd—but the called utility reads every single byte regardless, wasting lots and lots of energy. We did not get complaints about such behavior but do get some for inefficient other operations. Let's face the reality that most of our users call into ImageReader and do not interact with the trait system directly—the static extension point also make it impossible to fit into as an optional path way into ImageReader since it is built around boxing and type erasing the underlying decoder early.

Closes: #2355


Restricting the viewport before a readout is one of the interactions I had in mind for the reader type in #2672. I'll come up with a defaulted method in ImageDecoder instead. Since the trait is misbehaved regardless that is a separate matter from removing it, to me.

@197g 197g marked this pull request as ready for review November 30, 2025 02:42
The argument is summarized in #2355 already. The details about the misfit is
expanded on in <#2481 (comment)>.

	The main part of its implementation is spent on policy working around
	the underlying decoder not really cropping by itself and still yielding
	all the data. In this sense it is more like the antipole to what the
	trait really should be. That is of course part of the issue with the
	trait; none of our implementations provide the real benefit over the
	naive 'throw-away-data' strategy and that happens by them using what is,
	really, a pitfall to be quite frank. You really should not be using this
	except in the circumstance that you can't avoid it. And performant
	decoding, in general, should not work by 'scanlines' (apart from png
	where the filter/compression depends on this concept and hence the
	previous line is always necessary).

This held true for both of the removed decoder bindings. In BMP it is
quite suspiciously so as that format is meant to be mmap'd—wasting lots
and lots of energy. Let's face the reality that most of our users call
into ImageReader and do not interact with the trait system directly—the
static extension point also make it impossible to fit into as an
optional path way into ImageReader since it is built around boxing and
type erasing the underlying decoder early.
Copy link
Contributor

@fintelia fintelia left a comment

Choose a reason for hiding this comment

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

Sounds good to me.

And if we do decide on something like a default-implemented method on ImageDecoder for adding back similar functionality, it won't be semver breaking so it is fine to land post-1.0 release.

@197g 197g merged commit 8660412 into main Nov 30, 2025
32 checks passed
@197g 197g deleted the remove-read-rect branch November 30, 2025 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Consider removing the ImageDecoderRect trait

3 participants