Patch: Shorten support

[Caleb Epstein]

Here is a diff against the 2003-12-09 nightly build which adds
full support for the Shorten lossless audio CODEC
<URL: http://www.softsound.com/>.

Here is a summary of the changes:

* In order to make this work properly I had to make a slight
change to CPAN/Audio/Wav.pm to support reading from pipelines.
This involved just changing 'new FileHandle "<$file"' to 'new
FileHandle $file'. I must be missing the reason for the < in
the original form. Security mayhap?

* I found a crash bug in the Slim/Formats/FLAC.pm module;
in some cases of bad data, Audio::FLAC::readFlacTag will
return -1, and slimserver was dying when trying to use "-1" as
a hash ref. This protects against that. Should an error be
logged?

* Added Slim/Formats/Shorten.pm. Code based on Wav.pm but
does not use MP3::Info to read any embedded ID3 tags from the
WAV file. This would involve reading the entire WAV stream,
which would be quite slow. I noticed that the
Slim/Formats/Wav.pm is making use of the debug flag $::d_wav,
which does not seem to be settable on the command line and is
not mentioned anyplace else in the code. I've use $::d_source
in the Shorten module instead and I think Wav.pm should be
changed similarly. Either that or --d_wav should be honored.

* Slim/Music/Info.pm: added use of Slim::Formats::Shorten and an
elsif case to handle Shorten files.

* convert.conf: fixed incorrect command for extracting SHN
files. Added SHN -> WAV target for squeezebox.

* slimserver.pl - put @INC at the *end* of the "use lib"
statement. From what I can tell of reading the "perlrun"
manual page, having @INC in that list at all is redundant.
I moved it to the end to ensure that the SlimServer's CPAN
directory is searched before any locally-installed modules of
the same name (I happen to have Audio/Wav installed in my
/usr/local/lib/site_perl directory as well). This is to
ensure that the modified Audio::Wav which can handle pipelines
is used.

All tested to be working with the /stream.mp3 URL on Linux.


diff -x '*~' -ur --new-file SlimServer_v2003-12-09.orig/CPAN/Audio/Wav/Read.pm SlimServer_v2003-12-09/CPAN/Audio/Wav/Read.pm
--- SlimServer_v2003-12-09.orig/CPAN/Audio/Wav/Read.pm 2003-12-09 08:01:29.000000000 -0500
+++ SlimServer_v2003-12-09/CPAN/Audio/Wav/Read.pm 2003-12-09 22:14:12.000000000 -0500
@@ -41,7 +41,7 @@
my $tools = shift;
$file =~ s#//#/#g;
my $size = -s $file;
- my $handle = new FileHandle "<$file";
+ my $handle = new FileHandle $file;

