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

Update existing feature #2244

Closed
VampiricAlien opened this issue Nov 6, 2022 · 17 comments · Fixed by #2245
Closed

Update existing feature #2244

VampiricAlien opened this issue Nov 6, 2022 · 17 comments · Fixed by #2245
Labels
enhancement New feature or request good-first-issue Good for newcomers

Comments

@VampiricAlien
Copy link

Describe the feature you'd like

On the item view page next to the date has tags based on the media file [1080] [cc] [5.1] and so on. Can we add AV1 to that list of tags (If already there, extend to 6 tags and give priority to media codec) and would it be possible to make the 4K tag gold? this would make it stand out. I got lost in the code trying to look for the right area.

[AV1] [4K] [ACC] [EAC3] [ [CC] [MA 15+] (media file that's AV1 with 2 audio tracks and is 4K)

@nielsvanvelzen You'll be best person to ask about this since you've built most of this app. No rush.

@VampiricAlien VampiricAlien added the enhancement New feature or request label Nov 6, 2022
@nielsvanvelzen
Copy link
Member

This part adds the tags for the audio stream

private static void addMediaDetails(Context context, MediaStream stream, LinearLayout layout) {
if (stream != null) {
if (stream.getCodec() != null && stream.getCodec().trim().length() > 0) {
String codec = stream.getCodec().equals("dca") || stream.getCodec().equals("DCA") ? "DTS" : stream.getCodec().equals("ac3") || stream.getCodec().equals("AC3") ? "Dolby" : stream.getCodec().toUpperCase();
addBlockText(context, layout, codec);
addSpacer(context, layout, " ");
}
if (stream.getChannelLayout() != null && stream.getChannelLayout().trim().length() > 0) {
addBlockText(context, layout, stream.getChannelLayout().toUpperCase());
addSpacer(context, layout, " ");
}
}
}

Which is called in

addMediaDetails(context, audioStream, layout);

It should be fairly easy to add another method to add video codec information like you suggested.

@nielsvanvelzen nielsvanvelzen added the good-first-issue Good for newcomers label Nov 7, 2022
@VampiricAlien
Copy link
Author

I was going to try a do a pull request based on that code but I know nothing about JAVA/Kotiin and would only get in your way.

Is it also possible to add colour to the tags?

@The-Randalorian
Copy link
Contributor

I have a basic form of the encoding tags working. This is my first issue, so I have to figure out how to submit my changes, but here is a picture

image

I switched the order from encoding then resolution to resolution then encoding, as I personally feel resolution is of higher priority. This is an easy switch if people want it the other way.

I have not done any coloring because I think there should be some discussion about what colors should be used for certain resolutions. For example, should we maybe make SD a dark orange/bronze, make 720/1080 a lighter gray/silver, and then make 1440/4k gold?

@nielsvanvelzen
Copy link
Member

The addBlockText method sets the background (defaults to R.drawable.block_text_bg). But I don't think coloring those items will look good, it will probably just clutter the UI even more.

This is my first issue, so I have to figure out how to submit my changes

You can create a pull request that targets the master branch and we will review it.

@VampiricAlien
Copy link
Author

@The-Randalorian Two small issues
Too little/big space between tags, [TV-MA] [1080] [HEVC] [CC] [ACC] (not spaced evenly)
Sometimes a tag is cut off like: [TV-MA] [1080] [HEVC] [CC] [ACC] [ster

Discussion
Changing 720 to HD, 1080 to FHD, 2160 to UHD and so on, I this this will look cleaner.

@VampiricAlien
Copy link
Author

Changing 720 to HD, 1080 to FHD, 2160 QHD and so on, I this this will look cleaner.

@nielsvanvelzen Would this be easy to change/add, I know it's not on your list, given that video code has been added why not complete this area is what I am thinking.
Below 720p shows as SD

@nielsvanvelzen
Copy link
Member

Personally I prefer "720p" over "SD" but if someone is willing to create a PR for this change we can look into it for 0.16.

@VampiricAlien
Copy link
Author

I don't see much point unless other feel the same way to change it. I'll leave it for now.

@The-Randalorian
Copy link
Contributor

@VampiricAlien The different spacing was based off of the existing different spacing, which appears to be between different groups of tags.

I never noticed tags getting cut off myself, and I don't see any reason the code changes I made should cause them (no data is altered or removed, only added).

Perhaps there is a length limit elsewhere that needs to be investigated.

@VampiricAlien
Copy link
Author

@The-Randalorian

Just notice that the spacing is per group [1080] [HEVC] [CC] [AAC] [Stereo] (Video then subs then audio)
I kind of looks messy like there is something wrong when their isn't.

I never noticed tags getting cut off myself, and I don't see any reason the code changes I made should cause them (no data is altered or removed, only added).

I have also seen a blank tag at the end, however I can not find any media with a blank or cut off tag. May it was just me?

@VampiricAlien
Copy link
Author

@The-Randalorian thoughts on adding HDR as an important tag after the 4K tag if available?

@VampiricAlien
Copy link
Author

@The-Randalorian are you still working on this?

@The-Randalorian
Copy link
Contributor

@The-Randalorian thoughts on adding HDR as an important tag after the 4K tag if available?

I am still here, but I missed your message before.

It must be possible, since the web interface displays it. That being said, I expect it to be significantly more difficult to implement than the codec tag; the codec was already readily available as a string in the existing code, but I don't recall seeing something similar for HDR.

@VampiricAlien
Copy link
Author

Was this supposed to have HDR displayed? #3205

@VampiricAlien
Copy link
Author

I was looking at the tags and wondered if when there are Hearing Impaired subtitles available could SDH be shown in place of CC?

Closed Captions (CC): Text overlays on a video that provides dialogue and auditory cues intended to make content accessible for deaf or hard-of-hearing audiences.

The reason being is that the app uses CC when any kind of subtitles are available and doesn't help the deaf community know when subtitles are for them or not.

@guiweber
Copy link
Contributor

Was this supposed to have HDR displayed? #3205

No, that just ensures the codec tag remains displayed if an HDR tag is displayed. Previously, displaying an HDR tag would remove the codec tag.

@nielsvanvelzen
Copy link
Member

I was looking at the tags and wondered if when there are Hearing Impaired subtitles available could SDH be shown in place of CC?

This information is currently not available until 10.9 (jellyfin/jellyfin#7379). Please open a separate issue if you want to see this in the app.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good-first-issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants