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 option that caches video thumbnails to disk #2197

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

zjupure
Copy link
Contributor

@zjupure zjupure commented Aug 25, 2018

Motivation

As #2171 comments, Fresco loads a thumbnail from local video URI without disk cache. Every time restart app or bitmap in memory cache are evicted, then Fresco need decode thumbnail from video again and it will be slow and poor performance. So put the thumbnail bitmap into disk cache will be a good choice.

To optimize for this case, I create a LocalVideoThumbnailProducer2 class that will return an EncodedImage, and then it can be consumed by DiskCacheWriteProducer and EncodedMemoryCacheProducer. We can build a new producer sequence for this case, see ProducerSequenceFactory#getLocalVideoFileFetchSequence2().

Of course, it is a feature that you can enable/disable it by ImageRequestBuilder, but I think it should be enabled by default.

Test Plan

put a video file test.mp4 in sdcard and got the READ_EXTERNAL_STORAGE permission, and use following code to test.

File file =  new File(Environment.getExternalStorageDirectory(), "test.mp4");
draweeView.setImageURI(Uri.fromFile(file));

@erikandre
Copy link
Contributor

Hi @zjupure

Thank you for your contribution! :)
The idea is good but I think we should be able to do this without creating a separate copy of LocalVideoThumbnailProducer and instead adding this functionality to the existing producer.
What do you think?

@zjupure
Copy link
Contributor Author

zjupure commented Aug 30, 2018

@erikandre

Hi, creating a separate copy of LocalVideoThumbnailProducer is really a bit ugly and the duplicated code might be extracted to an base abstract class.

I think need to create new class not modify the existing class are based on following considration.

  • The existing LocalVideoThumbnailProducer are return CloseableImage but the new feature requires it return an EncodedImage.
  • If just change the return type of LocalVideoThumbnailProducer, and it will change the whole producer sequence whether the user disable or enable this feature.
  • Existing producer sequence: bitmap cache ---> xxx ---> LocalVideoThumbnailProducer (CloseableImage)
  • New feature producer sequence: bitmap cache ---> DecoderProducer ---> DiskCacheXXXProduer ---> EncodedMemoryCacheProducer ---> LocalVideoThumbnailProducer(EncodedImage)
    the new producer sequence work flow: video --> thumnailb bitmap (decoded) ---> inputstream ---> bitmap. First time without disk cache, it will decoed bitmap twice.
  • If change the existing sequence to this, and user disable the feature, bitmap decoding twice will occur in many cases. In fact, if user disable disk cache, we only need decode once from video file directly.

So I think when user enable this feature and we should build a new producer sequence. But if user disable it using the exsting producer sequence would be a better choice. @erikandre How do you think for this case?

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

erikandre has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants