Bug 518546

Summary: Review Request: libva - VAAPI video playback acceleration
Product: [Fedora] Fedora Reporter: Adam Williamson <awilliam>
Component: Package ReviewAssignee: Peter Robinson <pbrobinson>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: ajax, bjohnson, bnocera, dnehring, dwmw2, fedora-package-review, felix, ktdreyer, kwizart, notting, otte, pbrobinson, rdieter, supercyper1, tcallawa
Target Milestone: ---Flags: pbrobinson: fedora-review+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: libva-1.0.10-1.fc15 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2011-02-21 17:59:07 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:    
Bug Blocks: 583236    
Attachments:
Description Flags
proposed .src.rpm with i965 driver removed
none
spec file for i965-removed build
none
updated srpm none

Description Adam Williamson 2009-08-20 20:21:32 UTC
Spec URL: http://www.happyassassin.net/extras/libva.spec
SRPM URL: http://www.happyassassin.net/extras/libva-0.30.4.1.sds3-1.aw_fc12.src.rpm
Description: libva contains a library and tools relating to the VAAPI video playback acceleration specification. At present the only really working implementations of VAAPI support on the video driver side are in two proprietary drivers (VIA's own driver for its Chrome chipsets, and the infamous Intel Poulsbo driver), but there's a borderline-functional implementation of MPEG-2 acceleration for mainline Intel chipsets in the package, and more free implementations are definitely planned for the future, so this isn't like VDPAU, and the Packaging Committee meeting which discussed VDPAU was firmly in favour of allowing libva in.

This package is the patched version of libva maintained by Gwenole Beauchesne of Splitted Desktop Systems, at http://www.splitted-desktop.com/~gbeauchesne/libva/ . Upstream libva is slated to include his changes from version 0.50 onwards. Some already seem to be merged, but not all, and building current upstream git doesn't give a libva that works with Poulsbo, so I'd prefer to go with Gwenole's patched version for now. I've got a spec on file for the upstream version too, so it'll be easy to switch when appropriate. Upstream libva lives at http://www.freedesktop.org/wiki/Software/vaapi , just for reference.

Comment 1 Nicolas Chauvet (kwizart) 2009-08-21 10:00:01 UTC
I think you are wrong, libva upstream is here:
http://www.freedesktop.org/wiki/Software/vaapi

The work of "Gwenole Beauchesne" is beeing merged in the freedesktop upstream project.

I'm currently in vacation, so I cannot help with this until next week, but currently packaging libva seems prematured. (actually I cannot test any native implementation since I don't own such hardware).

Comment 2 Adam Williamson 2009-08-21 14:58:33 UTC
Err. That's exactly what I said:

"This package is the patched version of libva maintained by Gwenole Beauchesne
of Splitted Desktop Systems, at
http://www.splitted-desktop.com/~gbeauchesne/libva/ . Upstream libva is slated
to include his changes from version 0.50 onwards...Upstream libva lives at
http://www.freedesktop.org/wiki/Software/vaapi , just for reference."

The Poulsbo implementation - which I'm currently working on packaging into RPM Fusion - is already usable and very useful to Poulsbo owners, since Poulsbo-based systems do not have the horsepower to manage decent video playback without hardware acceleration. Therefore I don't see this as 'premature', it will already be useful, and there's no 'disadvantage' to having something that will become useful packaged early anyway.

-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

Comment 3 Adam Williamson 2009-08-31 19:30:52 UTC
Updated to Gwenole's latest patch version, which adds OpenGL extensions. Spec in the same place. New .src.rpm is http://www.happyassassin.net/extras/libva-0.30.4.1.sds5-2.fc11.src.rpm .

-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

Comment 4 Adam Williamson 2009-08-31 19:31:24 UTC
damn, wrong URL:

http://adamwill.fedorapeople.org/poulsbo/src/libva-0.30.4.1.sds5-2.fc11.src.rpm

-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

Comment 5 Adam Williamson 2009-09-03 22:07:48 UTC
thanks, ismael. updated the spec at the same URL as before: now it doesn't set the stack executable on the libva.so.0 that's generated. that's the only change. will link to updated .src.rpm shortly.

-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

Comment 6 Adam Williamson 2009-09-04 03:52:35 UTC
updated SRPM: http://adamwill.fedorapeople.org/video-experimental/rawhide/SRPMS/libva-0.30.4.1.sds5-3.fc12.src.rpm

-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

Comment 7 Adam Williamson 2009-09-04 17:56:22 UTC
I've realized there are potential patent issues with this package. It includes one working backend, i965_drv_video, which does accelerated MPEG decoding on Intel i965 hardware.

I'm not sufficiently knowledgeable about the applicable patents in this area, nor enough of a coder to understand exactly what gets done in the code in libva as opposed to what gets done in the hardware on the card, so I don't know if this component of the package is shippable in Fedora or not. We can always ship the package without it, but I'd like to find out whether that's necessary. Hence, blocking FE-LEGAL.

The code in question is in the i965_drv_video/ directory within the source tarball in the SRPM. Thanks!

-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

Comment 8 Adam Williamson 2009-09-04 18:07:35 UTC
sorry, updated SRPM link (I realized putting mplayer builds on fedorapeople was a bad idea...):

http://www.happyassassin.net/video-experimental/rawhide/SRPMS/libva-0.30.4.1.sds5-3.fc12.src.rpm

-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

Comment 9 Adam Williamson 2009-09-14 20:47:33 UTC
ismael, you took ownership of this review but don't seem to have done anything with it. could you please either review the package or release it so someone else can?

btw, latest version of SRPM:

