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

Couple of edge cases are handled wrong #2

Merged
merged 3 commits into from
Nov 19, 2023
Merged

Couple of edge cases are handled wrong #2

merged 3 commits into from
Nov 19, 2023

Conversation

zvavybir
Copy link

I looked a bit through the code of this plugin (extremely readable btw :) ) and found a bug and couple things which are weird (note though that I don't have any experience writing GIMP plugins and haven't programmed in C for a long time, so I might wrongly think that something is wrong even though it's correct):

  • The only bug I'm certain about is that a QOI_OP_RUN doesn't cause an update of the array, because it looks like the array was already updated when the last pixel was processed. But this isn't true when the first pixel is #000000FF, because the previous pixel is initialized with #000000FF, but the array is with #00000000. The issue is well-known: Possible edge case in decoder when very first OP is QOI_OP_RUN phoboslab/qoi#258
  • I'm not entirely sure whether maybe I am the one confusing something, but I think the pixel_index > max_pixel_index + run in line 280 should be pixel_index + run > max_pixel_index.
  • In the read implementation there appears to be an ++column_index; missing in the QOI_OP_RGB section.
  • The variable name max_pixel_index is technically speaking wrong, because what it actually stores is the length of the pixel array and the last (i.e. maximal) index is of course one less. (I know this is extremely nitpicky, but if I'm already unnecessarily complaining I can at least be thorough.)
  • The dr_db variable which stores the values for dr and db look like the dr_dg and db_dg of the standard, but is completely different. (again: extremely nitpicky)
  • That's probably just because I don't know C/GIMP, but the QOI_COLORSPACE_COUNT variant of the enum QoiColorspace seems to be unused.

@Multipacker
Copy link
Owner

Thanks for the pull request and kind words:) This is my first GIMP plugin so there is a possibility that the code that's interfacing with GIMP is completely wrong. I appreciate the thorough review, and I'll try to comment on all of your points:

  • Great catch!
  • That is also correct. I think I had the loop inside of the if earlier and forgot to change the condition.
  • Yeah I was a bit conflicted when choosing the name. Something like pixels_length or pixel_count would be better.
  • Are you referring to the fact that the names have a similar structure but that the values they store represent completely different things? I understand that this could confuse someone looking at the code. We could either inline the read from the file data or change the name to something less confusing.
  • This is a habit I've picked up from this blog post by Casey Muratori. In this case it was unused, but I always include a count member in my enums so that I can easily iterate over them or create an array with space for each variant. Something like this:
for (guint32 colorspace = 0; colorspace < QOI_COLORSPACE_COUNT; ++colorspace) {
    ...
}

All your changes look good to me and I'd be happy to merge them:)

@Multipacker Multipacker merged commit d4df43b into Multipacker:main Nov 19, 2023
@zvavybir
Copy link
Author

Are you referring to the fact that the names have a similar structure but that the values they store represent completely different things? I understand that this could confuse someone looking at the code. We could either inline the read from the file data or change the name to something less confusing.

As I wrote it's extremely nitpicky. I just noticed it and thought I tell you about it. (If you do something about it, I think a rename is better than inlining – the lack of inlining is exactly one of the primary reasons it felt so very readable to me and looked like conforming to a very good coding standard.)

This is a habit I've picked up from this blog post by Casey Muratori. In this case it was unused, but I always include a count member in my enums so that I can easily iterate over them or create an array with space for each variant.

Oh, this is extremely clever! Next time I write an enum I'm certainly going to do this. :)

@Multipacker
Copy link
Owner

Thanks for the input! You actually inspired me to look at this project again and try to fix some things I never got around to do.

[...] the lack of inlining is exactly one of the primary reasons it felt so very readable to me [...]

I'll keep this in mind for the future.

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.

2 participants