Bug 553717

Summary: Review Request: libcrystalhd - Broadcom Crystal HD device interface library
Product: [Fedora] Fedora Reporter: Jarod Wilson <jarod>
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: alexl, bjohnson, bnocera, c.shoemaker, fedora-package-review, jarodwilson, notting, otte, pbrobinson
Target Milestone: ---Flags: pbrobinson: fedora‑review+
tibbs: fedora‑cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: libcrystalhd-0.9.25-2.fc13 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2010-03-12 21:34:47 EST Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:

Description Jarod Wilson 2010-01-08 13:21:09 EST
Spec URL: http://jwilson.fedorapeople.org/packaging/libcrystalhd/libcrystalhd.spec
SRPM URL: http://jwilson.fedorapeople.org/packaging/libcrystalhd/libcrystalhd-0.9.25-1.fc12.src.rpm
Description:

The libcrystalhd library provides userspace access to Broadcom Crystal HD
video decoder devices. The device supports hardware decoding of MPEG-2,
h.264 and VC1 video codecs, up to 1080p at 40fps.

The crystalhd kernel driver was accepted to the linux staging tree just a few days ago, and this library is the sole way one is supposed to interface with the decoder from userspace. It does also require a firmware image, which doesn't yet have a redistributable license applied to it, but Broadcom is working on making it so. For the moment, I'm using a modified tarball that strips out the firmware, but in the future, intend to produce a firmware sub-package straight off of this package.

While one might be slightly scared at first blush with the mention of mpeg2, h.264 and vc1 decoding, its done entirely in hardware, which has its own paid-for codec licenses, so this gives us a viable way to decode video on Fedora without any patent issues. Applications simply dma an encoded bitstream into the device, and it spits out raw decoded frames of video for you to do with as you please (i.e., display them on your screen).
Comment 1 Peter Robinson 2010-01-08 14:07:52 EST
I'll review this one
Comment 2 Jarod Wilson 2010-01-08 17:21:32 EST
Excellent, thanks much!

A few additional notes I neglected to mention... There's actually source for a gstreamer plugin for the decoder in here as well, but I'm not yet building it. It did build last I tried, and kind of worked at little bit, but not entirely. That code needs love from a gstreamer person before I'm particularly comfortable building binary packages of it.
Comment 3 Peter Robinson 2010-01-09 14:25:43 EST
Just taking an initial look at this. A couple of queries. The main package says it "Requires: crystalhd-firmware" but the firmware package in the rpm is called "bcm970012-firmware" are they the same?

Also where does the 0.9.25 version number come from.

And finally the gstreamer plugin would be really cool!
Comment 4 Jarod Wilson 2010-01-09 14:56:41 EST
(In reply to comment #3)
> Just taking an initial look at this. A couple of queries. The main package says
> it "Requires: crystalhd-firmware" but the firmware package in the rpm is called
> "bcm970012-firmware" are they the same?

That was a someone not entirely thought out attempt to get things looking sane-ish for when we add support for the bcm970015 as well, which uses a different firmware. The idea was to have both bcm970012-firmware and bcm970015-firmware packages, both of which Provides: cyrstalhd-firmware, then just Require: crystalhd-firmware in the main package, but of course, now that I think about it, that is potentially error-prone, as one might end up with the wrong firmware... I should probably just make a crystalhd-firmware package with the same version as the main package, and plan on putting both the 12 and 15 firmwares in it, then document the specific firmware file versions in a README. This all depends on Broadcom slapping an appropriate redistribution license on the firmware, of course... I'll clean that up soonish.

> Also where does the 0.9.25 version number come from.

Its the library version from within the library source code. I should probably look at fixing up the library build so that's actually evident in the soname...

> And finally the gstreamer plugin would be really cool!    

Indeed. Planning to ping Bastien Nocera about it next week...

Nb: I should add that I'm actually the upstream proxy maintainer of the open-source bits, working directly with Broadcom on it. They feed me changes, and vice versa. I'm mostly focused on the kernel driver though, and my library-fu is a bit lacking... I have both bcm970012 and bcm970015 hardware, the latter of which isn't yet supported by the kernel driver (I just got the hardware sent to me by Broadcom a few days ago).

Current development tree, which the patch in the srpm is generated from:
http://git.wilsonet.com/crystalhd.git/
Comment 5 Peter Robinson 2010-01-09 16:25:18 EST
> That was a someone not entirely thought out attempt to get things looking
> sane-ish for when we add support for the bcm970015 as well, which uses a
> different firmware. The idea was to have both bcm970012-firmware and
> bcm970015-firmware packages, both of which Provides: cyrstalhd-firmware, then
> just Require: crystalhd-firmware in the main package, but of course, now that I
> think about it, that is potentially error-prone, as one might end up with the
> wrong firmware... I should probably just make a crystalhd-firmware package with
> the same version as the main package, and plan on putting both the 12 and 15
> firmwares in it, then document the specific firmware file versions in a README.
> This all depends on Broadcom slapping an appropriate redistribution license on
> the firmware, of course... I'll clean that up soonish.

Cool, at about a meg for the firmware I would just put them both in the same rpm. It also makes is easier for general distro support for both devices as they can be installed at the same time. If they both provide crystalhd-firmware they'll conflict which will cause issues with supporting them in a general install.

> > Also where does the 0.9.25 version number come from.
> 
> Its the library version from within the library source code. I should probably
> look at fixing up the library build so that's actually evident in the soname...

Cool, looking at various files I had seen 0.1 versions, hence the query.

> > And finally the gstreamer plugin would be really cool!    
> 
> Indeed. Planning to ping Bastien Nocera about it next week...

Excellent :-)