http://www.happyassassin.net/video-experimental/rawhide/SRPMS/libva-0.31.0.1.sds3-1.fc12.src.rpm

spec link has also been updated.

-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

Comment 10 Ismael Olea 2009-09-14 22:07:18 UTC
I thought you were expecting the FE-LEGAL resolution :-m

Comment 11 Adam Williamson 2009-09-14 22:15:45 UTC
oh, damn, yes, sorry, you're quite right. It's been a busy day :|. sorry! i'll bug them instead.

-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

Comment 12 Adam Williamson 2009-09-24 16:40:24 UTC
quick update: since this is a technical/legal overlap, spot and ajax will look at it together. adding ajax to CC. spot tells me ajax will not have time to check it out immediately; that's fine, there's no big rush (though for selfish purposes i'd like to be free from the hassle of maintaining it in my own repository asap :>)

-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

Comment 13 Ismael Olea 2009-10-10 11:21:02 UTC
I can't get this review for momment. Sorry for the inconvenience.

Comment 14 Bernard Johnson 2009-11-22 23:23:33 UTC
Hunted around and found most recent SRPM:
http://www.happyassassin.net/video-experimental/rawhide/SRPMS/libva-0.31.0.1.sds7-1.fc13.src.rpm
(which contained +sds5? +sds8 being the newest available)


I could probably give you a review once it clears legal.  It appears I also have hardware that I can test with:

$ vainfo
libva: libva version 0.31.0-sds3
libva: va_getDriverName() returns 0
libva: Trying to open /usr/lib64/va/drivers/i965_drv_video.so
libva: va_openDriver() returns 0
vainfo: VA API version: 0.31
vainfo: Driver version: i965 Driver 0.1
vainfo: Supported profile and entrypoints
      VAProfileMPEG2Simple            :	VAEntrypointVLD
      VAProfileMPEG2Main              :	VAEntrypointVLD

Comment 15 Adam Williamson 2009-11-23 20:09:49 UTC
Oh bugger, you just found me a bug - that SRPM is _supposed_ to contain sds7, but I didn't update the relevant macro so it doesn't. Heh. I'll bump to sds8 and do it right. Thanks.

legal update - we've established the i965 plugin definitely does evil stuff so Fedora will need a build that drops that code. ajax is also not willing to commit himself to the core bits of libva not doing anything evil, yet, so still waiting. I'll try and stick an updated srpm without the i965 plugin into this report soon.

-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

Comment 16 Rex Dieter 2010-04-08 17:42:13 UTC
We may be wanting this in fedora sooner rather than later, if the kde-sig starts pushing to get phonon-vlc packaged.

Any news on either the legal(spot) or technical(ajax) fronts?

Comment 17 Nicolas Chauvet (kwizart) 2010-04-08 18:23:39 UTC
So far, vaapi output will be pointless without ffmpeg that's not how vlc uses vaapi at least. There is a qffmpeg in rhel, it is outdated but could probably solve this side of the problem if acceptable by legal and technically makes sense.

Comment 18 Adam Williamson 2010-04-08 18:55:48 UTC
the thing is, acceleration is only available for codecs which are legally encumbered, AIUI: MPEG-2, h264, VC-1. Nothing supported by libva actually does acceleration of a non-encumbered codec, yet.

So you're always going to hit the patent wall at some point. I'd still like to have libva in Fedora just to make things more convenient and allow for the situation where someone does implement support for an unencumbered codec, but for now it's not the case.



-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

Comment 19 Chen Lei 2010-07-27 15:09:29 UTC
Any news on this package? Some Meego 1.1 packages need libva 1.0 to be included in Fedora.

Comment 20 Chen Lei 2010-07-27 15:16:45 UTC
Hi Adam,

It seems Meego and debian already include this package in their repos(version 1.0.x). Can we lift FE-legal now or it there any FE-legal persons to clasify the patent issue? 

Debian package:
http://packages.debian.org/source/sid/libva

Meego package:

http://repo.meego.com/MeeGo/builds/trunk/daily/core/repos/source/libva-1.0.3-1.5.src.rpm

Comment 21 Adam Williamson 2010-07-27 15:27:26 UTC
We always go with Red Hat / Fedora Legal's take, not what other distros do, so no, we can't. Still waiting on a legal review of the package without i965 plugin.

Note that the package is now in RPM Fusion - https://bugzilla.rpmfusion.org/show_bug.cgi?id=1321 .



-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

Comment 22 Chen Lei 2010-07-27 15:42:41 UTC
(In reply to comment #21)
> We always go with Red Hat / Fedora Legal's take, not what other distros do, so
> no, we can't. Still waiting on a legal review of the package without i965
> plugin.
> Note that the package is now in RPM Fusion -
> https://bugzilla.rpmfusion.org/show_bug.cgi?id=1321 .
> -- 
> Fedora Bugzappers volunteer triage team
> https://fedoraproject.org/wiki/BugZappers    

Actually, libva is inclued in all major linux distributions(debian/ubuntu opensuse mandriva gentoo arch) except Fedora.
I'll post a request on Legal list for help soon :), this Review Request waits for a legal review for nearly one year. We need this package to be included in fedora not rpmfusion, otherwise we have to miss the whole meego MTF spin in Fedora.

Can you help to contact spot or ajax? Is there any other one familiar with libva?

Regards,
Chen Lei

Comment 23 Tom "spot" Callaway 2010-07-27 17:00:26 UTC
At a minimum, the i965 code in this package is not acceptable for inclusion in Fedora. That begs the question of whether it would be better to separate it out or simply push it all to rpmfusion.

Comment 24 Tom "spot" Callaway 2010-07-27 17:01:07 UTC
(In reply to comment #22)

> I'll post a request on Legal list for help soon :), this Review Request waits
> for a legal review for nearly one year. We need this package to be included in
> fedora not rpmfusion, otherwise we have to miss the whole meego MTF spin in
> Fedora.

What in meego depends on libva?

Comment 25 Chen Lei 2010-07-27 17:18:53 UTC
(In reply to comment #24)
> (In reply to comment #22)
> > I'll post a request on Legal list for help soon :), this Review Request waits
> > for a legal review for nearly one year. We need this package to be included in
> > fedora not rpmfusion, otherwise we have to miss the whole meego MTF spin in
> > Fedora.
> What in meego depends on libva?    

Thanks a lot, Tom. Meego netbook spin(based on moblin) currently don't have a dependency on libva, however Netbook MTF(multitouchframework) spin depends on it.

libva is also used by clutter-gst, gnash and totem which is already included in fedora. i965 part is a drive and can be packaged seperately, there are some other 3rd party drive/backend available for libva.


See http://freedesktop.org/wiki/Software/vaapi

Comment 26 Tom "spot" Callaway 2010-07-27 17:34:50 UTC
My understanding was that all of the other non-i965 backend/drivers for libva were not legally acceptable for Fedora.

So, I'm wondering how the Netbook MTF spin actually depends on libva.

Comment 27 Bill Nottingham 2010-07-27 17:40:26 UTC
Siimlarly, it's not as if libva is a blocker for the inclusion of totem, gnash, clutter-gst, etc.

Comment 28 Adam Williamson 2010-07-27 19:01:56 UTC
Right. They can use it, but they don't *need* it. If you install gstreamer-vaapi then gstreamer-based players like Totem will have libva available as a decoder, if you don't, they'll just use something else (non-accelerated, of course). There's no need to have a dependency anywhere afaik. If you want to build the vaapi support into gnash then you have to build gnash against libva, it's true, but then gnash sucks ass so I don't think anyone really cares about that. It's not really usable for most people. I haven't even bothered building it in my private repo, for that reason.

I believe spot's correct that there's still no backend that's not encumbered in some way. Though given that we've accepted WebM, if someone were to write a backend for, say, Intel which did WebM decoding, we could presumably take that.



-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

Comment 29 Adam Williamson 2010-07-27 19:08:18 UTC
btw, libva is in Mandriva because I put it there. The legal review process in Mandriva consists of "Is anyone suing us right now? No? We're probably good, then". =)



-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

Comment 30 Tom "spot" Callaway 2010-07-27 19:15:55 UTC
(In reply to comment #29)
> btw, libva is in Mandriva because I put it there. The legal review process in
> Mandriva consists of "Is anyone suing us right now? No? We're probably good,
> then". =)

Also, Debian/Ubuntu/Gentoo are known to simply ignore patent issues.

Comment 31 Tom "spot" Callaway 2010-07-27 19:17:00 UTC
Adam, basically, this is waiting on you at this point. Either make a package that doesn't have the i965 code (at all, not even in the source), or close out this ticket and work on it in rpmfusion. I'm leaving it blocked against FE-Legal until I see a "clean" package I can review.

Comment 32 Adam Williamson 2010-07-27 19:42:29 UTC
Um, really? I already asked months ago if you could simply review the code apart from the i965 plugin, which I would drop if the rest of the code was OK. If you were waiting for me to build an SRPM before doing that (which seems a bit unnecessary) you could at least have *told* me :(



-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

Comment 33 Tom "spot" Callaway 2010-07-27 20:00:38 UTC
Apologies. I just decided that today, previously, I was hopeful that ajax would have time to do another audit pass on the code without the i965 stuff, but I think I'm comfortable at this point with the libva bits as is, since any actual work would happen in the driver/backends.

Comment 34 Tom "spot" Callaway 2010-07-27 20:01:19 UTC
(In reply to comment #33)
> Apologies. I just decided that today, previously, I was hopeful that ajax would
> have time to do another audit pass on the code without the i965 stuff, but I
> think I'm comfortable at this point with the libva bits as is, since any actual
> work would happen in the driver/backends.    

To be absolutely clear, when i say "as is", I mean "with the i965 stuff removed entirely".

Comment 35 Adam Williamson 2010-07-27 20:24:37 UTC
yep, I understand. I'll co-ordinate with the fusion maintainer and we'll submit a build with the 965 driver removed.



-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

Comment 36 Chen Lei 2010-07-28 02:17:20 UTC
(In reply to comment #30)
> (In reply to comment #29)
> > btw, libva is in Mandriva because I put it there. The legal review process in
> > Mandriva consists of "Is anyone suing us right now? No? We're probably good,
> > then". =)
> 
> Also, Debian/Ubuntu/Gentoo are known to simply ignore patent issues.    

Correct, Debian ignored many decoder patents, but they are also aware of encoder patents. If the main part libva is patent free, I think it's not dangerous for Fedora to include it, because one year ago FESCo permitted a similar package - libvdpau[1] to go into fedora which is useless without Nvidia binary driver. 


[1]https://fedorahosted.org/fesco/ticket/238

Comment 37 Bastien Nocera 2010-08-10 13:05:03 UTC
(In reply to comment #23)
> At a minimum, the i965 code in this package is not acceptable for inclusion in
> Fedora. That begs the question of whether it would be better to separate it out
> or simply push it all to rpmfusion.    

Could you explain why? The code there seems to be a demuxer, and at worst, a simple parser, which matches things we already ship in gstreamer-plugins-bad-free (we ship MPEG demuxers, parsers, but not decoders).

Most of that code seems to me like it's just pushing bits of data to the GPU, nothing more.

Comment 38 Tom "spot" Callaway 2010-08-11 16:04:27 UTC
(In reply to comment #37)
> (In reply to comment #23)
> > At a minimum, the i965 code in this package is not acceptable for inclusion in
> > Fedora. That begs the question of whether it would be better to separate it out
> > or simply push it all to rpmfusion.    
> 
> Could you explain why? The code there seems to be a demuxer, and at worst, a
> simple parser, which matches things we already ship in
> gstreamer-plugins-bad-free (we ship MPEG demuxers, parsers, but not decoders).
> 
> Most of that code seems to me like it's just pushing bits of data to the GPU,
> nothing more.    

Ajax disagreed.

Comment 39 Benjamin Otte 2010-08-11 16:12:58 UTC
I would expect the code in http://cgit.freedesktop.org/libva/tree/i965_drv_video/shaders to be problematic. Though I completely suck at reading Intel's GPU asm, so I cannot say anything but point at the directory names. ;)

Comment 40 Nicolas Chauvet (kwizart) 2010-09-07 09:09:59 UTC
Does this mean there is a agreement to clear the Fe-Legal blocker once the i965 backend is removed ?
@Adam 
Does that make sense for you ? In which case I will take the review.

Comment 41 Tom "spot" Callaway 2010-09-07 15:21:32 UTC
Yep. Once I see a package with a clean source tarball that has none of the i965 bits in it, I will lift FE-Legal.

Comment 42 Adam Williamson 2010-09-08 12:42:04 UTC
nic: I'll try and stick a package based on the current RPM Fusion package with the i965 stuff removed here, but it may go faster if you do it and send it to me so I can review it and submit it here. Just predicting my own laziness =)

Comment 43 Adam Williamson 2010-10-07 18:05:37 UTC
Here's a .src.rpm with the whole i965 driver removed, I think I followed project guidelines on how to do this (I included a -mod tarball with instructions in the spec file on how to produce it).

Comment 44 Adam Williamson 2010-10-07 18:06:44 UTC
Created attachment 452172 [details]
proposed .src.rpm with i965 driver removed

Comment 45 Adam Williamson 2010-10-07 18:07:30 UTC
Created attachment 452173 [details]
spec file for i965-removed build

Comment 46 Tom "spot" Callaway 2010-10-14 14:55:24 UTC
Adam, the spec file in comment 45 doesn't match what falls out of the srpm in comment 44. Can you resolve that inconsistency?

Comment 47 Adam Williamson 2010-11-22 23:52:09 UTC
no idea why, but I just did another rpmbuild -bs from that libva.spec, here it is.



-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

Comment 48 Adam Williamson 2010-11-22 23:52:56 UTC
Created attachment 462180 [details]
updated srpm

Comment 49 Tom "spot" Callaway 2010-11-23 15:13:36 UTC
Adam, a few points here:

1) When you're making a clean tarball, you probably don't want to point to the URL for the original, with the modified tarball name, as that isn't valid. Just put the link for the original in a comment.

2) The i965_drv_video/ directory is still present. This is clearly a problem. :)

Comment 50 Adam Williamson 2010-11-23 16:23:47 UTC
sigh. thanks. attempt #2 coming.



-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

Comment 52 Tom "spot" Callaway 2010-11-23 19:29:38 UTC
Legal concerns are resolved, thanks for your patience Adam. Lifting FE-Legal.

Comment 53 Adam Williamson 2010-11-23 21:25:09 UTC
updated to drop the pointless provides/obsoletes of itself (hangover from the rpmfusion package):

http://www.happyassassin.net/extras/libva.spec
http://www.happyassassin.net/extras/libva-0.31.1-3.sds4.aw_fc14.src.rpm

can someone review this now? thanks! rpmlint output is clean.



-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

Comment 54 Adam Williamson 2010-11-23 21:25:30 UTC

-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

Comment 55 Jason Tibbitts 2010-11-23 23:31:14 UTC
OK, let's take a look.  Seems to build OK; rpmlint says:

  libva.x86_64: W: no-manual-page-for-binary vainfo
It's nice to have manpages for binaries, but not essential.

  libva-devel.x86_64: E: useless-provides libva-devel
Packages automatically provide themselves; there is no point in doing it manually.

  libva-devel.x86_64: W: no-documentation
Not a big deal.

  libva.x86_64: W: unused-direct-shlib-dependency
   /usr/lib64/libva-tpi-0.31.1.1.so.1.0.3 /lib64/libdl.so.2
  libva.x86_64: W: unused-direct-shlib-dependency
   /usr/lib64/libva-compat.so.0.0.0 /usr/lib64/libva-x11-0.31.1.1.so.1
  libva.x86_64: W: unused-direct-shlib-dependency
   /usr/lib64/libva-compat.so.0.0.0 /usr/lib64/libva-0.31.1.1.so.1
  libva.x86_64: W: unused-direct-shlib-dependency
   /usr/lib64/libva-compat.so.0.0.0 /usr/lib64/libX11.so.6
  libva.x86_64: W: unused-direct-shlib-dependency
   /usr/lib64/libva-compat.so.0.0.0 /usr/lib64/libXext.so.6
  libva.x86_64: W: unused-direct-shlib-dependency
   /usr/lib64/libva-compat.so.0.0.0 /usr/lib64/libdrm.so.2
  libva.x86_64: W: unused-direct-shlib-dependency
   /usr/lib64/libva-compat.so.0.0.0 /usr/lib64/libXfixes.so.3
  libva.x86_64: W: unused-direct-shlib-dependency
   /usr/lib64/libva-glx-0.31.1.1.so.1.0.3 /usr/lib64/libXext.so.6
  libva.x86_64: W: unused-direct-shlib-dependency
   /usr/lib64/libva-glx-0.31.1.1.so.1.0.3 /usr/lib64/libdrm.so.2
  libva.x86_64: W: unused-direct-shlib-dependency
   /usr/lib64/libva-glx-0.31.1.1.so.1.0.3 /usr/lib64/libXfixes.so.3
Those libraries are all linked against things they don't actually call.  This isn't generally a big problem unless it causes additional dependencies or forces libraries to be pulled into memory that otherwise wouldn't be there.  It can usually be fixed by passing --as-needed into the linker.

I would anticipate this library being multilib, but it seems to me that the vainfo executable will cause a conflict.  Am I wrong?  Seems better living in a libva-utils package, honestly, but perhaps I've failed to properly understand multilib.

It would be nice to have more explicit instructions for modifying the tarball.  Otherwise the next person who has to do it risks letting the prohibited bits back in.  I did a diff and of course it's huge due to different autotools versions; I can't really figure out what changed.  There are even several files in the new tarball that weren't in the old one.  It might be simpler just to run the autotools as part of the build process.

I don't know if you intend to target RHEL4 or 5 with this, but if not you can drop BuildRoot, %clean and the first line of %install.

Comment 56 Adam Williamson 2010-11-24 01:13:05 UTC
"  libva-devel.x86_64: E: useless-provides libva-devel
Packages automatically provide themselves; there is no point in doing it
manually."

that's just another left-over from the rpmfusion libva-freeworld obsoleting my side repo libva; i'll fix it.

"Those libraries are all linked against things they don't actually call.  This
isn't generally a big problem unless it causes additional dependencies or
forces libraries to be pulled into memory that otherwise wouldn't be there.  It
can usually be fixed by passing --as-needed into the linker."

I thought that was already done in our default optflags?

"Seems better living in a libva-utils package, honestly, but perhaps I've failed to properly understand multilib."

This isn't really Fedora policy, most things aren't multilib in Fedora and we don't apparently split libs from binaries as a matter of course. Only things you might actually want to install the 32-bit version of on a 64-bit system are explicitly marked multilib. I can't see any reason you'd want to do that with libva.

"It would be nice to have more explicit instructions for modifying the tarball. 
Otherwise the next person who has to do it risks letting the prohibited bits
back in."

What do you think is missing? The comments list the entire sequence. The only thing you have to figure out for yourself is how to 'rediff 108_drivers_path.patch' but it's inherently difficult to explain that exactly because it may change between releases. All the other changes are consequences of the steps listed in the comments, there's nothing left out. Try it yourself and you'll see. (the 'autoreconf -i' step causes most of the changes). AIUI running autoreconf in the spec would not be optimal because it would leave some i965 references in the tarball, they aren't purged until the autoreconf step is run.

"I don't know if you intend to target RHEL4 or 5 with this, but if not you can
drop BuildRoot, %clean and the first line of %install."

yeah, I know, I guess I'll do that. Though someone might want to pull it into EPEL, I guess, and they're not doing much harm.



-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

Comment 57 Chen Lei 2010-11-24 03:16:54 UTC
Why not to use upstream libva instead of a forked version from splitted Desktop Systems? It seems both Debian and Meego all packaged the upstream one.

Comment 58 Jason Tibbitts 2010-11-24 04:37:44 UTC
(In reply to comment #56)
> I thought that was already done in our default optflags?

'tis not, I'm afraid.  Not that it's a big deal anyway.

> This isn't really Fedora policy, most things aren't multilib in Fedora and we
> don't apparently split libs from binaries as a matter of course. Only things
> you might actually want to install the 32-bit version of on a 64-bit system are
> explicitly marked multilib.

Perhaps the process has changed; it used to involve a magic formula and the presence of a -devel package.  Looking at the mash code, though, I'm pretty sure this will end up multilibbed.  Unless I'm misunderstanding the mash code, of course.

> I can't see any reason you'd want to do that with libva.

It's not something that someone explicitly does.  And even if I'm wrong and it turns out not to be multilibbed, it may later become multilibbed if some multilib package enters the distro which depends on it.

So, yeah, pretty sure it's going to be multilib and you'll have a conflict with the file in /usr/bin.  But if you want, sure, wait to see what the first mash does.

> What do you think is missing? The comments list the entire sequence.

I guess what's throwing me is the fact that usually this kind of modified tarball thing involves simply deleting the bad content, so a recursive diff would just list a few deleted files instead of giving a few thousand lines of modifications.  Fixing up things to build and running autotools and such would normally be done within the spec itself.  To see such a huge diff starts to look more like an internal fork than a simple deletion of a few files.

> yeah, I know, I guess I'll do that. Though someone might want to pull it into
> EPEL, I guess, and they're not doing much harm.

Oh, there's no requirement to remove that stuff.  I just remind people that it can be done.

Comment 59 Adam Williamson 2010-11-24 05:06:13 UTC
I certainly never heard anything about most things defaulting to multilib. I'll check with Jesse I think.

It didn't seem neat to ship a tarball with nothing changed but the 'bad stuff' removed, because that tarball would in itself be useless; it'd completely fail to compile. It seems odd to ship a package which contains a source tarball that's useless and requires further  changes from the spec to work at all. I'm not sure if there's an 'official' preferred process here, I'll see if I can find anything.



-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

Comment 60 Adam Williamson 2010-11-24 05:07:19 UTC
chen: there's no great reason any more, there's not a lot of difference. I started with the sds builds because they were significantly better than upstream, but most of gwenole's changes have been merged now. We could probably switch to upstream, I guess.



-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

Comment 61 Adam Williamson 2010-11-24 18:57:27 UTC
actually, sds branch still has one significant advantage: support for Poulsbo, which is why I packaged this in the first place and, along with Intel, the two platforms on which libva is actually really useful. The psb driver was built to support an old version of libva, 0.29, and sds includes a compatibility shim for 0.29. Upstream doesn't have this, and the patch isn't trivial to port to upstream. so I'd like to stick with sds for now, until it gets merged or a new Poulsbo driver comes out.



-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

Comment 62 Chen Lei 2010-11-25 02:32:39 UTC
(In reply to comment #61)
> actually, sds branch still has one significant advantage: support for Poulsbo,
> which is why I packaged this in the first place and, along with Intel, the two
> platforms on which libva is actually really useful. The psb driver was built to
> support an old version of libva, 0.29, and sds includes a compatibility shim
> for 0.29. Upstream doesn't have this, and the patch isn't trivial to port to
> upstream. so I'd like to stick with sds for now, until it gets merged or a new
> Poulsbo driver comes out.
> 
The upstream version is binary incompatiable with sds version, so we can't test many 3rdparty meego packages if we include sds one in fedora repo.

Maybe you can find a new Poulsbo driver in meego daily repo:
http://repo.meego.com/MeeGo/builds/trunk/daily/non-oss/repos/source/

Comment 63 Adam Williamson 2010-11-25 17:37:57 UTC
ew, what a horrible binary pile of shit. not interested.

I'll probably be able to do the upstream build with gwenole's backwards compatibility patches added, but I need to test to see exactly how to set up the library files first, I'm doing that but got distracted with egroupware stuff yesterday.



-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

Comment 64 Adam Williamson 2010-11-26 17:49:25 UTC
so far it looks like getting libva 1.x to fly with psb is a no-go, which kind of leaves us in an awkward position - honestly I packaged libva for the purpose of working with psb and I'm not that interested in packaging 1.x if it won't work with psb.

We could have two packages, I guess: libva and libva-sds or something like that. If all you need the upstream version for is to test external packages against, we could build Fedora / Fusion packages against the sds libva, and that would probably make everyone happyish?

Comment 65 Nicolas Chauvet (kwizart) 2010-11-26 19:00:34 UTC
Would it be possible to keep the sd version for current fedora and import the 1.x only for rawhide ? 
That way, work can be done to enable moblin based on 1.x.
Even if psb isn't ready for the f15 timeline, we could forward these users to a later release, targeting f16 if they want that functionality restored.

Comment 66 Adam Williamson 2010-11-26 20:45:27 UTC
possible, yeah. I still kind of favour the Two Package Solution though. What's the advantage of your approach over that?

Comment 67 Nicolas Chauvet (kwizart) 2010-11-26 22:16:54 UTC
Since these two libva packages are binary incompatible, doesn't that mean two FFmpeg built with one and the other ?
And then same for several vaapi capable players that need to be built with the libva matching the ffmpeg built they rely upon, so it doesn't crash at runtime because of one or another missing symbol ?

I don't think that's reliable at all.

Comment 68 Adam Williamson 2010-11-26 22:23:15 UTC
my suggestion is to have everything in the fedoraverse (so, mostly fusion stuff) built against libva-sds and just have libva1 for chen's desired purpose (testing external packages that are built against that libva version).

Comment 69 Adam Williamson 2011-01-07 11:05:50 UTC
okay, I guess I'll resign myself to maintaining a variant libva for psb use, and just switch this review over to the upstream version. package coming.



-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

Comment 70 Adam Williamson 2011-01-07 16:31:58 UTC
Here's a spec / src for a build of 1.0.7 from upstream.

My plan for now is to have this in Fedora, a package containing only the i965 driver in a third-party repository for people for whom the patent is not a problem, and I will ship a libva-sds in my side repo for Poulsbo users. That should keep everyone happy, I guess.



-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

Comment 72 Peter Robinson 2011-01-24 10:25:16 UTC
I'll review this

Comment 73 Peter Robinson 2011-01-24 11:38:08 UTC
In the 1.0.7 update it looks like the intel i965 wasn't completely stripped. The directory is still there although the configure/Makefile looks OK. 

Other than that there's just the rpmlint error below. The rest looks OK to me.

- rpmlint output
E: useless-provides libva-devel
I presume that's suppose to obsolete libva-freeworld and libva-freeworld-devel from rpmfusion

rpmlint libva-1.0.7-1.fc15.src.rpm libva-1.0.7-1.fc15.x86_64.rpm libva-devel-1.0.7-1.fc15.x86_64.rpm libva.spec 
libva.src:16: W: macro-in-comment %{version}
libva.src: W: invalid-url Source0: libva-1.0.7-mod.tar.bz2
libva.x86_64: W: no-manual-page-for-binary vainfo
libva-devel.x86_64: E: useless-provides libva-devel
libva-devel.x86_64: W: no-documentation
libva.spec:16: W: macro-in-comment %{version}
libva.spec: W: invalid-url Source0: libva-1.0.7-mod.tar.bz2
3 packages and 1 specfiles checked; 1 errors, 6 warnings.

+ package name satisfies the packaging naming guidelines
+ specfile name matches the package base name
+ package should satisfy packaging guidelines
+ license meets guidelines and is acceptable to Fedora
+ license matches the actual package license
+ latest version packaged
  1.0.7
+ %doc includes license file
+ spec file written in American English
+ spec file is legible
+ upstream sources match sources in the srpm
  3903b0463e2892f8afbd631daa5619ae  libva-1.0.7.tar.bz2 (original)
  this seems OK based on my checking of the process documented in the .spec
+ package successfully builds on at least one architecture
  tested using koji scratch build
  http://koji.fedoraproject.org/koji/taskinfo?taskID=2738557
+ BuildRequires list all build dependencies
n/a %find_lang instead of %{_datadir}/locale/*
+ binary RPM with shared library files must call ldconfig in %post and %postun+ does not use Prefix: /usr
n/a package owns all directories it creates
n/a no duplicate files in %files
+ Package perserves timestamps on install
+ Permissions on files must be set properly 
+ %defattr line
+ consistent use of macros
+ package must contain code or permissible content
n/a large documentation files should go in -doc subpackage
+ files marked %doc should not affect package runtime 
+ header files should be in -devel
n/a static libraries should be in -static
+ packages containing pkgconfig (.pc) files need 'Requires: pkgconfig'
+ libfoo.so must go in -devel
+ devel must require the fully versioned base
+ packages should not contain libtool .la files
n/a packages containing GUI apps must include %{name}.desktop file
+ packages must not own files or directories owned by other packages
+ filenames must be valid UTF-8

Optional:

+ if there is no license file, packager should query upstream to include it
n/a translations of description and summary for non-English languages, if
available
+ reviewer should build the package in mock/koji
+ the package should build into binary RPMs on all supported architectures
n/a review should test the package functions as described
+ scriptlets should be sane
n/a non -devel packages should require fully versioned base
+ pkgconfig files should go in -devel
+ shouldn't have file dependencies outside /etc /bin /sbin /usr/bin or /usr/sbin
? Package should have man files
  Not a big problem

Comment 74 Nicolas Chauvet (kwizart) 2011-01-24 12:00:02 UTC
Please do not Obsoletes libva-freeworld. This package will still be needed since '3rd repo' rely on the Splitted Desktop version which is binary incompatible with the Freedesktop version.
As such, I would like not to introduce this package beyond rawhide for now; since this will requires a rebuild of current released packages.
Note that the vdpau-video package does indeed mandatory requires the SD version.
And for the record, as the hardware support range for this package is currently shorter than the SD version, that's unlikely that packages provided by '3rd repo' will link to this version.

Comment 75 Adam Williamson 2011-01-24 15:48:16 UTC
"Please do not Obsoletes libva-freeworld. This package will still be needed
since '3rd repo' rely on the Splitted Desktop version which is binary
incompatible with the Freedesktop version."

I don't think libva-freeworld is an appropriate package name for a build of the SDS fork.

"Note that the vdpau-video package does indeed mandatory requires the SD
version."

You keep saying that, but I haven't seen any proof; I'm pretty sure I built vdpau-video against upstream libva already...Gwenole himself told me there's almost no point to the SDS fork except Poulsbo, now.

"And for the record, as the hardware support range for this package is currently
shorter than the SD version, that's unlikely that packages provided by '3rd
repo' will link to this version."

Honestly, there's a limit to how long we can carry around an obsolete library just to make poulsbo's shitty driver happy, and I say that as the maintainer of poulsbo's shitty driver...

Comment 76 Nicolas Chauvet (kwizart) 2011-01-25 17:28:56 UTC
Taken from vdpau-video configure.ac (only the relevant part).

if test "$ac_cv_libva_sds_extensions" = "yes"; then
    AC_DEFINE_UNQUOTED([VA_DRIVER_INIT_FUNC], [$VA_DRIVER_INIT_FUNC], [Define driver entry-point])
else
    AC_MSG_ERROR([Your VA-API SDK does not include SDS extensions])
fi


It would have been an AC_MSG_WARNING, it wouldn't have compiled either.

So if Adam said upstream reported it to work, either don't trust Adam or upstream, at your choice.

Comment 77 Adam Williamson 2011-01-25 17:41:33 UTC
I think you're looking at an old vdpau-video.

dnl Check for SDS extensions to VA-API
AC_CACHE_CHECK([for VA-API],
  ac_cv_libva_sds_extensions, [
  saved_CFLAGS="$CFLAGS"
  CFLAGS="$CFLAGS $LIBVA_DEPS_CFLAGS"
  AC_TRY_COMPILE([
    #include <va/va_version.h>
    #if VA_MAJOR_VERSION == 0 && VA_MINOR_VERSION == 29
    # if !defined(VA_SDS_VERSION) || (VA_SDS_VERSION < $LIBVA_SDS_VERSION_0_29)
    #  error "VA-API version >= 0.29.0-sds$LIBVA_SDS_VERSION_0_29 is required"
    # endif
    #elif VA_MAJOR_VERSION == 0 && VA_MINOR_VERSION == 30
    # if !defined(VA_SDS_VERSION) || (VA_SDS_VERSION < $LIBVA_SDS_VERSION_0_30)
    #  error "VA-API version >= 0.30.0-sds$LIBVA_SDS_VERSION_0_30 is required"
    # endif
    #elif !VA_CHECK_VERSION(0,31,0)
    # error "VA-API version >= 0.31 is required"
    #endif
  ], [],
  [ac_cv_libva_sds_extensions="yes"],
  [ac_cv_libva_sds_extensions="no"])
  CFLAGS="$saved_CFLAGS"
])

is what's in 0.7.3-pre4 .

Comment 78 Adam Williamson 2011-01-25 18:16:41 UTC
To put this to bed:

[adamw@adam SPECS]$ rpmbuild -ba vdpau-video.spec
...
+ /usr/bin/gzip -dc /home/adamw/rpmbuild/SOURCES/vdpau-video-0.7.3.pre4.tar.gz
...
checking for LIBVA_DEPS... yes
checking for LIBVA_X11_DEPS... yes
checking for LIBVA_GLX_DEPS... yes
checking for VA-API... yes
checking for VA-API (GLX extensions)... yes
checking for VA drivers path... /usr/lib64/dri
...
vdpau-video configuration summary:

VA-API version ................... : 0.31.1
VA-API drivers path .............. : /usr/lib64/dri
VDPAU version .................... : 1
VDPAU/MPEG-4 support ............. : yes
GLX support ...................... : yes
...
Wrote: /home/adamw/rpmbuild/SRPMS/vdpau-video-0.7.3-0.1.pre4.aw_fc15.src.rpm
Wrote: /home/adamw/rpmbuild/RPMS/x86_64/vdpau-video-0.7.3-0.1.pre4.aw_fc15.x86_64.rpm
Wrote: /home/adamw/rpmbuild/RPMS/x86_64/vdpau-video-debuginfo-0.7.3-0.1.pre4.aw_fc15.x86_64.rpm
...
[adamw@adam SPECS]$ rpm -q libva-devel
libva-devel-1.0.8-1.fc15.x86_64

Comment 79 Adam Williamson 2011-01-25 18:17:42 UTC
btw, I suggest we could keep libva-freeworld and libva-freeworld-devel in Fusion to contain patent-encumbered libva drivers - for now just the i965 driver, in future potentially others.

Comment 80 Adam Williamson 2011-01-25 18:19:54 UTC
updated spec and .src.rpm:

http://www.happyassassin.net/extras/libva.spec
http://www.happyassassin.net/extras/libva-1.0.8-1.aw_fc15.src.rpm

this is actually kind of a merge of two libva.spec generations I found I had lying around, with the newest version 1.0.8. It restores the -utils package for vainfo, so the main libva package can be multilib if necessary. It doesn't obsolete anything, per comment #79.

Comment 81 Peter Robinson 2011-01-25 19:29:29 UTC
OK. So:
+ problematic code removed
+ rpmlint errors fixed

Based on that: APPROVED.

Comment 82 Nicolas Chauvet (kwizart) 2011-01-25 22:24:10 UTC
I must apologies, the vdpau-video pre4 built with libva-1.0.8
I might have a remaining pre2 somewhere. What's stange is that the error was reproduced with pre4 as in (rfbz#562c) so it may be something with 1.0.7.

Anyway, let's go with upstream libva finally. That's rather recent to me to I'm still testing and investigating if it's worth to switch from libva SD to freedesktop in current releases...

Comment 83 Adam Williamson 2011-01-25 22:33:05 UTC
nic: the remaining advantage of sds is the backwards compat stuff for psb, I don't think there's really anything else. I'm planning to just put a libva-psb in my psb side repo, with a build of mplayer, as I always used to.

Comment 84 Adam Williamson 2011-01-26 05:31:13 UTC
New Package SCM Request
=======================
Package Name: libva
Short Description: VAAPI video playback acceleration
Owners: adamwill kwizart
Branches: f13 f14
InitialCC:

Comment 85 Jason Tibbitts 2011-01-26 17:26:24 UTC
Git done (by process-git-requests).

Comment 86 Fedora Update System 2011-02-15 11:05:13 UTC
libva-1.0.8-1.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/libva-1.0.8-1.fc15

Comment 87 Fedora Update System 2011-02-15 17:28:16 UTC
libva-1.0.8-1.fc15 has been pushed to the Fedora 15 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update libva'.  You can provide feedback for this update here: https://admin.fedoraproject.org/updates/libva-1.0.8-1.fc15

Comment 88 Dirk Nehring 2011-02-21 08:43:26 UTC
The current release is 1.0.10, where the VA-API changed to 0.32. Please consider adding this to Fedora 15.

Comment 89 Fedora Update System 2011-02-21 14:42:09 UTC
libva-1.0.10-1.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/libva-1.0.10-1.fc15

Comment 90 Adam Williamson 2011-02-21 17:59:07 UTC
the package is in, let's close this. thanks for pushing it, nic.

Comment 91 Fedora Update System 2011-03-08 02:28:34 UTC
libva-1.0.10-1.fc15 has been pushed to the Fedora 15 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 92 Dirk Nehring 2011-03-28 14:44:02 UTC
Before releasing F15 beta, we should update to 1.0.11:

http://cgit.freedesktop.org/libva/snapshot/libva-1.0.11.tar.bz2

Comment 93 Nicolas Chauvet (kwizart) 2011-03-28 14:56:42 UTC
(In reply to comment #92)
> Before releasing F15 beta, we should update to 1.0.11:
> 
> http://cgit.freedesktop.org/libva/snapshot/libva-1.0.11.tar.bz2
All changes are related to the backend which this package doesn't bundle. So it doesn't matter to update it.
Once that said the backend driver which is provided in 3rd part already have a git revision that match the content of 1.0.11.

Thx for the report, but it's uneeded.

Comment 94 Adam Williamson 2011-03-28 15:58:37 UTC
also, it doesn't have any particular relevance to the Beta release. it's not even shipped on media.

Comment 95 Ken Dreyer 2011-07-14 17:08:59 UTC
Package Change Request
======================
Package Name: libva
New Branches: el6
Owners: ktdreyer

Discussed with Adam, and I'll be maintaining the EL-6 branch.

Comment 96 Nicolas Chauvet (kwizart) 2011-07-14 17:51:48 UTC
I denied this CVS request. the driver that would make this wrapper anything usefull isn't capable to run on the libdrm version in EL6.
This request is dangerously pointless