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

Only show IsAVC for video streams #6002

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

Only show IsAVC for video streams #6002

wants to merge 2 commits into from

Conversation

Bond-009
Copy link
Member

No description provided.

@Bond-009 Bond-009 requested a review from a team as a code owner August 30, 2024 18:29
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

ESLint doesn't pass. Please fix all ESLint issues.

src/components/itemMediaInfo/itemMediaInfo.js Outdated Show resolved Hide resolved
Copy link

@jellyfin-bot
Copy link
Collaborator

jellyfin-bot commented Aug 30, 2024

Cloudflare Pages deployment

Latest commit 32906e8
Status ✅ Deployed!
Preview URL https://4798a917.jellyfin-web.pages.dev
Type 🔀 Preview

View build logs

@@ -107,7 +107,7 @@ function getMediaSourceHtml(user, item, version) {
if (stream.CodecTag) {
attributes.push(createAttribute(globalize.translate('MediaInfoCodecTag'), stream.CodecTag));
}
if (stream.IsAVC != null) {
if (stream.Type === 'Video' && stream.IsAVC != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Although this change looks fine, I see that IsAVC is supposed to be nullable. Did we accidentally set it for non-video streams?

Copy link
Member Author

Choose a reason for hiding this comment

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

At least on my install, yes

Copy link
Contributor

@dmitrylyzo dmitrylyzo Aug 30, 2024

Choose a reason for hiding this comment

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

It probably becomes false here https://github.com/jellyfin/jellyfin/blob/72077490447d3ff588563cc7f8aa705e1a7e9ecc/Emby.Server.Implementations/Data/SqliteItemRepository.cs#L5627
TryGetBoolean doesn't seem to return null if the value in the DB is NULL.
Wouldn't result be assigned the value false instead of null?
https://github.com/jellyfin/jellyfin/blob/72077490447d3ff588563cc7f8aa705e1a7e9ecc/Emby.Server.Implementations/Data/SqliteExtensions.cs#L151

UPD:
Changed in jellyfin/jellyfin@1027792 after your request.
IsAVC is nullable, but never assigned to null from the DB, because TryGetBoolean doesn't accept nullable.

Copy link
Member Author

Choose a reason for hiding this comment

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

TryGetBoolean doesn't seem to return null if the value in the DB is NULL.
Wouldn't result be assigned the value false instead of null?

No, it doesn't assign any value as the Try method returns false bc it's null inside of the DB.
We don't have to assign anything as the default value is null

Copy link
Contributor

@dmitrylyzo dmitrylyzo Sep 13, 2024

Choose a reason for hiding this comment

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

This means that we saved false in the database. I checked my database - not a single NULL (IsAvc column).

Could it be that is_avc in the ffprobe output is always set (not null)?
https://github.com/jellyfin/jellyfin/blob/2f602deb5a2ba3306e94c660173de273ee12a742/MediaBrowser.MediaEncoding/Probing/MediaStreamInfo.cs#L235

Copy link
Member

Choose a reason for hiding this comment

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

@Bond-009 any thoughts on this?

Copy link

sonarqubecloud bot commented Jan 6, 2025

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.

4 participants