> Nb: I should add that I'm actually the upstream proxy maintainer of the
> open-source bits, working directly with Broadcom on it. They feed me changes,
> and vice versa. I'm mostly focused on the kernel driver though, and my
> library-fu is a bit lacking... I have both bcm970012 and bcm970015 hardware,
> the latter of which isn't yet supported by the kernel driver (I just got the
> hardware sent to me by Broadcom a few days ago).
> 
> Current development tree, which the patch in the srpm is generated from:
> http://git.wilsonet.com/crystalhd.git/    

So nice to see Broadcom starting to place nice. Is there an eta for the fixing of the distribution of the firmware?
Comment 6 Jarod Wilson 2010-01-09 20:24:30 EST
(In reply to comment #5)
...
> Cool, at about a meg for the firmware I would just put them both in the same
> rpm. It also makes is easier for general distro support for both devices as
> they can be installed at the same time. If they both provide crystalhd-firmware
> they'll conflict which will cause issues with supporting them in a general
> install.

Okay, new build simply has a crystalhd-firmware sub-package.

> Cool, looking at various files I had seen 0.1 versions, hence the query.

Specifically, its from include/version_lnx.h:

/*======================= Device Interface Library ========================*/
#define DIL_MAJOR_VERSION       BRCM_MAJOR_VERSION
#define DIL_MINOR_VERSION       9
#define DIL_REVISION            25

I see what you mean with the Makefile, which does suggest v0.1...

> > Nb: I should add that I'm actually the upstream proxy maintainer of the
> > open-source bits, working directly with Broadcom on it. They feed me changes,
> > and vice versa. I'm mostly focused on the kernel driver though, and my
> > library-fu is a bit lacking... I have both bcm970012 and bcm970015 hardware,
> > the latter of which isn't yet supported by the kernel driver (I just got the
> > hardware sent to me by Broadcom a few days ago).
> > 
> > Current development tree, which the patch in the srpm is generated from:
> > http://git.wilsonet.com/crystalhd.git/    
> 
> So nice to see Broadcom starting to place nice.

Definitely a nice change of pace vs. Broadcom's stance on wireless. We're dealing with a completely different group within Broadcom here though.

> Is there an eta for the fixing
> of the distribution of the firmware?    

Real Soon Now, I hope, but I'm not certain. The wheels at Broadcom legal didn't turn quite as quickly as we would have liked for the driver and library sources, so we'll see... :)
Comment 7 Jarod Wilson 2010-01-11 10:08:43 EST
Adding our gstreamer expert to the cc list... I spent a bit of time poking at the gst plugin this weekend, and while it does build and install, and totem tries to use it, it fails miserably. It works reasonably well with totem in RHEL5, slightly less so with totem in Fedora 11, and then degrades to not working at all in F12. I'm fairly certain the plugin was mainly targeted at totem as it exists in Ubuntu 8.04, which is what HP is/was shipping on the Linux-based HP Mini 110 w/a Crystal HD card in it. I'm hoping Bastien can look at it, and say "oh, here's the trivial fix you need to make this work again". :)
Comment 8 Bastien Nocera 2010-01-11 11:11:18 EST
You're not telling me which versions of the plugins you're using, or what errors you're seeing on the command-line when this happens.
Comment 9 Jarod Wilson 2010-01-11 12:03:04 EST
D'oh, sorry. I'll collect that info shortly...
Comment 10 Jarod Wilson 2010-01-11 12:27:21 EST
Nggh. New bugzilla... Retyping this entry...

