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

add .ksy for the GIMP brush file format #427

Merged
merged 18 commits into from
Jul 5, 2021

Conversation

armijnhemel
Copy link
Collaborator

This is a .ksy for version 2 of the GIMP brush file format

image/gimp_brush.ksy Outdated Show resolved Hide resolved
encoding: UTF-8
instances:
body_size:
value: width * height * color_depth
Copy link
Contributor

Choose a reason for hiding this comment

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

Please consider structuring it, since now it is not clear, what is the order of the pixels and components within pixels.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not sure I entirely understand this one (this is my first .ksy, so please bear with me). It is basically a row based format. Depending on the color depth there might be a single byte (grayscale) or four bytes (RGBA). How would you structure this?

Copy link
Contributor

@KOLANICH KOLANICH Mar 3, 2021

Choose a reason for hiding this comment

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

seq:
...
  - id: body
    type: row
    repeat: expr
    repeat-expr: header.height

types:
  row:
    seq:
      - id: pixels
        type:
          switch-on: _root.header.color_depth
          cases:
            color_depth::grayscale: pixel_gray
            color_depth::rgba: pixel_rgba
        repeat: expr
        repeat-expr: _root.header.width
    types:
      pixel_gray:
        seq:
          - id: gray
            type: u1
        instances:
          red:
            value: gray
          green:
            value: gray
          blue:
            value: gray
          alpha:
            value: 1

      pixel_rgba:
        seq:
          - id: red
            type: u1
          - id: green
            type: u1
          - id: blue
            type: u1
          - id: alpha
            type: u1

Copy link
Member

@generalmimon generalmimon Mar 14, 2021

Choose a reason for hiding this comment

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

Thank you @KOLANICH for the pixel structures! They work fine except the pixel_gray type, which should be like this instead:

      pixel_gray:
        seq:
          - id: gray
            type: u1
        instances:
          red:
            value: 0
          green:
            value: 0
          blue:
            value: 0
          alpha:
            value: gray

I found this experimentally: after you edit the pixel value (actually the last byte) of the brush Basic/pixel.gbr in a hex editor (e.g. to 0x55 or 85 in decimal), load it into GIMP and try to draw something with it (but I recommend setting Size to some large value like 50, because with the default size 1 the edges of the brush stroke will be smoothed and you might not be able to pick up the right color).

Then if you press the O (as in Oscar, not zero: 0) key to select the Color Picker and pick the color while holding Shift, the info window that pops up should show this:

Color Picker
Pixel RGB (%)
R: 0 ...
G: 0
B: 0
A: 85

However, although it kind of makes sense to describe all structures in the KS language that can be described in it (at least for documentation/educational purposes), the question is if they are usable in the real world scenarios (whether the performance penalty and memory usage will be acceptable even for larger files).

I haven't measured anything, but I suspect that creating a class instance for each pixel will slightly (I estimate 3-4 times) increase the memory consumption and the overhead with creating them will probably slow down the parsing a bit (in comparison with creating a fixed-size byte array in advance and just gradually filling it with RGBA values as you parse the bitmap, just as you would've done when parsing the file with a hand-written code). Naturally, it would be best to have kaitai-io/kaitai_struct#188, which would be ultimately exactly just filling the fixed-size buffer, but I wonder if the parsing of bitmap pixels is feasible using purely (or mostly) KS in the current state.

But it's true that just for the GIMP brush format, the real file sizes tend to be quite small (usually just tens of kilobytes), so that's not really something to worry about.

Copy link
Contributor

Choose a reason for hiding this comment

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

However, although it kind of makes sense to describe all structures in the KS language that can be described in it (at least for documentation/educational purposes), the question is if they are usable in the real world scenarios (whether the performance penalty and memory usage will be acceptable even for larger files).

Absolutely valid concern. The solution may be to mark some types and fields as "hardware-implemented", so the compiler won't usually generate code for them, but to keep them as a blob.

And an another mitigation is to redesign runtimes to have structs with less overhead for simple cases (i.e. when _io is not needed) and make the compiler to use them and generate the code allocating them properly.