my $self = {
'real_size' => $size,
diff -x '*~' -ur --new-file SlimServer_v2003-12-09.orig/Slim/Formats/FLAC.pm SlimServer_v2003-12-09/Slim/Formats/FLAC.pm
--- SlimServer_v2003-12-09.orig/Slim/Formats/FLAC.pm 2003-12-09 08:01:29.000000000 -0500
+++ SlimServer_v2003-12-09/Slim/Formats/FLAC.pm 2003-12-10 08:33:37.000000000 -0500
@@ -34,12 +34,15 @@
my $tags = Audio::FLAC::readFlacTag($file);

# lazy? no. efficient. =)
- while (my ($old,$new) = each %tagMapping) {
-
- if (exists $tags->{$old}) {
- $tags->{$new} = $tags->{$old};
- delete $tags->{$old};
- }
+ if (ref $tags eq "HASH") {
+ while (my ($old,$new) = each %tagMapping) {
+ if (exists $tags->{$old}) {
+ $tags->{$new} = $tags->{$old};
+ delete $tags->{$old};
+ }
+ }
+ } else {
+ $tags = {};
}

$tags->{'SIZE'} = -s $file;
diff -x '*~' -ur --new-file SlimServer_v2003-12-09.orig/Slim/Formats/Shorten.pm SlimServer_v2003-12-09/Slim/Formats/Shorten.pm
--- SlimServer_v2003-12-09.orig/Slim/Formats/Shorten.pm 1969-12-31 19:00:00.000000000 -0500
+++ SlimServer_v2003-12-09/Slim/Formats/Shorten.pm 2003-12-10 08:26:11.000000000 -0500
@@ -0,0 +1,85 @@
+package Slim::Formats::Shorten;
+
+# $Id$
+
+################################################# ##############################
+# FILE: Slim::Formats::Shorten.pm
+#
+# DESCRIPTION:
+# Extract tag information from a Shorten file and store in a hash
+# for easy retrieval. Requires the external command line "shorten"
+# decoder and uses the Audio::Wav module.
+#
+# NOTES:
+# This code has only been tested on Linux.
+################################################# ##############################
+use strict;
+
+use Audio::Wav;
+use MP3::Info; # because WAV files sometimes have ID3 tags in them!
+use Slim::Utils::Misc; # this will give a sub redefined error because we ues Slim::Music::Info;
+
+my $bail; # nasty global to know when we had a fatal error on a file.
+
+# Given a file, return a hash of name value pairs, where each name is
+# a tag name.
+sub getTag {
+ my $file = shift || "";
+
+ # Extract the file and read from the pipe; redirect stderr to
+ # /dev/null since we will be closing the pipe before the
+ # entire file has been extracted and we don't want to see
+ # shorten's error message "failed to write decompressed
+ # stream". Note that this requires a slightly modified
+ # Audio/Wav.pm
+ $file = "shorten -x \Q$file\E - 2>/dev/null|";
+
+ $::d_source &&
+ Slim::Utils::Misc::msg( "Reading WAV information from $file\n");
+
+ # This hash will map the keys in the tag to their values.
+ # Don't use MP3::Info since we can't seek around the stream
+ # and don't want to open it multiple times
+ my $tags = {};
+
+ # bogus files are considered empty
+ $tags->{'SIZE'} ||= 0;
+ $tags->{'SECS'} ||= 0;
+
+ $bail = undef;
+
+ my $wav = Audio::Wav->new();
+
+ $wav->set_error_handler( \&myErrorHandler );
+
+ my $read = $wav->read($file);
+
+ unless ($bail) {
+ $tags->{'OFFSET'} = $read->position();
+ $tags->{'SIZE'} = $read->length();
+ $tags->{'SECS'} = $read->length_seconds();
+ }
+
+ return $tags;
+}
+
+sub myErrorHandler {
+ my %parameters = @_;
+
+ if ( $parameters{'warning'} or
+
+ # When reading from a pipeline, the seek done in
+ # Audio::Wav::move_to will fail, but we don't really care
+ # about that
+ ($parameters{'filename'} =~ /\|$/ and
+ $parameters{'message'} =~ /^can\'t move to position/)) {
+ # This is a non-critical warning
+ $::d_source && Slim::Utils::Misc::msg( "Warning: $parameters{'filename'}: $parameters{'message'}\n");
+ } else {
+ # Critical error!
+ $bail = 1;
+ $::d_source && Slim::Utils::Misc::msg( "ERROR: $parameters{'filename'}: $parameters{'message'}\n");
+ }
+}
+
+1;
diff -x '*~' -ur --new-file SlimServer_v2003-12-09.orig/Slim/Music/Info.pm SlimServer_v2003-12-09/Slim/Music/Info.pm
--- SlimServer_v2003-12-09.orig/Slim/Music/Info.pm 2003-12-09 08:01:29.000000000 -0500
+++ SlimServer_v2003-12-09/Slim/Music/Info.pm 2003-12-09 22:27:34.000000000 -0500
@@ -22,6 +22,7 @@
use Slim::Formats::Ogg;
use Slim::Formats::Wav;
use Slim::Formats::WMA;
+use Slim::Formats::Shorten;
use Slim::Utils::Misc;
use Slim::Utils::OSDetect;
use Slim::Utils::Strings qw(string);
@@ -1707,7 +1708,10 @@
$tempCacheEntry = Slim::Formats::WMA::getTag($filepath);
} elsif ($type eq "mov") {
$tempCacheEntry = Slim::Formats::Movie::getTag($filepath);
+ } elsif ($type eq "shn") {
+ $tempCacheEntry = Slim::Formats::Shorten::getTag ($filepath);
}
+

$::d_info && !defined($tempCacheEntry) && Slim::Utils::Misc::msg("Info: no tags found for $filepath\n");

diff -x '*~' -ur --new-file SlimServer_v2003-12-09.orig/convert.conf SlimServer_v2003-12-09/convert.conf
--- SlimServer_v2003-12-09.orig/convert.conf 2003-12-09 08:01:29.000000000 -0500
+++ SlimServer_v2003-12-09/convert.conf 2003-12-09 22:41:48.000000000 -0500
@@ -44,7 +44,10 @@
$lame$ --silent -b $BITRATE$ $FILE$ -

shn mp3 * *
- shorten -d $FILE$ | $lame$ --silent -b $BITRATE$ - -
+ shorten -x $FILE$ - | $lame$ --silent -b $BITRATE$ - -
+
+shn wav squeezebox *
+ shorten -x $FILE$ -

flc mp3 * *
$flac$ -dc $FILE$ | $lame$ --silent -b $BITRATE$ - -
diff -x '*~' -ur --new-file SlimServer_v2003-12-09.orig/slimserver.pl SlimServer_v2003-12-09/slimserver.pl
--- SlimServer_v2003-12-09.orig/slimserver.pl 2003-12-09 08:01:29.000000000 -0500
+++ SlimServer_v2003-12-09/slimserver.pl 2003-12-09 22:39:45.000000000 -0500
@@ -87,7 +87,7 @@
use POSIX qw(:signal_h :errno_h :sys_wait_h);
use Socket qw(:DEFAULT :crlf);

-use lib (@INC, $Bin, catdir($Bin,'CPAN'), catdir($Bin,'CPAN','arch',$Config::Config{archname }) );
+use lib ($Bin, catdir($Bin,'CPAN'), catdir($Bin,'CPAN','arch',$Config::Config{archname }), @INC);
use Time::HiRes;

use Slim::Utils::Misc;

--
Caleb Epstein | bklyn . org | The computer can't tell you the emotional
cae at | Brooklyn Dust | story. It can give you the exact mathematical
bklyn dot org | Bunny Mfg. | design, but what's missing is the eyebrows.
| | - Frank Zappa

[Erik Reckase]

Caleb,

Thanks for finding that bug in the Slim/Music/Flac routines. Dan and I have been concentrating on improving/updating the new CPAN module Audio::FLAC, and so I haven't had a chance to go back and look at the slimserver interface to that module - it'll most likely change again once Audio::FLAC is released to CPAN.

Good work on shorten support - now I just have to add APE and MPC support, and we'll really be flying.

Regards,
Erik

[Dan Sully]

* Caleb Epstein <cae (AT) bklyn (DOT) org> shaped the electrons to say...

> Here is a summary of the changes:

Caleb - looks good overall -

> * In order to make this work properly I had to make a slight
> change to CPAN/Audio/Wav.pm to support reading from pipelines.
> This involved just changing 'new FileHandle "<$file"' to 'new
> FileHandle $file'. I must be missing the reason for the < in
> the original form. Security mayhap?

These should be exactly the same. What version of perl are you using?

> * I found a crash bug in the Slim/Formats/FLAC.pm module;
> in some cases of bad data, Audio::FLAC::readFlacTag will
> return -1, and slimserver was dying when trying to use "-1" as
> a hash ref. This protects against that. Should an error be logged?

Thanks! See Erik's message.

> * Added Slim/Formats/Shorten.pm. Code based on Wav.pm but
> does not use MP3::Info to read any embedded ID3 tags from the
> WAV file. This would involve reading the entire WAV stream,
> which would be quite slow. I noticed that the
> Slim/Formats/Wav.pm is making use of the debug flag $::d_wav,
> which does not seem to be settable on the command line and is
> not mentioned anyplace else in the code. I've use $::d_source
> in the Shorten module instead and I think Wav.pm should be
> changed similarly. Either that or --d_wav should be honored.

If --d_wav is used only once, it should be changed to --d_source, or
something common to the format modules.

> * slimserver.pl - put @INC at the *end* of the "use lib"
> statement. From what I can tell of reading the "perlrun"
> manual page, having @INC in that list at all is redundant.
> I moved it to the end to ensure that the SlimServer's CPAN
> directory is searched before any locally-installed modules of
> the same name (I happen to have Audio/Wav installed in my
> /usr/local/lib/site_perl directory as well). This is to
> ensure that the modified Audio::Wav which can handle pipelines
> is used.

I don't think this is needed at all - have you tried it without?

On win32, the shorten binary is called "shortn32", and on others
"shorten". convert.conf doesn't have a platform concept yet.

In convert.conf - "shorten" should be changed to "$shorten$" so that
it searches for the binary properly.

-D
--
For every new fool-proof invention there is a new and improved fool.

[Caleb Epstein]

On Wed, Dec 10, 2003 at 07:29:11AM -0800, Dan Sully wrote:

> > * In order to make this work properly I had to make a slight
> > change to CPAN/Audio/Wav.pm to support reading from pipelines.
> > This involved just changing 'new FileHandle "<$file"' to 'new
> > FileHandle $file'. I must be missing the reason for the < in
> > the original form. Security mayhap?
>
> These should be exactly the same. What version of perl are you using?

Not if $file is a pipeline (e.g. $file = "shorten -x \Q$file\E
-|"). From my perl -v:

This is perl, v5.8.2 built for i386-linux-thread-multi

Try this on for size:

% perl -MFileHandle -e '$fh = new FileHandle "<cat /etc/profile|" or die "$!"'
No such file or directory at -e line 1.

This works if you remove the "<"

> > * slimserver.pl - put @INC at the *end* of the "use lib"
> > statement. From what I can tell of reading the "perlrun"
> > manual page, having @INC in that list at all is redundant.
> > I moved it to the end to ensure that the SlimServer's CPAN
> > directory is searched before any locally-installed modules of
> > the same name (I happen to have Audio/Wav installed in my
> > /usr/local/lib/site_perl directory as well). This is to
> > ensure that the modified Audio::Wav which can handle pipelines
> > is used.
>
> I don't think this is needed at all - have you tried it without?

Without @INC? No, but I agree on principle that it is
redundant and should not be in the "use lib" statement.

> On win32, the shorten binary is called "shortn32", and on others
> "shorten". convert.conf doesn't have a platform concept yet.

The most up-to-date version of shorten (available from
etree.org) is built with cygwin and is called
shorten.exe.

> In convert.conf - "shorten" should be changed to "$shorten$" so
> that it searches for the binary properly.

Fair enough.

--
Caleb Epstein | bklyn . org | He only knew his iron spine held up the sky
cae at | Brooklyn Dust | -- he didn't realize his brain had fallen to
bklyn dot org | Bunny Mfg. | the ground.
| | -- The Book of Serenity

[Dan Sully]

* Caleb Epstein <cae (AT) bklyn (DOT) org> shaped the electrons to say...

> % perl -MFileHandle -e '$fh = new FileHandle "<cat /etc/profile|" or die "$!"'
> No such file or directory at -e line 1.

Ah, I see. Could you submit a patch to the Audio::Wav author to
change it in the upstream code? I don't see why the "<" is needed in Wav.pm

> The most up-to-date version of shorten (available from
> etree.org) is built with cygwin and is called shorten.exe.

You are correct - I had an older version around.

-D
--
This knob controls the thing that changes when you turn it. - noah

[Caleb Epstein]

On Wed, Dec 10, 2003 at 08:14:54AM -0800, Dan Sully wrote:

> * Caleb Epstein <cae (AT) bklyn (DOT) org> shaped the electrons to say...
>
> > % perl -MFileHandle -e '$fh = new FileHandle "<cat /etc/profile|" or die "$!"'
> > No such file or directory at -e line 1.
>

> Ah, I see. Could you submit a patch to the Audio::Wav author
> to change it in the upstream code? I don't see why the "<"
> is needed in Wav.pm

Will do.

--
Caleb Epstein | bklyn . org | When it comes to helping you, some people stop
cae at | Brooklyn Dust | at nothing.
bklyn dot org | Bunny Mfg. |

[John Strickler]

I've been lurking on this group for a couple of weeks, and just though I'd
jump in here, since Perl is my area of expertise.
I think you could use either
% perl -MFileHandle -e '$fh = new FileHandle "cat /etc/profile|" or die
"$!"'
or
% perl -MFileHandle -e '$fh = new FileHandle "</etc/profile" or die "$!"'


< opens a FILE for reading
| opens a PROCESS for reading

I don't believe they can be combined.
HTH

----- Original Message -----
From: "Caleb Epstein" <cae (AT) bklyn (DOT) org>
To: <developers (AT) lists (DOT) slimdevices.com>
Sent: Wednesday, December 10, 2003 11:25 AM
Subject: Re: [Developers] Patch: Shorten support


> On Wed, Dec 10, 2003 at 08:14:54AM -0800, Dan Sully wrote:
>
> > * Caleb Epstein <cae (AT) bklyn (DOT) org> shaped the electrons to say...
> >
> > > % perl -MFileHandle -e '$fh = new FileHandle "<cat /etc/profile|" or
die "$!"'
> > > No such file or directory at -e line 1.
> >
>
> > Ah, I see. Could you submit a patch to the Audio::Wav author
> > to change it in the upstream code? I don't see why the "<"
> > is needed in Wav.pm
>
> Will do.
>
> --
> Caleb Epstein | bklyn . org | When it comes to helping you, some people
stop
> cae at | Brooklyn Dust | at nothing.
> bklyn dot org | Bunny Mfg. |
>

[John Strickler]

And to reply to my own post with my real point....
it's better to use "</etc/profile" than "cat /etc/profile|", because the
first version has to launch a separate process (ie, "cat"), and the second
just opens the file.

----- Original Message -----
From: "John Strickler" <jstrick (AT) mindspring (DOT) com>
To: "SlimDevices Developers" <developers (AT) lists (DOT) slimdevices.com>
Sent: Wednesday, December 10, 2003 11:31 AM
Subject: Re: [Developers] Patch: Shorten support


> I've been lurking on this group for a couple of weeks, and just though I'd
> jump in here, since Perl is my area of expertise.
> I think you could use either
> % perl -MFileHandle -e '$fh = new FileHandle "cat /etc/profile|" or die
> "$!"'
> or
> % perl -MFileHandle -e '$fh = new FileHandle "</etc/profile" or die "$!"'
>
>
> < opens a FILE for reading
> | opens a PROCESS for reading
>
> I don't believe they can be combined.
> HTH
>
> ----- Original Message -----
> From: "Caleb Epstein" <cae (AT) bklyn (DOT) org>
> To: <developers (AT) lists (DOT) slimdevices.com>
> Sent: Wednesday, December 10, 2003 11:25 AM
> Subject: Re: [Developers] Patch: Shorten support
>
>
> > On Wed, Dec 10, 2003 at 08:14:54AM -0800, Dan Sully wrote:
> >
> > > * Caleb Epstein <cae (AT) bklyn (DOT) org> shaped the electrons to say...
> > >
> > > > % perl -MFileHandle -e '$fh = new FileHandle "<cat /etc/profile|" or
> die "$!"'
> > > > No such file or directory at -e line 1.
> > >
> >
> > > Ah, I see. Could you submit a patch to the Audio::Wav author
> > > to change it in the upstream code? I don't see why the "<"
> > > is needed in Wav.pm
> >
> > Will do.
> >
> > --
> > Caleb Epstein | bklyn . org | When it comes to helping you, some
people
> stop
> > cae at | Brooklyn Dust | at nothing.
> > bklyn dot org | Bunny Mfg. |
> >

[Caleb Epstein]

On Wed, Dec 10, 2003 at 11:36:50AM -0500, John Strickler wrote:

> And to reply to my own post with my real point.... it's better to
> use "</etc/profile" than "cat /etc/profile|", because the first
> version has to launch a separate process (ie, "cat"), and the second
> just opens the file.

Yes I know all of this. I was just trying to illustrate the
bug in the Audio::Wav module with a simple Perl one-liner.

--
Caleb Epstein | bklyn . org | Egotist: A person of low taste, more
cae at | Brooklyn Dust | interested in himself than in me. -- Ambrose
bklyn dot org | Bunny Mfg. | Bierce