So I believe the relevant components are:
totem-2.28.5-1.fc12 (x86_64)
gstreamer-0.10.25.1-2.fc12 (x86_64)

I swear I was able to roll back just totem at one point in time and get things working, but a quick try with backing off to 2.27.x showed no change, and going back down to 2.26.x looks not entirely trivial at the moment.

Here's the totem output trying to play a clip that plays just fine through the decoder under RHEL5:

Running DIL (0.9.25) Version
Mdata Pool Created...
DtsSetupHardware: DtsPushAuthFwToLink
DtsGetFirmwareFiles:Ctx->FwBinFile is /lib/firmware/bcm70012fw.bin
DtsSetupHardware: Success
OPEN success
0:00:09.208368303  8286      0x100e100 ERROR                  totem bacon-video-widget-gst-0.10.c:366:bvw_error_msg: message =      Internal data flow error.
0:00:09.208397147  8286      0x100e100 ERROR                  totem bacon-video-widget-gst-0.10.c:368:bvw_error_msg: domain  = 2699 (gst-stream-error-quark)
0:00:09.208408741  8286      0x100e100 ERROR                  totem bacon-video-widget-gst-0.10.c:369:bvw_error_msg: code    = 1
0:00:09.208419776  8286      0x100e100 ERROR                  totem bacon-video-widget-gst-0.10.c:370:bvw_error_msg: debug   =      gstbasesrc.c(2388): gst_base_src_loop (): /GstPlayBin2:play/GstURIDecodeBin:uridecodebin0/GstFileSrc:source:
streaming task paused, reason not-negotiated (-4)
0:00:09.208436258  8286      0x100e100 ERROR                  totem bacon-video-widget-gst-0.10.c:371:bvw_error_msg: source  =      <source>
0:00:09.208451554  8286      0x100e100 ERROR                  totem bacon-video-widget-gst-0.10.c:372:bvw_error_msg: uri     =      file:///home/jarod/Videos/espnhd.mpeg4
** Message: Error: Internal data flow error.
gstbasesrc.c(2388): gst_base_src_loop (): /GstPlayBin2:play/GstURIDecodeBin:uridecodebin0/GstFileSrc:source:
streaming task paused, reason not-negotiated (-4)

DtsFWDecFlushChannel: Ioctl failed: 11
0:00:09.305070976  8286      0x100e100 ERROR                 bcmdec gstbcmdec.c:1518:gst_bcmdec_change_state:<bcmdec0> Dec flush    failed 11
DtsCancelFetchOutInt: Called
DtsCancelFetchOutInt: No Pending Req
DtsAllocIoctlData Error
Deleted Mdata Pool...
DtsDeviceClose: Invoked
0:00:09.549956524  8286      0x100e100 ERROR                 bcmdec gstbcmdec.c:3237:bcmdec_del_shmem:<bcmdec0> bcmdec_del_shmem:   deleted shmem segment ...
Comment 11 Jarod Wilson 2010-01-11 12:29:09 EST
And the gstreamer plugins installed:
gstreamer-plugins-base-0.10.25.1-2.fc12
gstreamer-plugins-good-0.10.17-3.fc12
gstreamer-plugins-flumpegdemux-0.10.15-8.fc12
Comment 12 Jarod Wilson 2010-01-21 10:19:52 EST
Nb: I've managed to scrape together a xine-lib-1.2 development tip build, and on top of that, built the xine crystalhd decoder plugin that is under development (can be found at http://sourceforge.net/apps/trac/archvdr/browser/branches/libcrystalhd ). Its not exactly perfect yet, but it does play back a sample clip I've got using the crystalhd.
Comment 13 Bastien Nocera 2010-01-21 10:33:22 EST
First, update to gstreamer-plugins-good-0.10.17-4 (will fix some problems with complicated pipelines).

I would also advise to install the non-free plugins (-bad, -ugly) and try again. The pipeline didn't link properly, most likely because of missing elements for conversion between types.
Comment 14 Jarod Wilson 2010-01-21 10:46:07 EST
No love, same errors as comment #10 with the following on the box:
$ rpm -qa gstreamer-plugins\*
gstreamer-plugins-ugly-0.10.13-1.fc12.x86_64
gstreamer-plugins-good-0.10.17-4.fc12.x86_64
gstreamer-plugins-bad-0.10.17-2.fc12.x86_64
gstreamer-plugins-base-devel-0.10.25.1-2.fc12.x86_64
gstreamer-plugins-flumpegdemux-0.10.15-8.fc12.x86_64
gstreamer-plugins-base-0.10.25.1-2.fc12.x86_64
Comment 15 Bastien Nocera 2010-01-21 11:01:13 EST
Benjamin would have more of an idea about what the problem is.
Comment 16 Jarod Wilson 2010-01-22 10:36:01 EST
Decoder works absolutely flawlessly using a current development trunk build of xbmc, playing back the 1080p h.264 Big Buck Bunny mov file:

