-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
feat(server, web): accepted codecs #6460
Conversation
Just wanted to add, that somehow my tests were failing, even before I changed any code, so this must be due to some other commits. But Lint and prettier ran as it should. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just some minor comments
@@ -10,12 +10,14 @@ | |||
export let isEdited = false; | |||
export let number = false; | |||
export let disabled = false; | |||
export let onChange: () => void = () => {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should use dispatch
. See setting-switch for an example of that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, thank you
web/src/lib/components/admin-page/settings/ffmpeg/ffmpeg-settings.svelte
Outdated
Show resolved
Hide resolved
web/src/lib/components/admin-page/settings/ffmpeg/ffmpeg-settings.svelte
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! I found one issue where it was possible to remove the target codec from the accepted codec list. I fixed this by updating config.core.ts to automatically/always add the target codec to the accepted lists. This also made the migration redundant, so I deleted it.
@@ -248,6 +250,14 @@ export class SystemConfigCore { | |||
} | |||
} | |||
|
|||
if (!config.ffmpeg.acceptedVideoCodecs.includes(config.ffmpeg.targetVideoCodec)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this so that the target video/audio codec is always included in the accepted lists.
}, | ||
}); | ||
|
||
savedConfig = { ...result.data }; | ||
config = cloneDeep(newConfig); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line makes it possible so that if the response from " update config" is different than the current config, to display it immediately. Saving "accepted codecs" that doesn't include the target codec, will automatically mark it as accepted after saving.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh true, that's a good catch thanks!
on:select={() => (config.ffmpeg.acceptedAudioCodecs = [config.ffmpeg.targetAudioCodec])} | ||
/> | ||
|
||
<SettingCheckboxes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved these checkboxes to be directly after the codec selection.
This PR is good to go, but it looks like some issue with the mobile build is related to the new enum list, which is used for the first time in this PR. We will need to fix this on main first. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
As mentioned in this feature request, I implemented two new settings under the transcoding settings, which allow the admin to specify which video and audio codecs are acceptable for him, so videos in those codecs will fall under the "desired format" (renamed to "accepted formats") in the policy and thus not be transcoded due to their codec.
If the admin changes the target video or audio codec, the appropriate "accepted codecs" selection will be cleared and only include the newly selected target codec, to keep the existing behaviour intact.