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

frontend: Log streaming service recommended maximums #11733

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

Conversation

prgmitchell
Copy link
Member

@prgmitchell prgmitchell commented Jan 16, 2025

Description

Log the maximum recommended audio and video bitrate when the user ticks the "Ignore streaming service setting recommendations" box.

image

Remake of #10917 because I broke something when rebasing and the commit new commit is not showing.

Motivation and Context

Mentioned by @Warchamp7 that we should warn when a user is going above the recommended bitrate in the loganalyzer, this logging is a requirement to make that happen.

How Has This Been Tested?

Tested Twitch and Youtube in both simple and advanced output mode, both showed the correct values.
Tested with "Custom" to confirm it is showing "None" for audio and video bitrate maximums when those values are returning 0.
https://obsproject.com/logs/ZuadI85OgoP0Yjjj

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.

@prgmitchell prgmitchell force-pushed the logServiceMaximums branch 3 times, most recently from 81d15e2 to fa436f5 Compare January 17, 2025 16:11
frontend/utility/AdvancedOutput.cpp Outdated Show resolved Hide resolved
frontend/utility/AdvancedOutput.cpp Outdated Show resolved Hide resolved
frontend/utility/SimpleOutput.cpp Outdated Show resolved Hide resolved
frontend/utility/SimpleOutput.cpp Outdated Show resolved Hide resolved
Log the maximum recommended audio and video bitrate when the
user ticks the "Ignore streaming service setting recommendations" box.
@prgmitchell
Copy link
Member Author

@PatTheMav updated according to your review comments. FYI I have gotten slightly mixed messages around some of this from comments on the previous PR that was approved but your suggestions all made sense.

@PatTheMav
Copy link
Member

PatTheMav commented Jan 17, 2025

@PatTheMav updated according to your review comments. FYI I have gotten slightly mixed messages around some of this from comments on the previous PR that was approved but your suggestions all made sense.

No worries, we're in a transitory phase away from "prior art" and towards "new rules", particularly with the frontend refactor PR merged that aims to establish some of those rules. It'll be a bit like pulling teeth for the foreseeable future to get rid of old habits among contributors. 😅

@WizardCM WizardCM added the Enhancement Improvement to existing functionality label Jan 19, 2025
Copy link
Member

@RytoEX RytoEX left a comment

Choose a reason for hiding this comment

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

Leaving a question about the approach here.

Comment on lines +207 to +217
std::string videoBitRateLogString = maxVideoBitrate > 0 ? std::to_string(maxVideoBitrate)
: "None";
std::string audioBitRateLogString = maxAudioBitrate > 0 ? std::to_string(maxAudioBitrate)
: "None";

blog(LOG_INFO,
"User is ignoring service bitrate limits.\n"
"Service Recommendations:\n"
"\tvideo bitrate: %s\n"
"\taudio bitrate: %s",
videoBitRateLogString.c_str(), audioBitRateLogString.c_str());
Copy link
Member

Choose a reason for hiding this comment

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

I wonder a bit if we're not overcomplicating this. We could instead:

			blog(LOG_INFO,
			     "User is ignoring service bitrate limits.\n"
			     "Service Recommendations:\n"
			     "\tvideo bitrate: %d\n"
			     "\taudio bitrate: %d",
			     maxVideoBitrate, maxAudioBitrate);

Or are maxVideoBitrate and maxAudioBitrate not guaranteed to be zero if no value is specified? Is there value in distinguishing 0 from None?

Copy link
Member Author

@prgmitchell prgmitchell Jan 23, 2025

Choose a reason for hiding this comment

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

I had originally done it this way actually but to me it seemed to read a little more awkward/unclear if we were to log a max value as 0 when it is actually meant that there is no max recommended value at all.

Copy link
Member

Choose a reason for hiding this comment

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

I'm willing to wait a bit for a other opinions. It just jumped out to me at first glance that it seemed like extra work here, and I wasn't sure how much extra clarity it granted. cc @Warchamp7

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
Status: No status
Development

Successfully merging this pull request may close these issues.

4 participants