About kaitai-io/kaitai_struct#188 : I don't really beleive in it for images, because there are quite some standard formats for pixels (with different encodings, not necessarily RGB, HSV or YCrCb), and there may be a lot of non-standard, especially in old games. It is impossible and makes no sense to support them all in KS. (Though I guess it may be possible to create a machine-readable spec DSL for pixel formats and let people encode them themeself, but I guess it is out of scope of KS project)

image/gimp_brush.ksy Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@armijnhemel armijnhemel left a comment

Choose a reason for hiding this comment

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

Thank you for the comments. I will review them carefully.

armijnhemel added a commit to armijnhemel/binaryanalysis-ng that referenced this pull request Mar 3, 2021
image/gimp_brush.ksy Outdated Show resolved Hide resolved
image/gimp_brush.ksy Outdated Show resolved Hide resolved
image/gimp_brush.ksy Outdated Show resolved Hide resolved
image/gimp_brush.ksy Outdated Show resolved Hide resolved
image/gimp_brush.ksy Show resolved Hide resolved
@armijnhemel
Copy link
Collaborator Author

oops, sorry, I shouldn't have pushed that last particular commit. I will undo tomorrow and resubmit.

@generalmimon
Copy link
Member

generalmimon commented Mar 5, 2021

@armijnhemel:

oops, sorry, I shouldn't have pushed that last particular commit. I will undo tomorrow and resubmit.

Yeah, I totally understand it. That's why you should never open PRs from master of your fork - in that case, the pull request blocks your master branch and you can't use it normally (for pulling upstream master changes, for example) until the PR is merged.

For your own convenience, remember a rule of thumb when making pull requests from your fork repository: never commit directly on master of your fork. Create a topic branch for every PR you create. See https://contribute.jquery.org/commits-and-pull-requests/#never-commit-on-master:

When you're working on a fork, you should always think of your master branch as a "landing place" for upstream changes. You should only ever make your commits to topic branches, and your own commits should only ever end up on master after they've been merged in upstream by a maintainer.

image/gimp_brush.ksy Outdated Show resolved Hide resolved
@armijnhemel
Copy link
Collaborator Author

@armijnhemel:

oops, sorry, I shouldn't have pushed that last particular commit. I will undo tomorrow and resubmit.

Yeah, I totally understand it. That's why you should never open PRs from master of your fork - in that case, the pull request blocks your master branch and you can't use it normally (for pulling upstream master changes, for example) until the PR is merged.

For your own convenience, remember a rule of thumb when making pull requests from your fork repository: never commit directly on master of your fork. Create a topic branch for every PR you create. See https://contribute.jquery.org/commits-and-pull-requests/#never-commit-on-master:

When you're working on a fork, you should always think of your master branch as a "landing place" for upstream changes. You should only ever make your commits to topic branches, and your own commits should only ever end up on master after they've been merged in upstream by a maintainer.

Talking to lawyers all day melted my brain and I thought I hadn't pushed from master. I only realised later. Won't happen again! :-)

image/gimp_brush.ksy Outdated Show resolved Hide resolved
image/gimp_brush.ksy Outdated Show resolved Hide resolved
Copy link
Member

@generalmimon generalmimon left a comment

Choose a reason for hiding this comment

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

Also see the unresolved comments above: #427 (comment), #427 (comment), #427 (comment)

image/gimp_brush.ksy Outdated Show resolved Hide resolved
image/gimp_brush.ksy Outdated Show resolved Hide resolved
image/gimp_brush.ksy Outdated Show resolved Hide resolved
image/gimp_brush.ksy Outdated Show resolved Hide resolved
image/gimp_brush.ksy Show resolved Hide resolved
image/gimp_brush.ksy Outdated Show resolved Hide resolved
Copy link
Member

@generalmimon generalmimon left a comment

Choose a reason for hiding this comment

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

Thanks for contributing!

@generalmimon generalmimon merged commit 81cefb9 into kaitai-io:master Jul 5, 2021
ZetaTwo pushed a commit to ZetaTwo/kaitai_struct_formats that referenced this pull request May 17, 2022
ZetaTwo pushed a commit to ZetaTwo/kaitai_struct_formats that referenced this pull request May 17, 2022
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