Bug 553717
Summary: | Review Request: libcrystalhd - Broadcom Crystal HD device interface library | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Jarod Wilson <jarod> |
Component: | Package Review | Assignee: | Peter Robinson <pbrobinson> |
Status: | CLOSED ERRATA | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | alex, bjohnson, bnocera, c.shoemaker, fedora-package-review, jarodwilson, notting, otte, pbrobinson |
Target Milestone: | --- | Flags: | pbrobinson:
fedora-review+
j: 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-13 02:34:47 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: |
Description
Jarod Wilson
2010-01-08 18:21:09 UTC
I'll review this one 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. 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! (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/ > 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? (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... :) 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". :) 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. D'oh, sorry. I'll collect that info shortly... 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 ... 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 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. 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. 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 Benjamin would have more of an idea about what the problem is. 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. 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. 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... 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? (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. > 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. (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. I'm interested in this library and was wondering if anyone could give a status update on this review. Sorry for the delay, I've been travelling, I'll get the review done by Friday. (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? 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 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... (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. 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? Yep, I think so. APPROVED! 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: CVS done (by process-cvs-requests.py). 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 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 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. 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. |