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

Expand video attached image extraction to support multiple images/types #6689

Merged
merged 5 commits into from Oct 21, 2021

Conversation

1337joe
Copy link
Member

@1337joe 1337joe commented Oct 11, 2021

Changes

  • Split VideoImageProvider to create EmbeddedImageProvider: Video for taking a screenshot of a video, Embedded for extracting image attachments.
  • Expanded EmbeddedImageProvider to support multiple embedded image types in the same file: Primary, Background, Logo
  • Added support for changing the extension of extracted images to match the type of the embedded image (important for embedded pngs with transparency, which can't be rendered to jpg).
  • Added tests for VideoImageProvider and EmbeddedImageProvider

Rationale

  • I have movies with the cover embedded and I want to prioritize that over online providers, but I don't want a screenshot from the movie if any other image is available. To match my desired priority order (Embedded > Online > Screenshot) I had to split attachment extraction out of VideoImageProvider into a separate provider. To minimize disruption the default order is for the new embedded provider to come just before the old one, which matches the behavior of the old combined provider.
  • Multiple images and file types can be embedded in an mkv video file, so I added checking against the filename/mimetype to determine the type of image and extension so the basic movie image types (poster, background, logo) can be embedded in a movie file.

Comment on lines 585 to 586
if (!string.Equals(streamInfo.CodecType, "attachment", StringComparison.OrdinalIgnoreCase) &&
!(streamInfo.Disposition != null && streamInfo.Disposition.GetValueOrDefault("attached_pic") == 1))
Copy link
Member Author

Choose a reason for hiding this comment

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

Attached images in mkv files are currently parsed as MediaStreams with MediaStreamType.EmbeddedImage (see line 738 of this file). To determine the type of an attached image I needed the filename and/or mimetype fields, which are only stored for MediaAttachments.

This was the minimal change to store the available metadata in the current structures, but it results in storing attached images in both the MediaAttachment and MediaStream lists. If there is a better way to store the data I'm open to suggestions.

@1337joe 1337joe marked this pull request as ready for review October 11, 2021 12:33
Comment on lines 498 to 510
string validatedOutputExtension;
if (string.IsNullOrEmpty(outputExtension))
{
validatedOutputExtension = ".jpg";
}
else if (outputExtension.FirstOrDefault() != '.')
{
validatedOutputExtension = "." + outputExtension;
}
else
{
validatedOutputExtension = outputExtension;
}
Copy link
Member

Choose a reason for hiding this comment

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

I think I would put this in ExtractImageInternal and rewrite it as this

            if (string.IsNullOrEmpty(outputExtension))
            {
                outputExtension = ".jpg";
            }
            else if (outputExtension[0] != '.')
            {
                outputExtension = "." + outputExtension;
            }

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry for the slow response, was away for the weekend without electronics.

Rider underlined it when I reassigned a parameter so I thought that was discouraged. I prefer this pattern as well, and I forget why I put it one method earlier in the call chain.

If this completes what you want changed let me know and I'd be happy to squash the review comment commits to clean up the git log.

Copy link
Member

@cvium cvium Oct 21, 2021

Choose a reason for hiding this comment

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

Rider underlined it when I reassigned a parameter so I thought that was discouraged. I prefer this pattern as well, and I forget why I put it one method earlier in the call chain.

I get why it underlines it. It's not nice, but in this case I think a new variable is overkill.

If this completes what you want changed let me know and I'd be happy to squash the review comment commits to clean up the git log.

I don't particularly care, but if you want to squash it, the go ahead (and I see you did).

Clean up style
Fix references in class summaries
Combine Where+FirstOrDefault queries
Break up large method, long lines
Add validation on file extension
Apply test naming conventions
Extract mock of Movie class, comment on why not mocking interface

Co-authored-by: Cody Robibero <cody@robibe.ro>
Co-authored-by: Claus Vium <cvium@users.noreply.github.com>
@cvium cvium merged commit 768ec60 into jellyfin:master Oct 21, 2021
@1337joe 1337joe deleted the expand-image-extraction branch October 21, 2021 22:14
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.

None yet

3 participants