Any suggestions?

D

David Warren

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
 
J

Jon Ericson

David Warren said:
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
 
D

David Warren

Jon said:
Maybe a more descriptive subject? :)

Yes that could be helpful...
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.
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.
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?)




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.




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.




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




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.
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
 
D

David Warren

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'
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 }





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
 
J

Jon Ericson

David Warren said:
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.

Ah. This is useful information. So the number before the slash is
always an integer and the number after the slash is the total number
of frames. The following should match that line and extract the two
numbers:

if (m{^Frame# +(\d+)/(\d+) +\d+ +kbps}){
my $frame = $1;
my $max_frames = $2;
...

With strictly consistent input like this, I like to match as much of
the line as possible. I think it makes it more clear what it is that
you expect.
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.

That would be good. The main thing missing for me is all of the
background information that you had. One of the benefits of a
self-contained example is that it reduces the number of red herrings.
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..

That spot was a mess. :)

Jon
 
D

David Warren

Jon said:
Ah. This is useful information. So the number before the slash is
always an integer and the number after the slash is the total number
of frames. The following should match that line and extract the two
numbers:

if (m{^Frame# +(\d+)/(\d+) +\d+ +kbps}){ my $frame = $1; my
$max_frames = $2; ...

With strictly consistent input like this, I like to match as much of
the line as possible. I think it makes it more clear what it is
that you expect.

Which makes perfect sense to me. Your suggestion above works perfectly.
I also added a ( $frame % 50 == 0 ) to the IF expression above. Now it
only updates the progress bar if the number is divisble by 50. Reduces
CPU usage and makes absolutely no difference to the progress bar updating.

Thanks again for your help.

David
 

Ask a Question

Want to reply to this thread or ask your own question?

You'll need to choose a username for the site, which only take a couple of moments. After that, you can post your question and our members will help you out.

Ask a Question

Members online

Forum statistics

Threads
473,744
Messages
2,569,482
Members
44,901
Latest member
Noble71S45

Latest Threads

Top