Bug 1671064

Summary: Review Request: libldac - LDAC library from AOSP
Product: [Fedora] Fedora Reporter: Gergely Gombos <gombosg>
Component: Package ReviewAssignee: Dominik 'Rathann' Mierzejewski <dominik>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: unspecified    
Version: rawhideCC: dan, dominik, hannsj_uhl, heri, ngompa13, olysonek, package-review, pasik, pbrobinson, rabin, rsandu, samuel-rhbugs, sgraf, tcallawa, xavier
Target Milestone: ---Flags: dominik: fedora-review+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2019-03-05 15:20:50 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On: 1677491    
Bug Blocks:    

Description Gergely Gombos 2019-01-30 16:45:09 UTC
Spec:
https://pagure.io/libldac/raw/master/f/libldac.spec
SRPM:
https://pagure.io/libldac/raw/master/f/libldac-2.0.2.2-2.fc30.src.rpm

Description:
LDAC library from AOSP.
LDAC is an audio coding technology developed by Sony.
It enables the transmission of High-Resolution Audio content,
even over a Bluetooth connection.

Fedora Account System Username:
gombosg

I originally submitted to RPMFusion:
https://bugzilla.rpmfusion.org/show_bug.cgi?id=5154

This package may be suitable for the Fedora repos depending on if and how LDAC itself is patented. The library itself has an Apache v2 license.

This is my first RPM package.
I'm looking for a sponsor because of this!

Comment 1 Dominik 'Rathann' Mierzejewski 2019-01-30 20:11:31 UTC
I can take the review and sponsor you, assuming this passes legal review.

Comment 2 Gergely Gombos 2019-01-31 16:19:15 UTC
Thank you!

For legal review:
There's a NOTICE file in the source that says:
https://android.googlesource.com/platform/external/libldac/+/master/NOTICE

Taking the certification process is required to use LDAC in your products.
   For the detail of certification process, see the following URL:
      https://www.sony.net/Products/LDAC/aosp/

Comment 4 Dominik 'Rathann' Mierzejewski 2019-02-07 18:04:57 UTC
License tag is fine. Packaging-wise it looks very good, too. I have some nitpicks below:

1. I suggest listing pkgconfig and include files explicitly to avoid missing changes from upstream:

--- libldac.spec.orig	2019-01-30 17:40:13.000000000 +0100
+++ libldac.spec	2019-02-07 18:50:43.172939850 +0100
@@ -50,8 +50,11 @@
 %{_libdir}/libldacBT_enc.so.%{sonamebase}.*
 
 %files devel
