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

Encode Operation #6

Merged
merged 8 commits into from Oct 2, 2022
Merged

Encode Operation #6

merged 8 commits into from Oct 2, 2022

Conversation

ashwani-rathee
Copy link
Collaborator

@ashwani-rathee ashwani-rathee commented Aug 23, 2022

This intends to solve the encoding operation for the gif file in Julia

  • Need to benchmark it
  • Memory usage and time taken is concerning
  • Ensure the file gets closed properly and it's safe against segmentation faults(so many different types occured while learning about this encoding part)
  • Local colormaps usage
  • Optimize the quantization process, right now using Noise.jl
  • Lots of tests

After first commit, it's no good for us

julia> @btime gif_encode("test1.gif", img) # from gifimages
  23.328 ms (181010 allocations: 8.33 MiB)

julia> @btime save("test.gif", img) # from imagemagick
  9.790 ms (52 allocations: 524.83 KiB)

Quantization, Unique functions are slow, might need to write quantization algorithm that return quantized image and colormap used. or something useful can be found in ImageSegmentation.jl. Makes sense to use staticarrays.

Some improvements in allocations after 2, use of inplace methods and removal of redundant functions helps :

julia> @btime gif_encode("test1.gif", img)
  17.782 ms (3218 allocations: 343.05 KiB)

But quantization and unique alone contribute 11 ms to gif_encode.

src/encode.jl Outdated Show resolved Hide resolved
# possible error likely here to return palette kwarg
octreequantisation!(img1; kwargs...)
return img1
end
Copy link
Member

Choose a reason for hiding this comment

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

This seems perfectly a case that can be implemented in a separate standalone package so that other packages (e.g., DitherPunk) can benefit from it.

cc: @adrhill

Copy link

Choose a reason for hiding this comment

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

Yes, this would be very useful and I'd be very interested in contributing to (or writing) such a package. :)

Copy link
Member

Choose a reason for hiding this comment

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

A small discussion on slack: because this package along with the other two https://github.com/ashwani-rathee/ExifViewer.jl and https://github.com/ashwani-rathee/JPEG2000.jl are part of GSoC 22', we'll quickly get this PR merged and then task switch to JPEG2000. So these quantization methods will be temporarily kept private to GIFImages.

Just opened an issue so that we don't forget this #7

@ashwani-rathee
Copy link
Collaborator Author

This is almost done for now, I'll merge this after few more checks and fixes tomorrow

Copy link
Member

@johnnychen94 johnnychen94 left a comment

Choose a reason for hiding this comment

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

If x86 persists to be an issue, we can skip it (but leave an issue to notify potential users about this situation). Since it doesn't block this PR, I didn't check the tree quantisation part, but it does look like a lot of places can be optimized (keyword: reducing memory allocation).

Just ensure that we're using IO safely -- always have a close operation for each open operation.

src/encode.jl Outdated Show resolved Hide resolved
src/encode.jl Outdated Show resolved Hide resolved
src/encode.jl Outdated Show resolved Hide resolved
src/quantizers.jl Outdated Show resolved Hide resolved
src/quantizers.jl Outdated Show resolved Hide resolved
src/encode.jl Outdated Show resolved Hide resolved
@ashwani-rathee
Copy link
Collaborator Author

Currently we are merging this to move forward with JPEG2000 task, quantizer are mostly optmized(have spent quite a bit of time on them already), albeit more time could be spent later.

@ashwani-rathee ashwani-rathee merged commit bef133b into main Oct 2, 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.

None yet

3 participants