Bug #32693 for Audio-FLAC-Header: Will not retrieve multiple pictures of the same picture type, proposed solution

Hi again :) In my work with adding support for embedded pictures to flac2mp3 I stumbled over a problem with the extraction of picture data using Audio-FLAC-Header, 'sub picture'. It is perfectly possible to embed several pictures of the same picture type (0..20) into a FLAC file using various tagging software (or using metaflac), and the FLAC format specification allows it. However, Audio:FLAC:Header 1.9 (and earlier versions) will not return more than ONE picture of each picture type. For instance, if there are several embedded pictures of type 5 ('Leaflet page') in the source file, then the command $srcfile->picture(5) will just return the data for the one which was most recently added (having the highest metadata block number). This is because the picture type is used as a hash key, and the hash holding the picture data is thus overwritten each time a new block is parsed which has the same picture type. I would like to suggest a slight modification of 'sub picture' and 'sub _parsePicture' which adds valuable functionality to the existing version. I propose that the string 'all' be added as an alternative to the numeric inputs (0..20) for 'sub picture'. When called with 'all', the subroutine will return not a hash, but an array of hashes. Each item in the array represents a picture and the hash data is organized just like in the existing released version. However, each hash has an additional key=>value pair containing the picture type info. This data structure in a natural way lends itself to retrieval of multiple pictures of the same type. At the same time, the existing functionality for numeric input arguments (0..20) is fully retained. The data structure (array-of-hashes) is basically the same as you get with the call $ID3v2->get_frames( "APIC" ) to the MP3::Tag module. It turned out to be really easy to do the necessary additions to the code. To this bug report I attach a patch file for all proposed changes, including changes to the documentation. A few comments: Besides the obvious modification of 'sub picture', we build an array consisting of the hashes generated by every pass of 'sub_parsePicture'. Line 700: - + $self->{'picture'}->{$pictureType}->{'pictureType'} = $pictureType; may look odd at first, and there may be other ways to do it, but somehow we must include the picture type info into the hash before pushing it into the array. A typical call, along with code for debugging would look like as in the attachment #2. This may save you some time when checking the result. I have used the patched Audio:FLAC:Header in the context of my modification of flac2mp3 and it works like a charm, I must say! I use ActivePerl 5.8.8 Build 822 on a MSWin32-x86 machine. Operating system is Windows XP pro, SP2. It hope you find it interesting and worthwhile to include in the next release. Best regards, Carl Asplund

--- H:/test/flac2mp3-0.3.0rc1/old/Header_v1.9.pm Thu Jan 24 10:23:19 2008 +++ H:/test/flac2mp3-0.3.0rc1/lib/Audio/FLAC/Header_v1.9_bugfixed_extended.pm Sat Jan 26 09:42:37 2008 @@ -164,8 +164,11 @@ sub picture { my $self = shift; - my $type = shift || 3; # front cover - + my $type; + $type = 3 unless defined ($type = shift); # defaults to front cover + if ($type eq 'all') { + return $self->{'allpictures'} if exists($self->{'allpictures'}); + }; # if the picture block exists, return it's content return $self->{'picture'}->{$type} if exists($self->{'picture'}->{$type}); @@ -694,7 +697,11 @@ $self->{'picture'}->{$pictureType}->{'depth'} = $depth; $self->{'picture'}->{$pictureType}->{'colorIndex'} = $colorIndex; $self->{'picture'}->{$pictureType}->{'imageData'} = $imageData; - + $self->{'picture'}->{$pictureType}->{'pictureType'} = $pictureType; + + # Create array of hashes with picture data from all the picture metadata blocks + push ( @{$self->{'allpictures'}}, {%{$self->{'picture'}->{$pictureType}}} ); + return 1; } @@ -960,9 +967,13 @@ =item * picture( [$type ] ) -Returns a hash containing data from a PICTURE block if found. +Returns a hash containing data from the last PICTURE block of this type if found. Defaults to type 3 - "Front Cover" + +When the passed variable is 'all', an array of hashes containing +picture data from all PICTURE blocks is returned. Allows for multiple instances +of the same picture type. =item * vendor_string( )

#!/usr/bin/env perl -w use warnings; use strict; use FindBin; use lib "$FindBin::Bin/lib"; use Data::Dumper; use Audio::FLAC::Header; my $srcfilename = 'h:\test\4.flac'; # create object to access flac tags my $srcfile = Audio::FLAC::Header->new($srcfilename); # get all picture data from flac file my $allsrcpictures = $srcfile->picture('all'); # my $allsrcpictures = $srcfile->picture(4); print "Data from FLAC picture metadata blocks:\n"; print Dumper($allsrcpictures);