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

obs-browser: Update default size #202

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

Conversation

cg2121
Copy link
Contributor

@cg2121 cg2121 commented Feb 22, 2020

Description

Changes default size to size of canvas.

Motivation and Context

A better default.

How Has This Been Tested?

Created browser source.

Types of changes

  • New feature (non-breaking change which adds 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.

@WizardCM WizardCM added the Enhancement New feature or improvement label Aug 20, 2020
@RytoEX
Copy link
Member

RytoEX commented Mar 12, 2021

I prefer something like this, but it's probably down to personal preference.
https://github.com/RytoEX/obs-browser/tree/change-default-size

@dodgepong dodgepong added this to PRs Pending Review in OBS Studio 28.0 via automation Jun 3, 2021
@dodgepong dodgepong removed this from PRs Pending Review in OBS Studio 28.0 Jun 3, 2021
@dodgepong dodgepong added this to PRs Pending Review in OBS Studio 27.1 via automation Jun 3, 2021
@notr1ch
Copy link
Member

notr1ch commented Aug 23, 2021

Do we even need a v2 here? We're not changing anything except the default value so users shouldn't experience any unexpected issues as the defaults are only used on source creation or reset.

In the event a user does restore the browser source back to defaults (and is thus presumably OK with losing all their settings and getting new defaults), it would be confusing that the defaults of a legacy browser source do not match the defaults of a newly added browser source, with no visible way to distinguish between them.

@WizardCM
Copy link
Member

Yes, v2 is required because the defaults aren't saved on source creation. It's the reason v2 exists at all.

Copy link
Member

@WizardCM WizardCM left a comment

Choose a reason for hiding this comment

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

The feedback I provided in my previous review has not yet been responded to, so I don't believe this should be merged as-is.

@notr1ch
Copy link
Member

notr1ch commented Aug 23, 2021

Ah I see, I didn't realise our defaults worked like that.

obs-browser-plugin.cpp Outdated Show resolved Hide resolved
@WizardCM
Copy link
Member

So, extending what I said above, today I learned that if you create a browser source and click "OK", all settings are saved because of the way the functions in the browser source are written (this should not actually be the case, but it is). So it's safe to assume that any browser made since the v2 rewrite (at least) has the current default resolution saved.

@RytoEX RytoEX removed this from PRs Pending Review in OBS Studio 27.1 Sep 14, 2021
@RytoEX RytoEX added this to PRs Pending Review in OBS Studio 27.2 via automation Sep 14, 2021
@WizardCM WizardCM moved this from PRs Pending Review to PRs Pending Merge (Reviewed) in OBS Studio 27.2 Oct 2, 2021
@tt2468
Copy link
Member

tt2468 commented Oct 7, 2021

I personally have reservations about this. I use OBS on very lightweight machines and browser sources account for most of the resource utilization on them. Making the default size the size of the canvas will end up leading to our users creating larger browser sources than necessary, and therefore increasing the performance footprint of OBS.

If I were to tackle this issue, I would add a way for the content in the browser source itself to announce a "optimal resolution" to obs-browser, and obs-browser could queue an obs_source_update() with the new width/height settings. This would improve user experience and make browser integrations more seamless.

@RytoEX
Copy link
Member

RytoEX commented Oct 7, 2021

I personally have reservations about this. I use OBS on very lightweight machines and browser sources account for most of the resource utilization on them. Making the default size the size of the canvas will end up leading to our users creating larger browser sources than necessary, and therefore increasing the performance footprint of OBS.

If I were to tackle this issue, I would add a way for the content in the browser source itself to announce a "optimal resolution" to obs-browser, and obs-browser could queue an obs_source_update() with the new width/height settings. This would improve user experience and make browser integrations more seamless.

This proposed change is the result of conversations at TwitchCon and in our Discord server where users mentioned that overlay tools expect/encourage you to build the overlay at canvas size or that they build them this way unprompted because that is intuitive. The fact that our default browser source size is neither the canvas size nor even the same aspect ratio, means that they have to change it in the settings manually each time. Resizing the scene item with transform settings produces sub-par results.

It has been suggested before that obs-browser should support meta tags that specify width, height, and FPS. It was just never implemented. See here: https://github.com/obsproject/obs-browser/blob/e54c9f14b3d7c1835853d130dcb135d3cfaf41e8/data/error.html

If you wanted to try to implement something like that, be my guest. Though that wouldn't necessarily help with alerts or overlays unless the providers also then implemented them.

@tt2468
Copy link
Member

tt2468 commented Oct 7, 2021

Fair enough. I do believe in the meta tags idea, so I'll try to find time to work on that.

@jp9000 jp9000 removed this from PRs Pending Merge (Reviewed) in OBS Studio 27.2 Nov 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants