[bug#46113] [PATCH] gnu: obs: Update obs to fb347c.

[bug#46113] [PATCH] gnu: obs: Update obs to fb347c.

[Andrew Tropin]

This version of obs adds support for OBS_PLUGINS*_PATH environment variables,
which are required to be able to load plugins, which are present in profile.

It will make it possible for following packages to work:
http://issues.guix.gnu.org/45961
http://issues.guix.gnu.org/45960

* gnu/packages/video.scm (obs): Update to fb347c.
---
 gnu/packages/video.scm | 116 ++++++++++++++++++++++-------------------
 1 file changed, 63 insertions(+), 53 deletions(-)

diff --git a/gnu/packages/video.scm b/gnu/packages/video.scm
index 28cde06f04..0f9b405261 100644
--- a/gnu/packages/video.scm
+++ b/gnu/packages/video.scm
@@ -3076,62 +3076,72 @@ be used for realtime video capture via Linux-specific APIs.")
     (license (list license:lgpl2.1+ license:gpl2))))
 
 (define-public obs
-  (package
-    (name "obs")
-    (version "26.1.0")
-    (source (origin
-              (method git-fetch)
-              (uri (git-reference
-                    (url "https://github.com/obsproject/obs-studio")
-                    (commit version)))
-              (file-name (git-file-name name version))
-              (sha256
-               (base32
-                "0p8wdzm9imn3s17arr206sz92g4pkacfcpfbwvhvgkrrs4w000bx"))))
-    (build-system cmake-build-system)
-    (arguments
-     `(#:configure-flags
-       (list (string-append "-DOBS_VERSION_OVERRIDE=" ,version)
-             "-DENABLE_UNIT_TESTS=TRUE")
-       #:phases
-       (modify-phases %standard-phases
-         (add-after 'install 'wrap-executable
-           (lambda* (#:key outputs #:allow-other-keys)
-             (let ((out (assoc-ref outputs "out"))
-                   (plugin-path (getenv "QT_PLUGIN_PATH")))
-               (wrap-program (string-append out "/bin/obs")
-                 `("QT_PLUGIN_PATH" ":" prefix (,plugin-path))))
-             #t)))))
-    (native-inputs
-     `(("cmocka" ,cmocka)
-       ("pkg-config" ,pkg-config)))
-    (inputs
-     `(("alsa-lib" ,alsa-lib)
-       ("curl" ,curl)
-       ("eudev" ,eudev)
-       ("ffmpeg" ,ffmpeg)
-       ("fontconfig" ,fontconfig)
-       ("freetype" ,freetype)
-       ("jack" ,jack-1)
-       ("jansson" ,jansson)
-       ("libx264" ,libx264)
-       ("libxcomposite" ,libxcomposite)
-       ("mbedtls" ,mbedtls-apache)
-       ("mesa" ,mesa)
-       ("pulseaudio" ,pulseaudio)
-       ("qtbase" ,qtbase)
-       ("qtsvg" ,qtsvg)
-       ("qtx11extras" ,qtx11extras)
-       ("speexdsp" ,speexdsp)
-       ("v4l-utils" ,v4l-utils)
-       ("zlib" ,zlib)))
-    (synopsis "Live streaming software")
-    (description "Open Broadcaster Software provides a graphical interface for
+  (let ((commit "fb347c3c62ced2ea302769e449d300fd923c2d4b")
+        (revision "1"))
+    (package
+      (name "obs")
+      (version (git-version "26.1.2" revision commit))
+      (source (origin
+                (method git-fetch)
+                (uri (git-reference
+                      (url "https://github.com/obsproject/obs-studio")
+                      (commit commit)))
+                (file-name (git-file-name name version))
+                (sha256
+                 (base32
+                  "017llgj1hlfvk2622qa44d8iz6d0kahhckn421dypj09a4n6aajz"))))
+      (build-system cmake-build-system)
+      (arguments
+       `(#:configure-flags
+         (list (string-append "-DOBS_VERSION_OVERRIDE=" ,version)
+               "-DENABLE_UNIT_TESTS=TRUE")
+         #:phases
+         (modify-phases %standard-phases
+           (add-after 'install 'wrap-executable
+             (lambda* (#:key outputs #:allow-other-keys)
+               (let ((out (assoc-ref outputs "out"))
+                     (plugin-path (getenv "QT_PLUGIN_PATH")))
+                 (wrap-program (string-append out "/bin/obs")
+                   `("QT_PLUGIN_PATH" ":" prefix (,plugin-path))))
+               #t)))))
+      (native-inputs
+       `(("cmocka" ,cmocka)
+         ("pkg-config" ,pkg-config)))
+      (inputs
+       `(("alsa-lib" ,alsa-lib)
+         ("curl" ,curl)
+         ("eudev" ,eudev)
+         ("ffmpeg" ,ffmpeg)
+         ("fontconfig" ,fontconfig)
+         ("freetype" ,freetype)
+         ("jack" ,jack-1)
+         ("jansson" ,jansson)
+         ("libx264" ,libx264)
+         ("libxcomposite" ,libxcomposite)
+         ("mbedtls" ,mbedtls-apache)
+         ("mesa" ,mesa)
+         ("pulseaudio" ,pulseaudio)
+         ("qtbase" ,qtbase)
+         ("qtsvg" ,qtsvg)
+         ("qtx11extras" ,qtx11extras)
+         ("speexdsp" ,speexdsp)
+         ("v4l-utils" ,v4l-utils)
+         ("zlib" ,zlib)))
+      (native-search-paths
+       (list
+        (search-path-specification
+         (variable "OBS_PLUGINS_DATA_PATH")
+         (files '("share/obs/obs-plugins")))
+        (search-path-specification
+         (variable "OBS_PLUGINS_PATH")
+         (files '("lib/obs-plugins")))))
+      (synopsis "Live streaming software")
+      (description "Open Broadcaster Software provides a graphical interface for
 video recording and live streaming.  OBS supports capturing audio and video
 from many input sources such as webcams, X11 (for screencasting), PulseAudio,
 and JACK.")
-    (home-page "https://obsproject.com")
-    (license license:gpl2+)))
+      (home-page "https://obsproject.com")
+      (license license:gpl2+))))
 
 (define-public libvdpau
   (package
-- 
2.30.0

[bug#46113] [PATCH] gnu: obs: Update obs to fb347c.

[Andrew Tropin]

It's a commit from master branch containing changes from PR:
https://github.com/obsproject/obs-studio/pull/4067

It's just few commits after 26.1.2 and it seems there is no any big
changes since that and should be safe to use.

I asked for 26.1.3 tag, but it seems that it won't happen in nearest
future: https://github.com/obsproject/obs-studio/issues/4100

As one of developers said to me, usually obs releases once in 3-4
months, that is why I decided to use a commit instead of tag.

The changes is needed to be able to specify plugin load path using
native-search-paths to be able to package obs plugins with guix.

[bug#46113] [PATCH] gnu: obs: Update obs to fb347c.

[Alexey Abramov]

Hi Adrew,

I patched obs here [1]. I checked the upstream patch, and noticed that it ammends AddExtraModulePaths function. With Guix the list of plugin directories will contain *two* locations with the very same plugins, one with the obs store and guix-profile, hence obs will print 'Duplicate library?' warning messages.

I packaged the main location to avoid such a message.

Footnotes:
[1]  https://issues.guix.gnu.org/45707

-- 
Alexey

[bug#46113] [PATCH] gnu: obs: Update obs to fb347c.

[Leo Famulari]

On Tue, Jan 26, 2021 at 06:59:39PM +0300, Andrew Tropin wrote:
> 
> This version of obs adds support for OBS_PLUGINS*_PATH environment variables,
> which are required to be able to load plugins, which are present in profile.
> 
> It will make it possible for following packages to work:
> http://issues.guix.gnu.org/45961
> http://issues.guix.gnu.org/45960

Thanks!

Your patch doesn't apply to the current master branch. Can you rebase it
and send a revision?

[bug#46113] [PATCH] gnu: obs: Update obs to fb347c.

[Andrew Tropin]

> I patched obs here [1]. I checked the upstream patch, and noticed that
> it ammends AddExtraModulePaths function. With Guix the list of plugin
> directories will contain *two* locations with the very same plugins,
> one with the obs store and guix-profile, hence obs will print
> 'Duplicate library?' warning messages.

It's very true, obs, will have same plugins in load paths twice, but
from what I found the problem will happen only during shutdown of obs,
when it will try to unload the same plugin twice, which doesn't affect
runtime anyhow. I had a small workaround for that (a separate
envirnoment variable, which toggles the loading of plugins from
INSTALL_PREFIX), but during review one of obs mantainers said that it's
an adhoc solution and better to solve this problem in general. The
comment from @kkartaltepe and related changes [fn:1].

The other solution was to check if OBS_PLUGINS_PATH is present and ommit
loading of builtin plugins in that case, but it will break in case
someone wants to specify just additional plugin load path (some non-guix
use case).

I decided to go without any fix to that problem as it is not directly
related to changeset I proposed and doesn't affect runtime, but it's
probably a good idea to report double free on shutdown issue to
upstream.

* Footnotes

[fn:1] https://github.com/obsproject/obs-studio/pull/4067 

[bug#46113] [PATCH] gnu: obs: Update to fb347c.

[Andrew Tropin]

This version of obs adds support for OBS_PLUGINS*_PATH environment variables,
which are required to be able to load plugins, which are present in profile.

It will make it possible for following packages to work:
http://issues.guix.gnu.org/45961
http://issues.guix.gnu.org/45960

* gnu/packages/video.scm (obs): Update to fb347c.
---
 gnu/packages/video.scm | 116 ++++++++++++++++++++++-------------------
 1 file changed, 63 insertions(+), 53 deletions(-)

diff --git a/gnu/packages/video.scm b/gnu/packages/video.scm
index 97cb7d6837..f9741dc843 100644
--- a/gnu/packages/video.scm
+++ b/gnu/packages/video.scm
@@ -3077,62 +3077,72 @@ be used for realtime video capture via Linux-specific APIs.")
     (license (list license:lgpl2.1+ license:gpl2))))
 
 (define-public obs
-  (package
-    (name "obs")
-    (version "26.1.2")
-    (source (origin
-              (method git-fetch)
-              (uri (git-reference
-                    (url "https://github.com/obsproject/obs-studio")
-                    (commit version)))
-              (file-name (git-file-name name version))
-              (sha256
-               (base32
-                "1k1asqiqw757v59ayx0w029ril947hs0lcp8n91knzjl891fr4nc"))))
-    (build-system cmake-build-system)
-    (arguments
-     `(#:configure-flags
-       (list (string-append "-DOBS_VERSION_OVERRIDE=" ,version)
-             "-DENABLE_UNIT_TESTS=TRUE")
-       #:phases
-       (modify-phases %standard-phases
-         (add-after 'install 'wrap-executable
-           (lambda* (#:key outputs #:allow-other-keys)
-             (let ((out (assoc-ref outputs "out"))
-                   (plugin-path (getenv "QT_PLUGIN_PATH")))
-               (wrap-program (string-append out "/bin/obs")
-                 `("QT_PLUGIN_PATH" ":" prefix (,plugin-path))))
-             #t)))))
-    (native-inputs
-     `(("cmocka" ,cmocka)
-       ("pkg-config" ,pkg-config)))
-    (inputs
-     `(("alsa-lib" ,alsa-lib)
-       ("curl" ,curl)
-       ("eudev" ,eudev)
-       ("ffmpeg" ,ffmpeg)
-       ("fontconfig" ,fontconfig)
-       ("freetype" ,freetype)
-       ("jack" ,jack-1)
-       ("jansson" ,jansson)
-       ("libx264" ,libx264)
-       ("libxcomposite" ,libxcomposite)
-       ("mbedtls" ,mbedtls-apache)
-       ("mesa" ,mesa)
-       ("pulseaudio" ,pulseaudio)
-       ("qtbase" ,qtbase)
-       ("qtsvg" ,qtsvg)
-       ("qtx11extras" ,qtx11extras)
-       ("speexdsp" ,speexdsp)
-       ("v4l-utils" ,v4l-utils)
-       ("zlib" ,zlib)))
-    (synopsis "Live streaming software")
-    (description "Open Broadcaster Software provides a graphical interface for
+  (let ((commit "fb347c3c62ced2ea302769e449d300fd923c2d4b")
+        (revision "1"))
+    (package
+      (name "obs")
+      (version (git-version "26.1.2" revision commit))
+      (source (origin
+                (method git-fetch)
+                (uri (git-reference
+                      (url "https://github.com/obsproject/obs-studio")
+                      (commit version)))
+                (file-name (git-file-name name version))
+                (sha256
+                 (base32
+                  "017llgj1hlfvk2622qa44d8iz6d0kahhckn421dypj09a4n6aajz"))))
+      (build-system cmake-build-system)
+      (arguments
+       `(#:configure-flags
+         (list (string-append "-DOBS_VERSION_OVERRIDE=" ,version)
+               "-DENABLE_UNIT_TESTS=TRUE")
+         #:phases
+         (modify-phases %standard-phases
+           (add-after 'install 'wrap-executable
+             (lambda* (#:key outputs #:allow-other-keys)
+               (let ((out (assoc-ref outputs "out"))
+                     (plugin-path (getenv "QT_PLUGIN_PATH")))
+                 (wrap-program (string-append out "/bin/obs")
+                   `("QT_PLUGIN_PATH" ":" prefix (,plugin-path))))
+               #t)))))
+      (native-inputs
+       `(("cmocka" ,cmocka)
+         ("pkg-config" ,pkg-config)))
+      (inputs
+       `(("alsa-lib" ,alsa-lib)
+         ("curl" ,curl)
+         ("eudev" ,eudev)
+         ("ffmpeg" ,ffmpeg)
+         ("fontconfig" ,fontconfig)
+         ("freetype" ,freetype)
+         ("jack" ,jack-1)
+         ("jansson" ,jansson)
+         ("libx264" ,libx264)
+         ("libxcomposite" ,libxcomposite)
+         ("mbedtls" ,mbedtls-apache)
+         ("mesa" ,mesa)
+         ("pulseaudio" ,pulseaudio)
+         ("qtbase" ,qtbase)
+         ("qtsvg" ,qtsvg)
+         ("qtx11extras" ,qtx11extras)
+         ("speexdsp" ,speexdsp)
+         ("v4l-utils" ,v4l-utils)
+         ("zlib" ,zlib)))
+      (native-search-paths
+       (list
+        (search-path-specification
+         (variable "OBS_PLUGINS_DATA_PATH")
+         (files '("share/obs/obs-plugins")))
+        (search-path-specification
+         (variable "OBS_PLUGINS_PATH")
+         (files '("lib/obs-plugins")))))
+      (synopsis "Live streaming software")
+      (description "Open Broadcaster Software provides a graphical interface for
 video recording and live streaming.  OBS supports capturing audio and video
 from many input sources such as webcams, X11 (for screencasting), PulseAudio,
 and JACK.")
-    (home-page "https://obsproject.com")
-    (license license:gpl2+)))
+      (home-page "https://obsproject.com")
+      (license license:gpl2+))))
 
 (define-public libvdpau
   (package
-- 
2.30.0

[bug#46113] [PATCH] gnu: obs: Update to fb347c.

[Leo Famulari]

On Wed, Jan 27, 2021 at 12:45:30PM +0300, Andrew Tropin wrote:
> This version of obs adds support for OBS_PLUGINS*_PATH environment variables,
> which are required to be able to load plugins, which are present in profile.
> 
> It will make it possible for following packages to work:
> http://issues.guix.gnu.org/45961
> http://issues.guix.gnu.org/45960
> 
> * gnu/packages/video.scm (obs): Update to fb347c.

Thanks for this!

I'm wondering, instead of building from a Git commit, can we cherry-pick
the commit as a patch?

https://github.com/obsproject/obs-studio/commit/fb347c3c62ced2ea302769e449d300fd923c2d4b.patch

Does it work if we apply that to the 26.1.2 release?

It would be a little more precise.

[bug#46113] [PATCH] gnu: obs: Update to fb347c.

[Andrew Tropin]

Hello Leo!

> Does it work if we apply that to the 26.1.2 release?

It should work, originally I wrote a patch for 26.1.2 and later rebased
the commit on master without any conflicts.

> It would be a little more precise.

There was a discussion on irc about it and at the end of the day I would
prefer to use the commit from master branch, rather than maintaining a
separate patch, which we will need to remove later, when 26.1.3 or
another version will be released.

There were not so many changes since 26.1.2 up to the fb347c, I fluently
went through them, they are mostly minor and were accepted after the
pretty strict code review process.

From what I know it should be safe to use this commit, but I can contact
upstream devs and additionally clarify that.

Overall I'm ok with using an exact patch, but prefer using a commit from
master. WDYT?


-- 
Best regards,
Andrew Tropin

[bug#46113] [PATCH] gnu: obs: Update to fb347c.

[Leo Famulari]

On Tue, Feb 02, 2021 at 02:39:03PM +0300, Andrew Tropin wrote:
> > Does it work if we apply that to the 26.1.2 release?
> 
> It should work, originally I wrote a patch for 26.1.2 and later rebased
> the commit on master without any conflicts.
> 
> > It would be a little more precise.
> 
> There was a discussion on irc about it and at the end of the day I would
> prefer to use the commit from master branch, rather than maintaining a
> separate patch, which we will need to remove later, when 26.1.3 or
> another version will be released.
> 
> There were not so many changes since 26.1.2 up to the fb347c, I fluently
> went through them, they are mostly minor and were accepted after the
> pretty strict code review process.
> 
> From what I know it should be safe to use this commit, but I can contact
> upstream devs and additionally clarify that.
> 
> Overall I'm ok with using an exact patch, but prefer using a commit from
> master. WDYT?

Thanks for explaining the process you went through. Based on that, I
agree that your patch is good as proposed.

The patch file doesn't apply to current master. It looks like some other
changes regarding this plugins problem have been done to the obs package
since your changes were made.

Can you rebase it on your end and re-generate the patch? Or is it still
necessary?

[bug#46113] [PATCH] gnu: obs: Update to fb347c.

[Andrew Tropin]

> Thanks for explaining the process you went through. Based on that, I
> agree that your patch is good as proposed.
>
> The patch file doesn't apply to the current master. It looks like some other
> changes regarding this plugins problem have been done to the obs package
> since your changes were made.
>
> Can you rebase it on your end and re-generate the patch? Or is it still
> necessary?

I'll post to the related thread first https://issues.guix.gnu.org/45707
to discuss what we can do about that situation, and will post back here,
when we will reach some agreement.

-- 
Best regards,
Andrew Tropin

[bug#45707] [PATCH] gnu: obs: Update obs to fb347c.

[Andrew Tropin]

> Should we wait for the next OBS release instead of packaging an
> arbitrary commit?

I'm not in a hurry, but as I explained earlier in this thread [fn:2] it seems
relatively safe for me to use this commit and getting back later to
26.1.3 or 26.2.0 or whatever next release will be.

> I am not insist on keeping [1], but I do think that is more cleaner
> solution.

In terms of implememntation I like that [fn:1] prevents double loading
of plugins, by excluding obs installation dir from "load-path". However
double loading of the same plugin doesn't seem to break anything. Also,
OBS_PLUGINS_DIRECTORY variable name maybe a little better than
OBS_PLUGINS_PATH as it contains only one path.

The problem is that now there are two almost identical mechanisms (one
in upstream and one via patch [fn:1]), which can bring some maintanance
problems in the future.

There are two good option in my opinion:
- contribute patch from [fn:1] to upstream (reverting OBS_PLUGINS_PATH)
- revert [fn:1] and use OBS_PLUGINS_PATH from upstream

If Alexey ready to contibute OBS_PLUGINS_DIRECTORY patch to obs
(reverting OBS_PLUGINS_PATH), I would be glad to support it. Otherwise,
I would prefer to revert [fn:1] and apply this one. To prevent
maintanance problems in the future.

* Footnotes

[fn:2] http://issues.guix.gnu.org/46113 

[fn:1] https://issues.guix.gnu.org/45707 

--
Best regards,
Andrew Tropin

[bug#46113] [PATCH] gnu: obs: Update obs to fb347c.

[Ludovic Courtès]

Hi!

Andrew Tropin <andrew@trop.in> skribis:

> The problem is that now there are two almost identical mechanisms (one
> in upstream and one via patch [fn:1]), which can bring some maintanance
> problems in the future.
>
> There are two good option in my opinion:
> - contribute patch from [fn:1] to upstream (reverting OBS_PLUGINS_PATH)
> - revert [fn:1] and use OBS_PLUGINS_PATH from upstream
>
> If Alexey ready to contibute OBS_PLUGINS_DIRECTORY patch to obs
> (reverting OBS_PLUGINS_PATH), I would be glad to support it. Otherwise,
> I would prefer to revert [fn:1] and apply this one. To prevent
> maintanance problems in the future.

I think it’s nicer to follow upstream in general, but I also don’t mind
using our OBS_PLUGINS_DIRECTORY patch until we upgrade to the next
release.

So, Andrew and Alexey: lemme know and I’ll apply what you consider best!

Ludo’.

[bug#46113] [PATCH] gnu: obs: Update obs to fb347c.

[guix-patches--- via]

Hi Andrew,

Andrew Tropin <andrew@trop.in> writes:

>> Should we wait for the next OBS release instead of packaging an
>> arbitrary commit?
>
> I'm not in a hurry, but as I explained earlier in this thread [fn:2] it seems
> relatively safe for me to use this commit and getting back later to
> 26.1.3 or 26.2.0 or whatever next release will be.
>
>> I am not insist on keeping [1], but I do think that is more cleaner
>> solution.
>
> In terms of implememntation I like that [fn:1] prevents double loading
> of plugins, by excluding obs installation dir from "load-path". However
> double loading of the same plugin doesn't seem to break anything. Also,
> OBS_PLUGINS_DIRECTORY variable name maybe a little better than
> OBS_PLUGINS_PATH as it contains only one path.
>
> The problem is that now there are two almost identical mechanisms (one
> in upstream and one via patch [fn:1]), which can bring some maintanance
> problems in the future.
>
> There are two good option in my opinion:
> - contribute patch from [fn:1] to upstream (reverting OBS_PLUGINS_PATH)

I would go with this one. But the thing is that [fn:1] is specific and
makes sense only for guix. At least from my point of view. So I doubt
that the upstream accept it. But you can try for sure.

> - revert [fn:1] and use OBS_PLUGINS_PATH from upstream
>
> If Alexey ready to contibute OBS_PLUGINS_DIRECTORY patch to obs
> (reverting OBS_PLUGINS_PATH), I would be glad to support it. Otherwise,
> I would prefer to revert [fn:1] and apply this one. To prevent
> maintanance problems in the future.

As I am not a regular obs user, I am afraid I won't be able to find time
for this soon. Feel free to use/delete/revert [fn:1]. It is your call.

> * Footnotes
>
> [fn:2] http://issues.guix.gnu.org/46113 
>
> [fn:1] https://issues.guix.gnu.org/45707 
>
> --
> Best regards,
> Andrew Tropin

-- 
Alexey