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

Support thread interruption while decoding. #414

Merged
merged 10 commits into from May 26, 2020

Conversation

colinrtwhite
Copy link
Member

@colinrtwhite colinrtwhite commented May 23, 2020

Fixes: #378

An initial attempt at supporting thread interruption while decoding.

The implementation is based on runInterruptible from the Coroutines library. The main difference is we need to prevent interruption while reading from a Source due to this reason.

I didn't add interruption support to VideoFrameFetcher as it seems like even if we interrupt the thread subsequent requests still seem to wait for the previous request to finish. This is likely a hardware limitation.

TODO: Benchmark this to see how much overhead InterruptionSource adds. According to my benchmark it adds negligible (too small to measure) overhead to decoding. In the typical case we're getting and setting a volatile int <20 times which is very fast.

message = "This is an internal Coil API that should not be used from outside of the `coil` package. " +
"No compatibility guarantees are provided."
)
annotation class InternalCoilApi
Copy link
Member Author

Choose a reason for hiding this comment

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

This is necessary to avoid duplicating the interruption code in coil-gif and coil-svg. Maybe withInterruptibleSource can be public API one day, but probably doesn't make sense.

Copy link
Member

@Jawnnypoo Jawnnypoo left a comment

Choose a reason for hiding this comment

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

LGTM. I also would be curious about the overhead of this and its benchmarking against the previous way we did things.

@colinrtwhite
Copy link
Member Author

@Jawnnypoo Ran the change through my benchmarking project and wasn't able to measure a change with enough accuracy so I'd say it has almost no decode time overhead.

@colinrtwhite colinrtwhite merged commit 9b82d84 into master May 26, 2020
@colinrtwhite colinrtwhite deleted the colin/thread_interruption branch May 26, 2020 20:07
colinrtwhite added a commit that referenced this pull request Oct 5, 2022
* Support thread interruption while decoding.

* Update API.

* Handle InterruptedIOException as well.

* Add test.

* Reduce delay.

* Docs.

* Replace AtomicInteger with atomicfu.

* Add to tests as well.

* Updates.

* Revert.
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.

Support thread interruption in cancel
2 participants