-%{_includedir}/*
-%{_libdir}/pkgconfig/*
+%dir %{_includedir}/ldac
+%{_includedir}/ldac/ldacBT_abr.h
+%{_includedir}/ldac/ldacBT.h
+%{_libdir}/pkgconfig/ldacBT-abr.pc
+%{_libdir}/pkgconfig/ldacBT-enc.pc
 %{_libdir}/libldacBT_abr.so
 %{_libdir}/libldacBT_enc.so
 
2. Summary: could be more informative, e.g. "A lossy audio codec for Bluetooth connections".

3. I'd also recommend using BR: cmake3 and %{cmake3} macro in %build to make it build seamlessly on EPEL as well.

--- libldac.spec.orig	2019-01-30 17:40:13.000000000 +0100
+++ libldac.spec	2019-02-07 19:03:01.479784500 +0100
@@ -10,7 +10,7 @@
 URL:            https://github.com/EHfive/ldacBT
 Source0:        %{url}/releases/download/v%{version}/%{archivename}-%{version}.tar.gz
 
-BuildRequires:  cmake
+BuildRequires:  cmake3
 BuildRequires:  gcc
 
 %package        devel
@@ -30,7 +30,7 @@
 %autosetup -n %{archivename}
 
 %build
-%{cmake} \
+%{cmake3} \
     -DLDAC_SOFT_FLOAT=OFF \
     -DINSTALL_LIBDIR=%{_libdir} \
     ./

Comment 5 Gergely Gombos 2019-02-08 04:01:02 UTC
Thanks a lot for the suggestions, plus for directly including the patch snippets! I have applied them plus updated the description.

Spec:
https://pagure.io/libldac/raw/master/f/libldac.spec
SRPM:
https://pagure.io/libldac/raw/master/f/libldac-2.0.2.2-3.fc30.src.rpm

Comment 6 Dominik 'Rathann' Mierzejewski 2019-02-09 01:33:06 UTC
Can you do at least one non-trivial unofficial package review while we're waiting for the FE-Legal block to be lifted (ping Tom if it doesn't happen soon)?

Please pick something recent from https://fedoraproject.org/PackageReviewStatus/NEW.html

This package is technically APPROVED. I'll change the review flag status once FE-Legal is lifted.

Comment 7 Tom "spot" Callaway 2019-02-11 16:02:23 UTC
Lifting FE-Legal block, this package is fine.

Comment 8 Dominik 'Rathann' Mierzejewski 2019-02-14 10:12:22 UTC
Great, package is approved and I'm sponsoring Gergely into the packager group now.

Comment 9 Pasi Karkkainen 2019-02-14 13:06:42 UTC
Great to see libldac getting approved in Fedora! Thanks a lot everyone.

Btw are you interested in packaging libopenaptx (https://github.com/pali/libopenaptx) aswell? It's a separate LGPL library which implements only the AptX and AptX-HD codecs. I believe the patents around aptx expired last year..

It's possible to use libopenaptx instead of ffmpeg to get aptx/aptx-hd codecs supported in pulseaudio/bluetooth.
(and there are patches for that on pulseaudio mailinglist already. patches to bluez were already merged).

Comment 10 Gwyn Ciesla 2019-02-14 19:01:39 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/libldac

Comment 11 Dominik 'Rathann' Mierzejewski 2019-02-15 14:49:59 UTC
You can add

# big endian is not supported https://bugzilla.redhat.com/show_bug.cgi?id=1677491
ExcludeArch: s390x

and build anyway for now.

Comment 12 Gergely Gombos 2019-02-15 15:04:11 UTC
Thanks Dominik, I was just doing that. :)

* Fri Feb 15 2019 Gergely Gombos <gombosg> - 2.0.2.2-4
- Add s390x ExcludeArch

See #1677491. I don't have an s390x machine nor an actual LDAC headset to patch & test upstream code so I'm excluding this arch. :)

Builds fine now:
Rawhide https://koji.fedoraproject.org/koji/taskinfo?taskID=32832242
F29 https://koji.fedoraproject.org/koji/taskinfo?taskID=32832365

This still fails for ppc64, with the same byte order error:
F28 https://koji.fedoraproject.org/koji/taskinfo?taskID=32832369
(https://kojipkgs.fedoraproject.org//work/tasks/2399/32832399/build.log)

Does anyone know if there is a difference between F28 ppc64 and F29 ppc64 byte order support?

I either have to exclude ppc64 or not release for F28, right? Which one is the better choice in this case? 
(I'd rather not release for F28 since ppc64 works on F29+ and F28 would be EOL'd anyway soon.)

Thanks for the help.

Comment 13 Peter Robinson 2019-02-15 17:13:25 UTC
> Does anyone know if there is a difference between F28 ppc64 and F29 ppc64
> byte order support?

ppc64 is big endian, ppc64le is little endian.

> I either have to exclude ppc64 or not release for F28, right? Which one is
> the better choice in this case? 

I just wouldn't release for F-28, new functionality should really only be going to the newer releases.

Comment 14 Neal Gompa 2019-02-16 01:30:37 UTC
(In reply to Pasi Karkkainen from comment #9)
> Great to see libldac getting approved in Fedora! Thanks a lot everyone.
> 
> Btw are you interested in packaging libopenaptx
> (https://github.com/pali/libopenaptx) aswell? It's a separate LGPL library
> which implements only the AptX and AptX-HD codecs. I believe the patents
> around aptx expired last year..
> 
> It's possible to use libopenaptx instead of ffmpeg to get aptx/aptx-hd
> codecs supported in pulseaudio/bluetooth.
> (and there are patches for that on pulseaudio mailinglist already. patches
> to bluez were already merged).

Feel free to propose a package review request for it, and it can be looked at to bring into the distribution.

See here: https://fedoraproject.org/wiki/Join_the_package_collection_maintainers

And here: https://fedoraproject.org/wiki/Package_Review_Process

Comment 15 Fedora Update System 2019-02-17 03:53:32 UTC
libldac-2.0.2.2-4.fc29 has been submitted as an update to Fedora 29. https://bodhi.fedoraproject.org/updates/FEDORA-2019-90c880c7cc

Comment 16 Gergely Gombos 2019-02-17 03:57:40 UTC
Wrapping up my first package's release!

I decided not to release this for F28 due to ppc64 (big-endian) failing. From F29+ apparently pp64le is default, which works.

F29 build:
https://koji.fedoraproject.org/koji/buildinfo?buildID=1210898

Rawhide build:
https://koji.fedoraproject.org/koji/buildinfo?buildID=1210896

F29 update:
https://bodhi.fedoraproject.org/updates/FEDORA-2019-90c880c7cc

This still depends on the s390x exclusion bug (https://bugzilla.redhat.com/show_bug.cgi?id=1677491), but there's not much to do since the upstream code throws an error on big-endian systems. Let me know if some further actions are needed.

Comment 17 Fedora Update System 2019-02-18 03:05:39 UTC
libldac-2.0.2.2-4.fc29 has been pushed to the Fedora 29 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2019-90c880c7cc

Comment 18 Fedora Update System 2019-03-05 15:20:50 UTC
libldac-2.0.2.2-4.fc29 has been pushed to the Fedora 29 stable repository. If problems still persist, please make note of it in this bug report.