gh-obsproject/obs-browser
Watch
1
0
Fork
You've already forked obs-browser
 0
Code Issues 37 Pull requests 13 Releases 15 Wiki Activity

obs-browser: Update default size #202

Open
cg2121 wants to merge 1 commit from cg2121/browser-defaults into master
pull from: cg2121/browser-defaults
merge into: gh-obsproject:master
Conversation 15 Commits 1 Files changed 1 +23 -1
cg2121 commented 2020-02-21 23:44:53 -06:00 (Migrated from github.com)

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.
### 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: - [x] My code has been run through [clang-format](https://github.com/obsproject/obs-studio/blob/master/.clang-format). - [x] I have read the [**contributing** document](https://github.com/obsproject/obs-studio/blob/master/CONTRIBUTING.rst). - [x] My code is not on the master branch. - [x] The code has been tested. - [x] All commit messages are properly formatted and commits squashed where appropriate.
❤️ 3
WizardCM (Migrated from github.com) reviewed 2020-02-22 03:07:51 -06:00
obs-browser-plugin.cpp
WizardCM (Migrated from github.com) commented 2020-02-22 03:07:51 -06:00

Could this technically call browser_source_get_defaults() first, then set the width/height in this function?

Could this technically call `browser_source_get_defaults()` first, then set the width/height in this function?
RytoEX commented 2021-03-12 15:18:31 -06:00 (Migrated from github.com)

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

I prefer something like this, but it's probably down to personal preference. https://github.com/RytoEX/obs-browser/tree/change-default-size
notr1ch commented 2021-08-22 19:32:27 -05:00 (Migrated from github.com)

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.

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 commented 2021-08-22 19:34:08 -05:00 (Migrated from github.com)

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

Yes, v2 is required because the defaults aren't saved on source creation. It's the reason v2 exists at all.
WizardCM (Migrated from github.com) requested changes 2021-08-22 19:35:22 -05:00
WizardCM (Migrated from github.com) left a comment

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.

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 commented 2021-08-22 19:46:47 -05:00 (Migrated from github.com)

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

Ah I see, I didn't realise our defaults worked like that.
notr1ch (Migrated from github.com) reviewed 2021-08-22 19:56:19 -05:00
obs-browser-plugin.cpp
notr1ch (Migrated from github.com) commented 2021-08-22 19:56:19 -05:00

I was also thinking this as code duplication is generally bad, but after realising our defaults work in the way they do, it does also seem somewhat fitting to keep them separated. If we're committed to never changing v1 defaults then I think removing the duplication is OK.

I was also thinking this as code duplication is generally bad, but after realising our defaults work in the way they do, it does also seem somewhat fitting to keep them separated. If we're committed to never changing v1 defaults then I think removing the duplication is OK.
notr1ch (Migrated from github.com) reviewed 2021-08-22 19:56:48 -05:00
obs-browser-plugin.cpp
notr1ch (Migrated from github.com) commented 2021-08-22 19:56:48 -05:00

Would it also make sense to default to the canvas FPS here?

Would it also make sense to default to the canvas FPS here?
cg2121 (Migrated from github.com) reviewed 2021-08-22 21:48:04 -05:00
obs-browser-plugin.cpp
cg2121 (Migrated from github.com) commented 2021-08-22 21:48:04 -05:00

Updated function to remove duplicate code.

Updated function to remove duplicate code.
cg2121 (Migrated from github.com) reviewed 2021-08-22 21:49:45 -05:00
obs-browser-plugin.cpp
cg2121 (Migrated from github.com) commented 2021-08-22 21:49:45 -05:00

This fps setting is only used if 'use custom fps' is checked.

This fps setting is only used if 'use custom fps' is checked.
notr1ch (Migrated from github.com) reviewed 2021-08-23 06:25:53 -05:00
obs-browser-plugin.cpp
notr1ch (Migrated from github.com) commented 2021-08-23 06:25:53 -05:00

I'm aware of that, but in those situations wouldn't it also make sense to default to the current FPS?

I'm aware of that, but in those situations wouldn't it also make sense to default to the current FPS?
WizardCM commented 2021-09-13 04:40:04 -05:00 (Migrated from github.com)

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.

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.
tt2468 commented 2021-10-07 05:29:54 -05:00 (Migrated from github.com)

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.

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 commented 2021-10-07 12:53:25 -05:00 (Migrated from github.com)

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: github.com/obsproject/obs-browser@e54c9f14b3/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.

> > > 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.
👍 1
tt2468 commented 2021-10-07 14:36:26 -05:00 (Migrated from github.com)

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

Fair enough. I do believe in the meta tags idea, so I'll try to find time to work on that.
This pull request has changes conflicting with the target branch.
  • obs-browser-plugin.cpp
View command line instructions

Manual merge helper

Use this merge commit message when completing the merge manually.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin 'cg2121/browser-defaults:cg2121/browser-defaults'
git switch 'cg2121/browser-defaults'

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch 'master'
git merge --no-ff 'cg2121/browser-defaults'
git switch 'cg2121/browser-defaults'
git rebase 'master'
git switch 'master'
git merge --ff-only 'cg2121/browser-defaults'
git switch 'cg2121/browser-defaults'
git rebase 'master'
git switch 'master'
git merge --no-ff 'cg2121/browser-defaults'
git switch 'master'
git merge --squash 'cg2121/browser-defaults'
git switch 'master'
git merge --ff-only 'cg2121/browser-defaults'
git switch 'master'
git merge 'cg2121/browser-defaults'
git push origin 'master'
Sign in to join this conversation.
Reviewers
No reviewers
WizardCM
Labels
Clear labels   
area/accessibility
Improves accessibility of the program

  
area/dependency
Issue is in external library or dependency, not OBS itself

  
Confirmed

   
Crash Report

   
Delayed
Merging has been delayed due to an issue

  
Design Issue
There is an issue with incorporating this pull request's design. It may take some time to finalize.

  
Duplicate

   
Feedback required
Issue does not contain enough information or is missing the template

  
Good first issue
New to contributing to OBS? Start here!

  
kind/bug
Categorizes issue or PR as related to a bug.

  
kind/cleanup
Non-breaking change which makes code smaller or more readable

  
kind/enhancement
Enhancements are not bugs or new features but can improve usability or performance.

  
platform/linux

   
platform/macos

   
platform/windows

   
priority/high
This is an important PR

  
Question

   
Request for Comments
More feedback & discussion is requested

  
Seeking Other Contributors
This PR needs help from any other developers who are willing to give a crack at this code

  
Seeking Testers

   
Won't Fix / By Design
No labels
area/accessibility
area/dependency
Confirmed
Crash Report
Delayed
Design Issue
Duplicate
Feedback required
Good first issue
kind/bug
kind/cleanup
kind/enhancement
platform/linux
platform/macos
platform/windows
priority/high
Question
Request for Comments
Seeking Other Contributors
Seeking Testers
Won't Fix / By Design
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gh-obsproject/obs-browser!202
No description provided.