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 for extracting a series of video thumbnails #146

Merged
merged 24 commits into from
Jan 24, 2022

Conversation

ReallyVasiliy
Copy link
Collaborator

This PR adds support for extracting thumbnails/frames from video as Bitmaps, while applying an optional set of filters.

VideoThumbnailExtractor serves as the entry point, and requires ThumbnailExtractParameters which defines the extraction behavior and other parameters.

One behavior, MediaMetadataExtractionBehavior (based on MediaMetadataRetriever), is included. The intent is that other behaviors can be added by consumers -- for example, consumers might opt to provide a custom FFmpeg-based or MediaCodec-based extractor, which may yield performance or functional improvements over MediaMetadataRetriever, while still retaining the ability to apply video filters to extracted frames.

Demo app has an example of creating a "film strip":

Screenshot_1636489276

* Optional size of the generated thumbnail Bitmap. The video frame will be resized and scaled to fill this [destSize], preserving the video frame's
* original aspect ratio.
*/
val destSize: Point? = null,
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we are preserving original aspect ratio, passing a resolution (smaller dimension) should be better. What happens when destSize's aspect ratio is different from video's? Or should we just scale the video to fit destSize like when we do in transcoding?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let me add this image just so we're using the same language around this
image

passing a resolution (smaller dimension) should be better

Can you clarify what you mean by resolution? Do you mean we would perform "aspect fit" scaling instead of "aspect fill"?

What happens when destSize's aspect ratio is different from video's?

Right now it will scale with "aspect fill": the video frame will be resized and cropped to fill the destSize area, and the aspect ratio of the video frame is preserved.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, do you mean we should do this? --

  1. the resulting bitmap will always have the same aspect ratio as the source video (i.e. we won't add any whitespace/padding)
  2. instead of "destination size", we pass in something like "maximum width" and "maximum height"

Copy link
Contributor

Choose a reason for hiding this comment

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

Originally, I was thinking of doing this:

  • thumbnail bitmap will have the aspect ratio of video
  • by default, thumbnail will also have a resolution of source video (true frame preview)
  • optionally we can pass the "resolution" parameter, thumbnail bitmap's smaller dimension will be equal to resolution. This is closer to how video is defined on Android - resolution + rotation in 90 degree increments.

For thumbnail use case, it is probably reasonable to aspect fill (AKA center crop) the thumbnail, as long as it is clearly defined in the API.

…escriptive about how this should be used

Remove unused imports
Add OptIn annotations to demo classes using the experimental frame extract feature
/**
* Provides the request lifecycle for extracting video frames. The specifics of extraction work are delegated to [FrameExtractBehavior]s.
*/
@ExperimentalFrameExtractorApi
Copy link
Contributor

Choose a reason for hiding this comment

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

This is exposed externally? We should make these internal
Note to self: In general, we should change our build config to enforce explicit access qualifiers in Kotlin classes, to prevent clients from using stuff that is not meant to be exposed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah good catch, done. I also made the executor/task related classes internal (they were originally being injected so they were open)

import com.linkedin.android.litr.frameextract.FrameExtractMode
import com.linkedin.android.litr.frameextract.FrameExtractParameters

@ExperimentalFrameExtractorApi
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need this here? Does this get inherited from parent interface?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I believe we still need this -- basically this is propagating the opt-in requirement to whoever is using this concrete behavior. If I leave this out, we'll get warnings in this class that it's using an experimental API. So we have two choices: opt in, or propagate. If we opt in here, then callers won't know this class is using experimental APIs. So instead we propagate the requirement. https://kotlinlang.org/docs/opt-in-requirements.html#propagating-opt-in

@ReallyVasiliy ReallyVasiliy merged commit 35554f5 into linkedin:main Jan 24, 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.

2 participants