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 building OBS Studio on FreeBSD #6026

Merged
merged 2 commits into from
Feb 26, 2022
Merged

Conversation

obiwac
Copy link
Contributor

@obiwac obiwac commented Feb 24, 2022

Description

Fix building OBS Studio on FreeBSD:

  • libobs/graphics/graphics.h: The gs_texture_create_from_dmabuf & gs_query_dmabuf_* functions were marked as Linux-only as of f7a55f4.
  • plugins/linux-v4l2/v4l2-output.c: versionsort (and by extension, __strverscmp) are not available on FreeBSD, as they are GNU extensions (versionsort is used by virtualcam_start to read /dev/video* devices in the right order), so implemented them.
  • UI/platform-x11.cpp: Was calling to_string on FreeBSD when not using namespace std;, so calling std::to_string instead.

Motivation and Context

Fixes building on FreeBSD.

How Has This Been Tested?

Tested on FreeBSD 13.0-RELEASE-p4 and FreeBSD-CURRENT, hardware is non-applicable.
Compiled with clang version 11.0.1 & 13.0.1.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

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.

Perhaps this is usererror, but git-clang-format doesn't seem to work?

YAML:35:21: error: unknown key 'AfterExternBlock'
  AfterExternBlock: false
                    ^~~~~
Error reading /usr/home/obiwac/obs-studio/.clang-format: Invalid argument
error: `clang-format -lines=178:178 -lines=187:187 -lines=191:191 -lines=196:196 -lines=198:255 -lines=258:258 -lines=260:260 plugins/linux-v4l2/v4l2-output.c` failed

So I attempted to format things myself.
Hope that's alright :)

@kkartaltepe
Copy link
Collaborator

I don't think we will want to include an implementation of versionsort from glibc just for freebsd. Is there an alternative function that can be used on freebsd?

@obiwac
Copy link
Contributor Author

obiwac commented Feb 24, 2022

I don't think we will want to include an implementation of versionsort from glibc just for freebsd. Is there an alternative function that can be used on freebsd?

Yeah, it's not ideal indeed, but again, to my knowledge and after a good look, I don't think there is.
There does exist alphasort though, as can be seen in the scandir(3) manual, but that means differing behaviour on different platforms.

Also, alphasort is not a good solution as it would order devices as follows:

/dev/video0
/dev/video1
/dev/video10
/dev/video2
...
/dev/video9

I guess I could implement a bit of a simpler sorting function without considering the fractional part for both Linux & FreeBSD?

@kkartaltepe
Copy link
Collaborator

Also, alphasort is not a good solution as it would order devices as follows:

It seems a decent solution to enable building on freebsd without introducing a large amount of platform specific code. You could consider leaving the results unsorted on freebsd as well perhaps the devices will simply be in the proper order for most users.

@WizardCM WizardCM added Bug Fix Non-breaking change which fixes an issue Other OS (*nix) Other Unix-like systems that are not officially supported (e.g. OpenBSD) labels Feb 24, 2022
@WizardCM
Copy link
Member

From CI:

clang-format-12 is /usr/bin/clang-format-12
=================================
Files were not formatted properly
plugins/linux-v4l2/v4l2-output.c

@obiwac
Copy link
Contributor Author

obiwac commented Feb 24, 2022

You could consider leaving the results unsorted on freebsd as well perhaps the devices will simply be in the proper order for most users.

Can confirm this is not the case, unfortunately :(

Something like this could always be used:

static int devsort(const struct dirent **_a, const struct dirent **_b) {
	int a = atoi((*_a)->d_name + 5);
	int b = atoi((*_b)->d_name + 5);

	return a - b;
}

This would eliminate the need for any platform specific code whatsoever, as it works both on Linux & FreeBSD. (Strings aren't sorted if not numeric.)

From CI:

clang-format-12 is /usr/bin/clang-format-12
=================================
Files were not formatted properly
plugins/linux-v4l2/v4l2-output.c

That's my bad, still figuring out how to use this clang-format stuff.

obiwac added a commit to obiwac/obs-studio that referenced this pull request Feb 25, 2022
After feedback from obsproject#6026, replace implementation of versionsort on
FreeBSD by scansort on both Linux & FreeBSD.
This sorts /dev/video* devices by their numeric part.
@obiwac
Copy link
Contributor Author

obiwac commented Feb 26, 2022

Implemented that function I mentioned in my previous comment. Is it better like that or should I just choose between alphasort on FreeBSD and versionsort on Linux?

@kkartaltepe
Copy link
Collaborator

I would prefer a function that wont do bad things in case it gets odd input, and it seems the only one available for BSD users with libc is alphasort.

Though I suspect no one will sort before filtering in their scandir implementation but if they did, us reading invalid memory is not a good idea.

@obiwac
Copy link
Contributor Author

obiwac commented Feb 26, 2022

Sure, I was operating under that assumption initially, and from that point of view I don't think anything bad could happen.

Regardless, I'll replace it with a simple alphasort/versionsort depending on platform in a bit.

@obiwac
Copy link
Contributor Author

obiwac commented Feb 26, 2022

Done

@kkartaltepe
Copy link
Collaborator

Please squash away the obsoleted commits, after that it seems good to merge.

@obiwac
Copy link
Contributor Author

obiwac commented Feb 26, 2022

Yup, of course

@obiwac obiwac force-pushed the freebsd-fix branch 2 times, most recently from 1d39a71 to d671c49 Compare February 26, 2022 12:50
@obiwac
Copy link
Contributor Author

obiwac commented Feb 26, 2022

There we go!

Copy link
Collaborator

@kkartaltepe kkartaltepe left a comment

Choose a reason for hiding this comment

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

You squashed the to_string fix into the v4l2 fix if you could split that back out or make note in the commit desc.

Sort video device entries with `alphasort` on non-Linux platforms,
as opposed to `versionsort` on Linux.
(`versionsort` is a GNU extension, unavailable on e.g. FreeBSD.)

UI: Fix call to `to_string` on FreeBSD
@obiwac
Copy link
Contributor Author

obiwac commented Feb 26, 2022

There we go.

@jp9000 jp9000 merged commit 93c2e68 into obsproject:master Feb 26, 2022
@obiwac obiwac deleted the freebsd-fix branch February 27, 2022 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Fix Non-breaking change which fixes an issue Other OS (*nix) Other Unix-like systems that are not officially supported (e.g. OpenBSD)
Projects
No open projects
Status: Fixed & Released
Development

Successfully merging this pull request may close these issues.

error: use of undeclared identifier 'GS_DMABUF_FLAG_IMPLICIT_MODIFIERS_SUPPORTED'
4 participants