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

fix default settings overwriting template settings #107

Closed

Conversation

toufali
Copy link

@toufali toufali commented Oct 3, 2020

Description of changes:
It appears the hard-coded default settings overwrite the templates created in MediaConvert. For example, specifying a MediaConvert CMAF template with segment length of 5 seconds gets overwritten by the CMAF default of 30 seconds.

It appears code was added to handle the case of custom template settings, however, this should apply to all templates specified in MediaConvert. MediaConvert template settings should take precedence over hard-coded settings in the encode lambda. By removing the conditional, this has the desired effect for my use-case.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@tomnight
Copy link
Contributor

tomnight commented Oct 5, 2020

Thanks for this, makes total sense. We will review the PR in the next release.

@morjoan
Copy link
Member

morjoan commented Dec 18, 2020

Fix has been put in version 5.2 to avoid overwriting template settings. Thanks for the feedback.

@tomgilman tomgilman closed this Feb 25, 2021
@RaeesBhatti
Copy link

@tomgilman I think there's been a slight oversight here. The function call to mergeSettingsWithDefault provides 3 parameters but the function expects only 2. This results in custom group always being ignored because it's the third parameter.

@eggoynes
Copy link
Member

Hi @RaeesBhatti

Thank you for pointing this out. We can fix that in our next release.

@jilladams
Copy link

jilladams commented Sep 13, 2021

Tracking the remaining change to fix that function in a new issue.

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.

7 participants