http://www.bigbuckbunny.org/index.php/download/

So definitely some available test fodder out there for this thing.
Comment 17 Benjamin Otte 2010-01-25 07:26:51 EST
I had a look at the GStreamer plugin today and didn't find anything obviously wrong. But then, the code looked very beta so I'm not surprised it didn't work.

I don't think I'm the right person to ask about this either (I don't have the hardware for a start so can't test it), but I'd suggest going to #gstreamer on Freenode and asking there.
Comment 18 Jarod Wilson 2010-01-28 12:43:12 EST
Benjamin, would you by chance become the right person if I could provide you with the hardware? If not, I'll pop into #gstreamer and try to find someone...
Comment 19 Alex Lancaster 2010-01-31 01:20:51 EST
Has anybody actually taken this review yet, because the review flag is not set to "?" as per standard procedure, yet it is ASSIGNED.

Is there some outstanding issue that needs to be resolved before the review can actually begin?
Comment 20 Jarod Wilson 2010-01-31 11:58:03 EST
(In reply to comment #19)
> Has anybody actually taken this review yet, because the review flag is not set
> to "?" as per standard procedure, yet it is ASSIGNED.

Peter was working on it... I certainly wouldn't object to additional review help though... :)

> Is there some outstanding issue that needs to be resolved before the review can
> actually begin?    

Nope. The core library works quite well with the driver in the rawhide kernel, been using the stack with xbmc svn trunk and the xine plugin for a while now. The gstreamer lib still needs love, but that can be enabled after the core library (and perhaps it should be maintained separately anyway).

Also, the firmware, I've seen the proposed redistribution license, its very similar to Intel's wifi firmware one, so we should be able to include that in the build fairly soon.
Comment 21 Peter Robinson 2010-02-01 17:08:33 EST
> Peter was working on it... I certainly wouldn't object to additional review
> help though... :)

I will endeavour to get the initial review done today.

> > Is there some outstanding issue that needs to be resolved before the review can
> > actually begin?    
> 
> Nope. The core library works quite well with the driver in the rawhide kernel,
> been using the stack with xbmc svn trunk and the xine plugin for a while now.
> The gstreamer lib still needs love, but that can be enabled after the core
> library (and perhaps it should be maintained ely anyway).
> 
> Also, the firmware, I've seen the proposed redistribution license, its very
> similar to Intel's wifi firmware one, so we should be able to include that in
> the build fairly soon.    

Both excellent news.
Comment 22 Alex Lancaster 2010-02-01 17:26:21 EST
(In reply to comment #21)
> > Peter was working on it... I certainly wouldn't object to additional review
> > help though... :)
> 
> I will endeavour to get the initial review done today.

