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

Reduce rendering work in UI threads #102

Closed
wants to merge 4 commits into from

Conversation

whistyun
Copy link

refs #51

This PR distributes the GIF decoding process for each frames.
The frame drawing process is performed asynchronously for each frames.

As a result, this PR prevents UI freeze when a large GIF image is set as the AnimatedSource.

NOTE: This PR removes support for net30 and net40.

Behaviour before change
Before

Behaviour after change
After

@thomaslevesque
Copy link
Member

Hi @whistyun,

Thanks for the PR! However, for such a big change, it's better to first discuss it in an issue to ensure we're on the same page. It's going to take me a while to review the PR, and to be honest I don't have a lot of spare time at the moment.

Could you please explain your changes to make it easier to review?

NOTE: This PR removes support for net30 and net40.

Was that necessary to make your changes? I mean, I'm not necessarily against dropping old frameworks, but it should be done for a good reason, and preferably as a separate PR.

@whistyun
Copy link
Author

NOTE: This PR removes support for net30 and net40.

Was that necessary to make your changes? I mean, I'm not necessarily against dropping old frameworks, but it should be done for a good reason, and preferably as a separate PR.

This is necessary to implement parallel operations.
net30 does not have Task class. I removed the old framework to use Task.Run method.

Could you please explain your changes to make it easier to review?

Apologies for sending a big PR without explanation.

The changes are listed below.

  1. move the methods related to frame creation from ImageBehavior.

    Because the processing related to rendering has become lengthy, I moved it from ImageBehavior.cs to DelayFrameAnimation.cs.

  2. create DelayFrameAnimation class instead of ObjectAnimationUsingKeyFrames

    To do asynchronous rendering every frame, a new ObjectAnimationBase has to be created to treat Task<BitmapSource> instead of BitmapSource.

  3. stop using 'RenderTargetBitmap.Render, use WriteableBitmap.WritePixels` instead

    RenderTargetBitmap.Render takes a long time (at least in my environment).
    I removed the use of DrawingContext.DrawImage and replaced with the process of writing byte[] to a WriteableBitmap. (DelayBitmapSource.Draw)
    byte[] is generated by decompresing GifImageData.LzwMinimumCodeSize (DelayBitmapData.Decompress)

  4. stop using BitmapDecoder

    As a result of 3., There is no more information to acquire from BitmapDecoder, so I modified the code to retrieve only GifFile without BitmapDecoder.

@thomaslevesque
Copy link
Member

Hi @whistyun,

Thanks for the explanations. It's a pretty extensive refactoring. To be honest, it makes such fundamental changes that I feel it's no longer really the same library... This isn't necessarily a bad thing in itself, but if I just merged your PR, I would no longer be able to maintain the code, because I would no longer understand it... See my problem here?

So, I'm not going to merge your PR. I know this is frustrating, and I'm sorry. It just changes too many things, and I don't have the time and energy to relearn how it all works, and to debug the new issues that will inevitably arise. If you really need those changes, consider forking the library. As you probably realized, I don't really work on this library anymore, so you won't have trouble keeping up with my changes 😉

I see you actually implemented the LZW decompression (nice work!). If it's fast enough, it might not even be necessary to prepare the frames in advance (which uses a lot of memory), they could just be rendered on the fly. That's what I attempted to do in my other GIF library, but the performance isn't great. Looks like your LZW implementation might perform better (although it could probably be optimized further; it does a lot of allocations that could be avoided). If you don't have an objection, I might try to use it in XamlAnimatedGif (not likely, because again, I don't have enough spare time, but it'd be interesting to see if it improves the performance).

@whistyun
Copy link
Author

whistyun commented May 15, 2023

Thank you for your time. Also sorry for the rude PR.
I will consider forking the library.

@whistyun whistyun closed this May 15, 2023
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