Any suggestions?

David Warren

unread,

Oct 19, 2004, 2:47:44 PM10/19/04

to

Hi,

Okay, I was wondering if anybody has any better ideas on how to
implement this....
This is for updating a progress bar. (As far as I've tried this is
working) I'm taking the output from:

"
ID3v2 found. Be aware that the ID3 tag is currently lost when transcoding.
input: 13 - Mrs. Robinson.mp3 (44.1 kHz, 2 channels, MPEG-1 Layer III)
output: 13 - Mrs. Robinson.mp3.wav (16 bit, Microsoft WAVE)
skipping initial 1105 samples (encoder+decoder delay)
Frame# 8575/8575 192 kbps L R
"

The only line I really care about is the last line with Frame#.
By the way, this is my first program I'm writing with perl, only have a
little background in shell scripting. Just don't know if this is the
most efficient way of doing this.

<code>

my ( @nums , $num );
local $/ = "\r";
open PIPE, "lame $args \"$song_source\" \"$song_destin\" 2>&1 |";
my $y = $e->gauge_start( text => "Converting: $song_source", percentage
=> 1 );
my $line_count = 1;

then I'm using a while loop:

while ( <PIPE> ) {
if ( /Frame/ && /kbps/ && /$line_count/ ) {
$line_count++;
unless ( defined($num) ) {
@nums = m/(\/\d+\.?\d*|\.\d+)/g;
$num = "@nums";
$num =~ s/\///;
}
else {
$y = $e->gauge_set( $line_count/$num * 100 );
chomp($y);
}
</code - snip >
(this should all be that is relevant (yes the code could probably be
cleaned up ... ;-)

I'm not too certain about the @nums = m/(\/\d+\.?\d*|\.\d+)/g .
That is extracting the latter half of the numbers in: 8575/8575
Any suggestions? I'm just doing this too familiarize myself with perl.

Thanks a lot for the help.
David

Jim Gibson

unread,

Oct 19, 2004, 3:56:42 PM10/19/04

to

In article <weCdnYecxoK...@golden.net>, David Warren
<davidw_spam@spam_gto.net> wrote:

> Frame# 8575/8575 192 kbps L R
> "
>
> The only line I really care about is the last line with Frame#.
> By the way, this is my first program I'm writing with perl, only have a
> little background in shell scripting. Just don't know if this is the
> most efficient way of doing this.
>
> <code>
>

I am assuming you have 'use strict' and 'use warnings' at the beginning
of your program.

> my ( @nums , $num );
> local $/ = "\r";
> open PIPE, "lame $args \"$song_source\" \"$song_destin\" 2>&1 |";
> my $y = $e->gauge_start( text => "Converting: $song_source", percentage
> => 1 );
> my $line_count = 1;
>
> then I'm using a while loop:
>
> while ( <PIPE> ) {
> if ( /Frame/ && /kbps/ && /$line_count/ ) {

Here you are trying to match the input line against the current value
of $line_count. Why are you doing that? The line you show above doesn't
have a line count value in it. This test will succeed randomly based on
whether or not the actual line has the current value of $line_count in
it as a substring somewhere.

You can test and extract the number you want from the input line in one
operation:

if( |Frame#\s*\d+/(\d+)| ) {
my $num = $1;
# do something with $num
}

> $line_count++;
> unless ( defined($num) ) {

Why are you testing to see if $num is defined? This will result in only
performing the extraction one time, since you are assigning a value to
$num below.

> @nums = m/(\/\d+\.?\d*|\.\d+)/g;

Why are you testing your numbers for decimal points? Your example
doesn't have them. If they could have decimal points, use a character
class for matching numbers: [\d.]+

> $num = "@nums";

This assigns a string to $num consisting of the elements of @nums
separated by spaces. That is probably not what you want to do, More
likely you want some member of @nums: $num = $nums[0] or $num =
$nums[1];

> $num =~ s/\///;
> }
> else {
> $y = $e->gauge_set( $line_count/$num * 100 );
> chomp($y);
> }
> </code - snip >
> (this should all be that is relevant (yes the code could probably be
> cleaned up ... ;-)

Then you should clean it up before asking hundreds of people for help.
If you need more help, please post a complete, working, minimal program
that shows the problem you are having.

Thanks.

Jon Ericson

unread,

Oct 19, 2004, 4:42:22 PM10/19/04

to

David Warren <davidw_spam@spam_gto.net> writes:

> Subject: Any suggestions?

Maybe a more descriptive subject? :-)

> my ( @nums , $num );
> local $/ = "\r";

Do you really need this? Normally the default setting of $/ is just
fine.

> open PIPE, "lame $args \"$song_source\" \"$song_destin\" 2>&1 |";

You could make this a tad easier to read with:

open PIPE, qq{lame $args "$song_source" "$song_destin" 2>&1 |};

I don't have lame (some sort of MP3 decoder?) installed on my system,
so I can't guess what sort of output it might have beyond the example
you gave.

> my $y = $e->gauge_start( text => "Converting: $song_source",
> percentage => 1 );
> my $line_count = 1;

See perlvar for the $. variable. (Oops. I read down and you are only
counting the number of lines that include /Frame/ and /kbps/ and the
current value of $line_count. $. doesn't help in this case. Is
$frame_count a better name?)

> then I'm using a while loop:
>
> while ( <PIPE> ) {
> if ( /Frame/ && /kbps/ && /$line_count/ ) {

Do you really want to look for the current value of $line_count here?
It seems odd if you do. It might be more clear if you combine this
into one regex. Maybe like:

if ( /^Frame.*kbps/ ) {

But I don't know where the $line_count should be.

> $line_count++;
> unless ( defined($num) ) {
> @nums = m/(\/\d+\.?\d*|\.\d+)/g;

This is the bit of code you had a question about. It finds all of the
strings that match a '/' followed by a number (with optional decimal
portion) or just the decimal portion of a number. It puts those
strings into the @num array. In the example input you gave there's
just one element.

> $num = "@nums";

This converts the array into a string with spaces separating the array
elements. In the example, $num is "/8575".

> $num =~ s/\///;

And this removes the '/' from $num.

I don't know the variety of input you expect, but it looks like you
expect to get just one number out of this. If so, the above block
might be better written something like this:

if (m{/(\d+\.?\d*|\.\d+)}){
$num = $1;
}

This isn't exactly equivalent, obviously. You'd have to test with a
bigger set of input. While you are at it, it's possible you could
combine this with the regex I suggested above:

if ( m{^Frame.*/(\d+\.?\d*|\.\d+).*kbps} ) {

> }
> else {
> $y = $e->gauge_set( $line_count/$num * 100 );
> chomp($y);
> }

> (this should all be that is relevant (yes the code could probably be
> cleaned up ... ;-)

Not exactly. I don't know what the gauge_start method is or what
class it belongs to, for instance. Creating a small, self-contained
example is often useful for having other people comment. It also
helps you understand what's going on. Also including a variety of
input and what output you expect, can help.

(Did I mention that it's hard to test my suggestions because I don't
know what sort of input is possible? ;-)

Jon

Tad McClellan

unread,

Oct 19, 2004, 5:54:01 PM10/19/04

to

David Warren <davidw_spam@spam_gto.net> wrote:

> Subject: Any suggestions?

Yes, I suggest putting the subject of your article in the
Subject of your article. That is what it is there for!

--
Tad McClellan SGML consulting
ta...@augustmail.com Perl programming
Fort Worth, Texas

David Warren

unread,

Oct 20, 2004, 2:55:16 AM10/20/04

to

Jon Ericson wrote:
> David Warren <davidw_spam@spam_gto.net> writes:
>
>
>> Subject: Any suggestions?
>
>
> Maybe a more descriptive subject? :-)
>

Yes that could be helpful...

>
>> my ( @nums , $num ); local $/ = "\r";
>
>
> Do you really need this? Normally the default setting of $/ is just
> fine.

Lame doesn't print new lines to stdout, it only updates the last line.
It only updates the portion of following line:

Frame# ----> 8575<----/8575 192 kbps L R

I've tried without adjusting $/ variable and the while loop doesn't
receive any input other than 5 lines.

>
>> open PIPE, "lame $args \"$song_source\" \"$song_destin\" 2>&1 |";
>
>
> You could make this a tad easier to read with:
>
> open PIPE, qq{lame $args "$song_source" "$song_destin" 2>&1 |};
>
> I don't have lame (some sort of MP3 decoder?) installed on my system,
> so I can't guess what sort of output it might have beyond the
> example you gave.
>

Yeah it's an MP3 decoder. It shouldn't have much variation than what I
showed you when decoding.

>
>> my $y = $e->gauge_start( text => "Converting: $song_source",
>> percentage => 1 ); my $line_count = 1;
>
>
> See perlvar for the $. variable. (Oops. I read down and you are
> only counting the number of lines that include /Frame/ and /kbps/ and
> the current value of $line_count. $. doesn't help in this case. Is
> $frame_count a better name?)
>
>
>> then I'm using a while loop:
>>
>> while ( <PIPE> ) { if ( /Frame/ && /kbps/ && /$line_count/ ) {
>
>
> Do you really want to look for the current value of $line_count here?
> It seems odd if you do. It might be more clear if you combine this
> into one regex. Maybe like:

Perhaps not, I'm was just trying to filter out all possibily of wierd
filenames. Lines 2 & 3 from LAME output filename's and the odds were against
Frame/kbps and the current value of $line_count being in the same line.

>
> if ( /^Frame.*kbps/ ) {
>
> But I don't know where the $line_count should be.
>
>
>> $line_count++; unless ( defined($num) ) { @nums =
>> m/(\/\d+\.?\d*|\.\d+)/g;
>
>
> This is the bit of code you had a question about. It finds all of
> the strings that match a '/' followed by a number (with optional
> decimal portion) or just the decimal portion of a number. It puts
> those strings into the @num array. In the example input you gave
> there's just one element.
>
>
>> $num = "@nums";
>
>
> This converts the array into a string with spaces separating the
> array elements. In the example, $num is "/8575".
>
>
>> $num =~ s/\///;
>
>
> And this removes the '/' from $num.
>
> I don't know the variety of input you expect, but it looks like you
> expect to get just one number out of this. If so, the above block
> might be better written something like this:
>
> if (m{/(\d+\.?\d*|\.\d+)}){ $num = $1; }
>

Right now I'm currently using this example and it is working. Thanks.

>
>> } else { $y = $e->gauge_set( $line_count/$num * 100 ); chomp($y); }
>>
>>
>>

>>
>> (this should all be that is relevant (yes the code could probably
>> be cleaned up ... ;-)
>
>
> Not exactly. I don't know what the gauge_start method is or what
> class it belongs to, for instance. Creating a small, self-contained
> example is often useful for having other people comment. It also
> helps you understand what's going on. Also including a variety of
> input and what output you expect, can help.
>

Gauge_start is from the module ui::Dialog::Backend::Zenity. Right now
I'm using the GNOME dialog tool (zenity) to do all the graphical work.
I would
like to make it use GTK, however I thought I'd try something slightly
more simple to begin with. ;-)

The excerpts from the program are working as I suggested, however I
wanted to clean it up, learn better syntax... ;-) Next time, I'll try
including small working example.

> (Did I mention that it's hard to test my suggestions because I don't
> know what sort of input is possible? ;-)
>
> Jon

The user input's is via a graphical menu, can only select between songs &
cancel. I've got some error handling implemented prior to it actually
decoding. So it's only going to have to handle the input I showed you
above.

Thanks a lot for the help, I'm just trying to make it a tad bit more
readable and tidy up the code and this was the spot that was bugging me.
Still learning howto use regex..

David

David Warren

unread,

Oct 20, 2004, 2:58:25 AM10/20/04

to

Jim Gibson wrote:
> In article <weCdnYecxoK...@golden.net>, David Warren
> <davidw_spam@spam_gto.net> wrote:
>
>
>> Frame# 8575/8575 192 kbps L R "
>>

> I am assuming you have 'use strict' and 'use warnings' at the
> beginning of your program.

Yes I'm using 'use strict' and 'use warnings'

>
>> my ( @nums , $num ); local $/ = "\r"; open PIPE, "lame $args
>> \"$song_source\" \"$song_destin\" 2>&1 |"; my $y =
>> $e->gauge_start( text => "Converting: $song_source", percentage =>
>> 1 ); my $line_count = 1;
>>
>> then I'm using a while loop:
>>
>> while ( <PIPE> ) { if ( /Frame/ && /kbps/ && /$line_count/ ) {
>
>
> Here you are trying to match the input line against the current value
> of $line_count. Why are you doing that? The line you show above
> doesn't have a line count value in it. This test will succeed
> randomly based on whether or not the actual line has the current
> value of $line_count in it as a substring somewhere.
>

Perhaps $line_count would be better defined as $frame_count. There is
no real reason other than trying to ensure the output is filtered.
On the 2 and 3rd lines of output from LAME it outputs a
filename and I was just trying to ensure it would be filtered out. (given
that there is some wierd filenames out there). Perhaps, not neccessary or
maybe there would be a better way, to make sure I just got the last line
of output ( which is updated/buffered ) from LAME.

The portion that updates is:

Frame# --->8575<---/8575 192 kbps L R "
That could range anywhere from 1105 -> 8575 (lame skips the first 1105
frames). In this examples, depends on size of mp3.

> You can test and extract the number you want from the input line in
> one operation:
>
> if( |Frame#\s*\d+/(\d+)| ) { my $num = $1; # do something with $num }
>
>
>

>> $line_count++; unless ( defined($num) ) {
>
>
> Why are you testing to see if $num is defined? This will result in
> only performing the extraction one time, since you are assigning a
> value to $num below.
>

Here I'm after only:

Frame# 8575/-->8575<-- 192 kbps L R

This never updates, and I don't need to try to match against it anymore
once I defined $num. Thought it would be unneccessary and pointless to
try and continue to try and match against it. Is there any differences in
efficency between checking if a variables is defined or using pattern
matching?

> Then you should clean it up before asking hundreds of people for
> help. If you need more help, please post a complete, working, minimal
> program that shows the problem you are having.
>
> Thanks.

I should rephrase, I am trying to clean it up. Partially that is what I
was looking for here. Next time I will include a small working
example and being a little clearer. Trying to learn how to use regex
(yes I've read docs) (NOTE - my version was working, I was looking for a
cleaner/more efficent way of doing the while loop.)

Thanks for the help.

David

Michele Dondi

unread,

Oct 20, 2004, 3:18:58 AM10/20/04

to

On Tue, 19 Oct 2004 14:47:44 -0400, David Warren
<davidw_spam@spam_gto.net> wrote: