bumptech / glide

An image loading and caching library for Android focused on smooth scrolling
https://bumptech.github.io/glide/
Other
34.59k stars 6.12k forks source link

How to force Glide to use only the VideoDecoder #4542

Open perracodex opened 3 years ago

perracodex commented 3 years ago

Glide Version: 4.11, 4.12

Device/Android Version: Different Huawei models

Issue details / Repro steps / Use case background:

Is there a way to force a Glide request to use only the VideoDecoder to obtain thumbnails, when a file's media type is known to be a video?

My question is because I need to find a temporary workaround to avoid the crashes reported in issues #4165 and #4519, until these get fixed. To put in context, and avoid repeating everything that is already explained in the above tickets, the bug is an uncatchable system crash which happens in different Huawei models when trying to get thumbnails from large video files (over 2GB).

Debugging Glide I've found that in its flow it also calls the BitmapFactory.decodeFileDescriptor for video files via the Downsampler class, to be more exact, the crash happens when trying to obtain the video thumbnail dimensions.

To avoid the crash, I guess that forcing to use the VideoDecoder when the type is known, would mitigate temporarily the issue.

See next screenshot for the frames stacktrace. Is from when reading a video file. As you can see marked in red, to get the video dimensions Glide is calling the ImageReader.decodeBitmap method, which content maps to a single BitmapFactory.decodeFileDescriptor call. Glide shouldn't be calling such API to get video dimensions, as it is mean to be for image files only. So, even if doesn't crash in several devices, the behavior is unpredictable as is passing the file descriptor of an unsupported format.

image

perracodex commented 3 years ago

Update: Seems the issue was introduced in 4.11.0 when merging the next change, which doesn't take into consideration that video formats shouldn't go through the BitmapFactory

https://github.com/bumptech/glide/commit/9281d8ed45591279b5fe646b136c956624a5da24

sjudd commented 3 years ago

Glide doesn't actually know the format of anything we decode (imagine being asked to decode a file for example).So Glide just walks through the decoders in the order of registration and attempts each one in turn. We generally prefer images to videos because images are more common, both because they're more frequently captured than videos and also because we cache stills in Glide's disk cache.

It might be a nice optimization to only try the video decoder, or try the video decoder first, when we do have enough information to determine that it is a video decoder.

That said, the bug is really just the broken changes made to BitmapFactory on these specific devices. I'm not sure there's a reliable way to workaround it in all cases.

sjudd commented 3 years ago

b/187987067 is an internal version of this issue

sjudd commented 3 years ago

Another awkward bit - BitmapFactory has APIs that would give us a mime type, which we could more practically use to avoid trying to decode the entire file.

Unfortunately that's the API that getDimensions uses, so it seems like that's not an option. We'd need to know ahead of time prior to calling BitmapFactory that we shouldn't attempt to decode the item. Either we'd have to guess based on the input, probably by checking if the media store uri type is video, or we'd have to write our own parser that recognizes the headers of common video files.

perracodex commented 3 years ago

I am currently using a temporary workaround, but which isn't a proper solution at all, as it only works when the media types are known beforehand. As In my case the media files details come from the MediaStore, I always know all the media types. So I created/registered a custom decoder which internally calls directly Glide's VideoDecoder. When I detect that a target file is a video, I ensure the custom decoder is used.

If this issue has no solution, then a way to give an optional mitigation, would be to add an option to the GlideRquest which allows forcing the VideoDecoder. But this would be more like a patch rather than a solution. If glide could somehow resolve beforehand the media type then all would be solved.

I am curious about the changes introduced in https://github.com/bumptech/glide/commit/9281d8ed45591279b5fe646b136c956624a5da24 which make the problem happen, as in previous versions before 4.11 works perfectly.

My guess, and it is only a guess, is that for all affected devices, probably the BitmapFactory native implementation is trying to find a marker by reading the image files in order to resolve the image type. As the crash only happens with very large video files (which type isn't handled by BitmapFactory), then may be it keeps on reading until it exceeds some limits and therefore crashing. This idea would explain the reported errors in the other issue tickets. But is just guess as I didn't dig up in the native implementation.

sjudd commented 3 years ago

Judging from the error message internally, it's actually just a corrupt file: 'FORTIFY: read: count 18446744071615423932 > SSIZE_MAX'. If we knew SSIZE_MAX would exclude files that we knew were larger than it, but it's probably not a complete fix due to the corrupt file issue.

perracodex commented 3 years ago

Another option, if a solution is not feasible, then would be to implement some heuristics before deciding how to decode a file.

For example, as the issue occurs only with videos, and more exactly on large videos files, then before making a decoding choice, when the file size is known, first try for videos or images according to such size.

It would be a matter of deciding at what file size threshold should first try for video or image decoding. For example, having 64 Megabytes images or larger, while very possible, is uncommon for most use-cases, and would be more common to have images at lesser sizes, so anything above a defined threshold could be first tried for video decoding. Even if some users have exceptionally large images, this would be a few cases against the norm.

Having some heuristics may give some speed optimization when deciding how to decode a file. And for this specific bug it would be a kind workable "patch", if such has no solution.

Probably reading the file header for file types would be the path to go, yet implementing some basic heuristics such choosing based on the file size may add some minimal optimizations.

sjudd commented 2 years ago

Sorry for the super long pause here. I agree, two reasonable paths:

The file type detection may be a little tricky to get right. It could be a bit of a challenge to keep perfectly in sync with the platform. The file types where file descriptor decoding is required for the decode to succeed tend to be large RAW file types anyway, so the cases we have to tackle aren't super simple.