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 support for writing images with alpha / simplify working with images #107

Merged
merged 10 commits into from
Nov 14, 2024

Conversation

BlazingTwist
Copy link
Contributor

@BlazingTwist BlazingTwist commented Oct 31, 2024

TODO

  • I've added this as separate instructions. I'll leave it up to you if:
    • this should replace the old instructions (breaking backwards compatibility)
    • or stay separate (possibly causing confusion about which one should be used)
    • or something else?

Some possibly dubious decisions

  • AyaImage2::toDict uses DoubleList for each Channel, so images take up 8x more memory than they should.
    • although the same is true for the old image instructions
  • image.read2 always provides an alpha channel
    • the use-case being: operations that require/support alpha can naively support images without an alpha channel as well
    • this also mirrors the java-native behaviour of getRGB

Dev Test

The documentation shows up and looks fine (if a bit lengthy)
image

A "test" that tries each available format with and without alpha:
test_images.txt
(note: this test behaves differently for different java versions, it's more of an example than a test)

Results in:
__testResult
line 1 of each group is the target pixel value.
line 2 is the result of 'write2 -> read2'
line 3 is the result of 'write2 -> read2 -> write2 -> read2' (looks slightly different for gif and jpg)

WBMP always fails because java expects a MultiPixelPackedSampleModel which isn't supported by image.write2.
Testing on sample files worked fine however.
"sample.wbmp" :{image.read2} "sample.png" :{image.write2}

BMP+alpha fails - correctly. Since BMP does not support alpha.

GIF+alpha looks suspicious, because GIFs can only have a single transparent color-index.

JPG/JPEG+alpha is version dependent.
In Java 8 it produces the shown result.
In later versions it (correctly) refuses to write images with alpha.

@nick-paul
Copy link
Collaborator

Thanks for the implementation and great write up

I've added this as separate instructions. I'll leave it up to you if this should replace the old instructions (breaking backwards compatibility)

I think I'd prefer to replace the old instructions instead of adding new ones, there have been plenty of other breaking changes since v0.4 so it is probably time to upgrade to v0.5 anyway.

AyaImage2::toDict uses DoubleList for each Channel, so images take up 8x more memory than they should. although the same is true for the old image instructions

Currently a double is the smallest (memory) numeric type in Aya. I don't have plans to add other primitive numeric types to the language (for now) so using the DoubleList is the best way to go. Obviously not ideal if we are storing single bytes from the image as doubles but most general image processing/reading/writing cases should be okay

image.read2 always provides an alpha channel

I think this is a good decision based on the factors you mentioned

The documentation looks good, though it does remind me that the documentation design is pretty dated and could probably use some updates like referencing other documentation (I like the hints you added), or storing documentation as objects instead of long strings so they are more versatile. Maybe something I can think about after v0.5...

- update 'image.aya' and 'filesystem.aya'
- fix file resolution for :{image.read2}
@BlazingTwist
Copy link
Contributor Author

I think I'd prefer to replace the old instructions instead of adding new ones

Alright, I've updated the .aya files. __init__ aside, image.aya should behave the same as before.
Good thing you added the filesystem test, my :{image.read2} was failing that.

@BlazingTwist BlazingTwist marked this pull request as ready for review November 1, 2024 14:43
@nick-paul
Copy link
Collaborator

I merged the latest master into this branch and it looks like our automatic testing caught an issue with image reading and writing. The image that is being read in has an alpha channel but the test does not.

[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  7.103 s
[INFO] Finished at: 2024-11-12T17:50:17-05:00
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-antrun-plugin:3.1.0:run (run-aya-tests) on project aya: An Ant BuildException has occured: The following error occurred while executing this line:
[ERROR] /home/npaul/git/aya/build-scripts/run-aya-tests-build.xml:42: The following error occurred while executing this line:
[ERROR] /home/npaul/git/aya/build-scripts/run-aya-tests-build.xml:77: AssertError: 
[ERROR]   Expected: [
[ERROR]   [ 41 -92 34 ]
[ERROR] ]
[ERROR]   Received: [
[ERROR]   [ 41 164 34 255 ]
[ERROR] ]
[ERROR] 
[ERROR] 
[ERROR] > File '/home/npaul/git/aya/target/test-package/base/test.aya', line 28, col 10:
[ERROR] 27 | .}
[ERROR] 28 |     blk :!
[ERROR]      ~~~~~~~~~^
[ERROR] 29 | } test.:test;
[ERROR] 
[ERROR] Function call traceback:
[ERROR]   File '/home/npaul/git/aya/target/test-package/test/filesystem.aya', line 86 in .test:
[ERROR]     {img.pixels [[41 -92 34]]} test.test
[ERROR]     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^
[ERROR] around Ant part ...<ant antfile="/home/npaul/git/aya/build-scripts/run-aya-tests-build.xml" />... @ 5:80 in /home/npaul/git/aya/target/antrun/build-main.xml
[ERROR] -> [Help 1]
[ERROR] 
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR] 
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoExecutionException

@BlazingTwist
Copy link
Contributor Author

Oops, you're right.
My "fix" is biased because when working with images so far I've always expected alpha data.
I've never used image.aya (I always preferred using the image instructions directly) so I can't say for sure if this breaks existing usages.

@nick-paul
Copy link
Collaborator

I tried running the test script and got errors for the formats. I updated the script to use :{image.read} instead of :{image.read2} etc. (I also added a utility function to make the errors print nicely, I need to add a better way to print errors to the core language).
Have there been other internal changes since you originally wrote the test script?

image

Updated script
test_images.aya.txt

Errors:

test_images_error.txt

Generally they are all related to the image writer format: msg: io_err at :{image.write}: unable to use resource /home/npaul/git/aya/w1_argb.jpg. io_err at :{image.write}: unable to use resource /home/npaul/git/aya/w1_argb.jpg. found no image writer that supports the requested format

@BlazingTwist
Copy link
Contributor Author

Generally they are all related to the image writer format: msg: io_err at :{image.write}: unable to use resource /home/npaul/git/aya/w1_argb.jpg. io_err at :{image.write}: unable to use resource /home/npaul/git/aya/w1_argb.jpg. found no image writer that supports the requested format

I believe this is the expected behaviour. From my initial dev-test notes:

JPG/JPEG+alpha is version dependent.
In Java 8 it produces the shown result.
In later versions it (correctly) refuses to write images with alpha.

I used Java 8. Where the JPG writer does not check if the image has alpha or not.
I'm assuming you're using a later version and thus see found no image writer that supports the requested format.

The error message is definitely too verbose. The new error message looks like this:

failed to write image w1_argb.jpeg: 

  type: ::io_err
  source: 
    file: F:\Downloads\test_images.aya
    line: 75
    col: 38
    context: > File 'F:\Downloads\test_images.aya', line 75, col 38:
74 |         {
75 |             target file :{image.write}
     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^
76 |             file images .B ;

  msg: found no 'jpeg' image writer that supports the provided metadata: ImageMeta{isGray=false, hasAlpha=true, premultiplied=false, javaImageType=null}

@nick-paul
Copy link
Collaborator

In Java 8 it produces the shown result.
In later versions it (correctly) refuses to write images with alpha.

Ah okay, missed this in your original description. I just pulled and tested everything again and it looks good to me!

@nick-paul nick-paul merged commit c45ab4a into aya-lang:master Nov 14, 2024
nick-paul added a commit to nick-paul/aya-digit that referenced this pull request Dec 2, 2024
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