Re: New Package Proposal: File_Ogg

Hi Stefan, On Wednesday 27 August 2003 23:49, Stefan Neufeind wrote: > as discussed earlier I like the idea very much. And I very much > appreciate the new, open concept - e.g. also providing a pecl- > interface. But should you really distribute an empty pecl-file and > require_once it even if it hasn't anything in it's body? Maybe think > that over again before doing a release. This has now been removed. This class can be put in as an when it is written. > Maybe the require_once-statements could be done inside the if- > statement to load the appropriate files only when needed? This has been corrected. > In your comments at the beginning of each file there is: > // $Id > > Do you intend to put this file in cvs? This tag is only there for cvs- > purpose. It needs to be $Id$ to be correct. This has now been removed. I just blindly copied from the license page, and this tag got included with the rest of it. :) > Then having a look at Ogg/PEAR.phps: > As discussed earlier also, shouldn't probably the streamnames be > lowercase alltogether? Have a look at flac? If you don't do it > lowercase (which I think might be a bad idea - but looks better when > you want to display it to the user) you should have written Vorbis > insteand of vorbis because that's the way xiph.org writes it afaik. > theora maybe the same? Why does speex have that many whitespaces in > the name? There are two sets of identifiers, there are those used as capture patterns (included speex followed by three spaces), and those for display to the user. The capture patterns must be in the format shown, but there is nothing to prevent the stream type being formatted to make it more human-friendly. > In listStreams() you mention the "stream serial number". In getStream > it's called $streamSerial - but in listStreams that's $stream_id. > Maybe rename for consistency? Good point. This has been changed. > Looking at File_Ogg/Ogg/Vorbis/PEAR.phps: > Is $_avgBitrate really an integer as you state? Or could it return > fractions and therefor be a double? Yes, again you are correct. This has also been changed. :) > But these are only a few minor things that I found. Altogether: Well > done! If you iron out these few things I'm +1 for File_Ogg. I've also added documentation for the accessor methods (e.g. getSerial()), and put and example in the download area. Thanks for your kind words. Cheers, -D. -- David Jonathan Grant E: david@davidjonathangrant.info W: http://www.davidjonathangrant.info/ T: +4479 6844 1706 B: Pinky, are you pondering what I'm pondering? P: I think so Larry, and Brain, but how we will get all seven dwarves to shave their legs.