Don't forget to set the review flag even before you start the actual review... ;)   That way it gets take out of the list of open reviews.
Comment 23 Chris Shoemaker 2010-02-23 20:35:25 EST
I'm interested in this library and was wondering if anyone could give a status update on this review.
Comment 24 Peter Robinson 2010-02-23 20:56:25 EST
Sorry for the delay, I've been travelling, I'll get the review done by Friday.
Comment 25 Chris Shoemaker 2010-03-06 12:35:25 EST
(In reply to comment #24)
> Sorry for the delay, I've been travelling, I'll get the review done by Friday.    


Hi Peter, how's this review coming?

Jarod, I see you've merged some improvements.  Has there been any progress on the firmware license?
Comment 26 Peter Robinson 2010-03-07 07:29:30 EST
Sorry for the delay. There's a few minor bits that need to be addressed. The only thing that looks like a major issue on the rpmlint output is the no-buildroot-tag.

- rpmlint output

$ rpmlint libcrystalhd.spec ../SRPMS/libcrystalhd-0.9.25-1.fc12.src.rpm ../RPMS/x86_64/libcrystalhd-*
libcrystalhd.spec: W: no-buildroot-tag
libcrystalhd.spec: W: invalid-url Source0: libcrystalhd-0.9.25.tar.bz2
libcrystalhd.src: W: spelling-error Summary(en_US) Broadcom -> Broad com, Broad-com, Broadcloth
libcrystalhd.src: W: spelling-error %description -l en_US userspace -> user space, user-space, users pace
libcrystalhd.src: W: spelling-error %description -l en_US Broadcom -> Broad com, Broad-com, Broadcloth
libcrystalhd.src: W: spelling-error %description -l en_US codecs -> codes, coders, code's
libcrystalhd.src: W: no-buildroot-tag
libcrystalhd.src: W: invalid-url Source0: libcrystalhd-0.9.25.tar.bz2
libcrystalhd.x86_64: W: spelling-error Summary(en_US) Broadcom -> Broad com, Broad-com, Broadcloth
libcrystalhd.x86_64: W: spelling-error %description -l en_US userspace -> user space, user-space, users pace
libcrystalhd.x86_64: W: spelling-error %description -l en_US Broadcom -> Broad com, Broad-com, Broadcloth
libcrystalhd.x86_64: W: spelling-error %description -l en_US codecs -> codes, coders, code's
libcrystalhd.x86_64: W: no-documentation
libcrystalhd-devel.x86_64: W: spelling-error Summary(en_US) libs -> lobs, lib, lbs
libcrystalhd-devel.x86_64: W: no-documentation
4 packages and 1 specfiles checked; 0 errors, 15 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

- %doc includes license file
+ spec file written in American English
+ spec file is legible
+ upstream sources match sources in the srpm
  fe11b1960f3ceda55ca6c2dd2d61ccff  crystalhd_linux_20091229.zip
+ package successfully builds on at least one architecture
  tested using koji scratch build
  http://koji.fedoraproject.org/koji/taskinfo?taskID=2036589
+ 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
+ package owns all directories it creates
+ no duplicate files in %files
+ Package perserves timestamps on install
+ %defattr line
+ %clean contains rm -rf $RPM_BUILD_ROOT
+ consistent use of macros
+ package must contain code or permissible content
n/a large documentation files should go in -doc subpackage
n/a files marked %doc should not affect package
+ header files should be in -devel
n/a static libraries should be in -static
n/a 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
+ %install must start with rm -rf %{buildroot} etc.
+ filenames must be valid UTF-8

Optional:

- if there is no license file, packager should query upstream
  No license file but source files contain headers
n/a translations of description and summary for non-English languages, if
available
+ reviewer should build the package in mock/koji
n/a 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 pkgconfig files should go in -devel
+ shouldn't have file dependencies outside /etc /bin /sbin /usr/bin or
/usr/sbin
Comment 27 Jarod Wilson 2010-03-08 10:24:52 EST
BuildRoot tag isn't required any longer:

https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag

No progress to report on the license front just yet, will ping Broadcom again...
Comment 28 Peter Robinson 2010-03-08 16:13:28 EST
(In reply to comment #27)
> BuildRoot tag isn't required any longer:

Ah, cool. Someone should update rpmlint! And re-looking at the review guidelines you only need to include a licence file if its included in the source release so that is OK too.

> No progress to report on the license front just yet, will ping Broadcom
> again...    

I don't think that blocks the rest of it as I think you remove the firmware from the source tar file.
Comment 29 Jarod Wilson 2010-03-10 16:01:21 EST
So... Does this mean we're good to go with approving this package, and I'll just add the firmware sub-package after the license for it gets sorted out?
Comment 30 Peter Robinson 2010-03-10 16:34:07 EST
Yep, I think so.

APPROVED!
Comment 31 Jarod Wilson 2010-03-10 17:33:19 EST
Excellent, thanks much!

New Package CVS Request
=======================
Package Name: libcrystalhd
Short Description: Broadcom Crystal HD device interface library
Owners: jwilson
Branches: F-12 F-13
InitialCC:
Comment 32 Jason Tibbitts 2010-03-10 17:41:42 EST
CVS done (by process-cvs-requests.py).
Comment 33 Fedora Update System 2010-03-11 11:31:20 EST
libcrystalhd-0.9.25-2.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/libcrystalhd-0.9.25-2.fc12
Comment 34 Fedora Update System 2010-03-11 11:32:09 EST
libcrystalhd-0.9.25-2.fc13 has been submitted as an update for Fedora 13.
http://admin.fedoraproject.org/updates/libcrystalhd-0.9.25-2.fc13
Comment 35 Fedora Update System 2010-03-12 21:22:48 EST
libcrystalhd-0.9.25-2.fc12 has been pushed to the Fedora 12 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 36 Fedora Update System 2010-03-12 21:34:42 EST
libcrystalhd-0.9.25-2.fc13 has been pushed to the Fedora 13 stable repository.  If problems still persist, please make note of it in this bug report.