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

docs: Add more documentation for video transcoding settings. #13388

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kevincox
Copy link
Contributor

@kevincox kevincox commented Oct 12, 2024

This adds documentation on many of the video transcoding settings ffmpeg.*. I focused the documentation on values that aren't just passthough to ffmpeg settings but that are custom to Immich.

server/src/enum.ts Outdated Show resolved Hide resolved
server/src/enum.ts Outdated Show resolved Hide resolved
server/src/enum.ts Outdated Show resolved Hide resolved
server/src/enum.ts Outdated Show resolved Hide resolved
@mmomjian
Copy link
Contributor

I feel like this should be written up in the Immich docs, not as comments in a file.

Copy link
Member

@danieldietzler danieldietzler left a comment

Choose a reason for hiding this comment

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

I agree, this shouldn't be a comment in the code as people who read the code typically know how to figure it out themselves anyways. If anything, this should help people figuring out the correct setting in the GUI

@mertalev
Copy link
Contributor

mertalev commented Oct 12, 2024

It can go in the transcode policy description in a more condensed form. Also, the use of "optimal" as the resolution-based policy has always bothered me. Calling it "resolution" would be easier to understand.

@kevincox
Copy link
Contributor Author

Can you clarify where you would like to see this?

@kevincox
Copy link
Contributor Author

kevincox commented Oct 12, 2024

the use of "optimal" as the resolution-based policy has always bothered me. Calling it "resolution" would be easier to understand.

A nice improvement would be to separate out resolution and bitrate into flags rather than presets. Right now you can pick either but not both, example (I would like to transcode if either resolution or bitrate is over the preference). Or maybe it would be better to specify this similar to video formats where there is allowed and the target when transcoding specified separately. So you would have transcode policy Always, Required, Never then have targetBitrate, maxBitrate and targetResolution, maxResolution. But that is a bigger change than just documenting the current state.

@mertalev
Copy link
Contributor

In the video transcoding settings in the admin UI.

@kevincox
Copy link
Contributor Author

Hmm, the problem with putting it there is that if you are setting the values from the config file you can't actually see the descriptions of individual settings.

Screencast.from.2024-10-12.13-03-32.webm

It also doesn't list the settings "keyword". Maybe we need some sort of documentation that can appear both in the UI and in some place for people using the config file?

@mertalev
Copy link
Contributor

mertalev commented Oct 12, 2024

I agree setting them as toggles would be nice to mix and match. It'd implicitly document how it works as well, such as having toggles for HDR etc.

Something like this, perhaps:

"videosToTranscode": {
    "hdr": true,
    "nonAcceptedCodec": true,
    "aboveTargetResolution": false,
    "aboveMaxBitrate": false
}

@mertalev
Copy link
Contributor

Hmm, the problem with putting it there is that if you are setting the values from the config file you can't actually see the descriptions of individual settings.

Screencast.from.2024-10-12.13-03-32.webm

It also doesn't list the settings "keyword". Maybe we need some sort of documentation that can appear both in the UI and in some place for people using the config file?

Hmm, I guess putting in the docs and linking to it in the admin UI would be best. There'd be more room to explain config file usage that way. It's okay to put it in the section you mentioned in the OP for the scope of this PR, but in the future it'd be nicer to have a separate transcoding page instead.

@github-actions github-actions bot added the documentation Improvements or additions to documentation label Oct 14, 2024
@kevincox kevincox changed the title Document video transcode policy Add more documentation for video transcoding settings. Oct 14, 2024
@kevincox kevincox changed the title Add more documentation for video transcoding settings. docs: Add more documentation for video transcoding settings. Oct 26, 2024
Copy link
Contributor

github-actions bot commented Oct 26, 2024

📖 Documentation deployed to pr-13388.preview.immich.app

@alextran1502
Copy link
Contributor

Thank you for the PR. However, the subtitles in the settings are self-explanatory, and breaking them down in the documentation will cause a mismatch when we change them, creating a maintenance burden. I will close the PR for the reason above.

@kevincox
Copy link
Contributor Author

The settings UI is one thing but there is currently zero documentation for the config file. I had to read the source to figure it out which I assume is not considered acceptable UX.

@alextran1502 alextran1502 reopened this Oct 29, 2024
@alextran1502
Copy link
Contributor

@kevincox ah that is fair, sorry I misunderstood the purpose of the PR

docs/docs/administration/system-settings.md Show resolved Hide resolved
docs/docs/administration/system-settings.md Outdated Show resolved Hide resolved
docs/docs/administration/system-settings.md Outdated Show resolved Hide resolved
docs/docs/administration/system-settings.md Outdated Show resolved Hide resolved
docs/docs/administration/system-settings.md Outdated Show resolved Hide resolved
docs/docs/administration/system-settings.md Outdated Show resolved Hide resolved
docs/docs/administration/system-settings.md Show resolved Hide resolved
Copy link
Contributor

@mertalev mertalev left a comment

Choose a reason for hiding this comment

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

A few minor nits, otherwise looks good!

docs/docs/administration/system-settings.md Outdated Show resolved Hide resolved
docs/docs/administration/system-settings.md Outdated Show resolved Hide resolved
docs/docs/administration/system-settings.md Outdated Show resolved Hide resolved
This adds documentation on many of the video transcoding settings `ffmpeg.*`. I focused the documentation on values that aren't just passthough to ffmpeg settings but that are custom to Immich.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:documentation documentation Improvements or additions to documentation 🗄️server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants