Skip to content

Expose is_top_down function in BMP Decoder#2760

Merged
197g merged 4 commits intoimage-rs:mainfrom
telecos:fix/expose-top-down2
Feb 9, 2026
Merged

Expose is_top_down function in BMP Decoder#2760
197g merged 4 commits intoimage-rs:mainfrom
telecos:fix/expose-top-down2

Conversation

@telecos
Copy link
Copy Markdown
Contributor

@telecos telecos commented Feb 6, 2026

During the codereview integrating streamed/resumabled BMP decoding in Skia that will be used from Chromium, we found a gap when copying partially decoded rows:

https://skia-review.googlesource.com/c/skia/+/1138796

The issue is that, despite BMP decoder is always doing conversion to always provide the same row ordering independently of the type of file, when we have partially decoded rows and only first n rows are available, it is required to know if the image is top down or not to know if the rows to copy are the first n rows or the last n rows.

This is a gap identified for completing #2696

@telecos telecos marked this pull request as ready for review February 6, 2026 00:43
@telecos
Copy link
Copy Markdown
Contributor Author

telecos commented Feb 6, 2026

Thinking about an alternative, I think that probably it is a better option to return as a second parameter of rows_read function instead of a new dedicated method as knowing orientation from the consumer side only makes sense while having partially decoded rows; once fully decoded, orientation does not matter for the consumer of BMPDecoder as it manages internally the row inversion (the only case where it would be required is that one in which you want to encode exact same file content you decoded, but I don't think it is a valued use case and would require exposing all other header attributes).

@telecos telecos marked this pull request as draft February 6, 2026 08:35
@telecos telecos marked this pull request as ready for review February 6, 2026 08:36
@telecos
Copy link
Copy Markdown
Contributor Author

telecos commented Feb 6, 2026

I updated to return the tuple in rows_decoded and removed the public function which could cause confusion when used on a fully decoded image.

Copy link
Copy Markdown
Member

@197g 197g left a comment

Choose a reason for hiding this comment

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

Let me know what you think about the design direction.

@telecos telecos requested review from 197g and anforowicz February 7, 2026 19:50
@197g 197g merged commit 5d0418d into image-rs:main Feb 9, 2026
32 checks passed
@telecos telecos deleted the fix/expose-top-down2 branch February 23, 2026 12:03
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.

3 participants