Skip to content

UI: Consistently reference the software H264 encoder properly#8015

Closed
Conan-Kudo wants to merge 1 commit intoobsproject:masterfrom
Conan-Kudo:fix-x264-encoder-name
Closed

UI: Consistently reference the software H264 encoder properly#8015
Conan-Kudo wants to merge 1 commit intoobsproject:masterfrom
Conan-Kudo:fix-x264-encoder-name

Conversation

@Conan-Kudo
Copy link
Contributor

@Conan-Kudo Conan-Kudo commented Jan 8, 2023

Description

This change adjusts the encoder string to indicate that it's an H.264 encoder from x264.

Motivation and Context

The code here assumes that the only software encoder is the x264-based H.264 encoder.
That may not always remain true.

How Has This Been Tested?

Built and ran this on Fedora Linux 37 on x86_64.
Observed that strings loaded as they should for the x264 encoder from locale data.

Types of changes

  • Tweak (non-breaking change to improve existing functionality)

Checklist:

  • My code has been run through clang-format.
  • I have read the contributing document.
  • My code is not on the master branch.
  • The code has been tested.
  • All commit messages are properly formatted and commits squashed where appropriate.
  • I have included updates to all appropriate documentation.

@Conan-Kudo Conan-Kudo force-pushed the fix-x264-encoder-name branch 2 times, most recently from 1cbbb73 to 9163ab8 Compare January 8, 2023 03:43
@Conan-Kudo Conan-Kudo marked this pull request as ready for review January 8, 2023 04:10
@Conan-Kudo Conan-Kudo force-pushed the fix-x264-encoder-name branch 2 times, most recently from 4dcb4e7 to de5098d Compare January 8, 2023 04:46
gxalpha
gxalpha previously requested changes Jan 8, 2023
Copy link
Member

@gxalpha gxalpha left a comment

Choose a reason for hiding this comment

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

Please only change the English translation file, the other files will be updated automatically.

@Conan-Kudo
Copy link
Contributor Author

@gxalpha Done.

@Conan-Kudo Conan-Kudo force-pushed the fix-x264-encoder-name branch from de5098d to e8c4ea8 Compare January 8, 2023 13:29
@Conan-Kudo Conan-Kudo requested a review from gxalpha January 8, 2023 14:33
@gxalpha gxalpha dismissed their stale review January 8, 2023 16:37

Requested changes done

@notr1ch
Copy link
Member

notr1ch commented Jan 9, 2023

This feels somewhat unnecessary to me. It's very unlikely there will be a software H264 encoder competitive with x264 any time soon, all R&D has moved on to newer codecs.

@Conan-Kudo
Copy link
Contributor Author

Well, it's also a problem for introducing software VP9/AV1 codecs too.

@Conan-Kudo Conan-Kudo force-pushed the fix-x264-encoder-name branch from e8c4ea8 to fd8c947 Compare January 11, 2023 09:30
@jp9000
Copy link
Member

jp9000 commented Jan 13, 2023

Due to the way the locale system is designed, we may just have to live with the old ID. I realize it's a bit annoying, but it would just needlessly clear out all translations for that string and force translators to retranslate that string. Are you sure there's no other way to handle this situation that doesn't require changing the translation identifier?

@WizardCM WizardCM added the Enhancement Improvement to existing functionality label Jan 14, 2023
@tt2468
Copy link
Member

tt2468 commented Jan 16, 2023

Due to the way the locale system is designed, we may just have to live with the old ID. I realize it's a bit annoying, but it would just needlessly clear out all translations for that string and force translators to retranslate that string. Are you sure there's no other way to handle this situation that doesn't require changing the translation identifier?

Aren't we able to rename translation string IDs in crowdin without wiping the values?

@Conan-Kudo
Copy link
Contributor Author

@jp9000 I intend to introduce changes to add more software encoders, particularly ones that I can ship in Fedora out of the box. Ideally, I'd like to eliminate the assumption the "one" software codec is x264 especially since it won't be in the near future. I'm considering implementing OpenH264 support since Fedora and openSUSE have arrangements with Cisco1 2 to support that. Additionally, I am looking to expose ffmpeg-aom as a software codec in the UI too.

In an ideal world, everything would be going through obs-ffmpeg and this would be easy. 😢

The code here assumes that the only software encoder is the x264-based
H.264 encoder. That may not always remain true. This change adjusts
the encoder string to indicate that it's an H.264 encoder from x264.
@Conan-Kudo Conan-Kudo force-pushed the fix-x264-encoder-name branch from a61d43a to 3a260f1 Compare March 26, 2023 12:40
@Conan-Kudo
Copy link
Contributor Author

I'm consolidating this pull request into #8529 per the advice of @pkviet. Thanks folks!

@Conan-Kudo Conan-Kudo closed this Mar 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Enhancement Improvement to existing functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants