Patch: ASF picture parser
lalinsky at gmail.com
Sat Sep 4 22:51:43 CEST 2010
I agree that this functionality should be added to TagLib, but I have
a few comments regarding the patch:
- ASF::Picture should be in a separate file. It should use no private
member variables or inline methods to make it possible to change it
without breaking binary compatibility. It would be nice if the private
d pointer was refcounted, like in MP4::CoverArt.
- I'm not sure about the integration with ASF::Attribute. While it's
useful, it doesn't follow the physical structure of ASF metadata
attributes. I expect there are more binary attributes than WM/Picture
and I'm not sure if they all deserve to have special parsers there.
But if it stays there, I think it should refuse to use the Picture
class for anything else than "WM/Picture".
- readNullTerminatedString is probably too specific to be in
ASF::File, it's not used anywhere but the WM/Picture parsing code. You
can also use ByteVector::find for the main loop in the function, see
MPEG::ID3v2::Frame::readStringField for example.
On Fri, Aug 27, 2010 at 5:13 PM, Anton Sergunov <setosha at gmail.com> wrote:
> Adds ASF::Picture class.
> Fix: ASF long (size >= 0x8000) Attribute in extended content descriptor.
> Technical details.
> class Picture
> To reach best performance all accessors is inline and Attribute
> returned a picture by const refference.
> Don't know is it correct on other build enviroments?
> When Attribute is binary and named "WM/Picture" attribute::parse() try
> to parse data as Picture.
> If successed data stored as picture only.
> When attribute requested as binaryVector it renders from stored Picture.
> If Picture::parse() fails parser calls debug("..."), performs
> File::seek() to begining of data, stores it as usual (ByteArray) and
> sets picture.isValid() to false;
> So Attribute::toPicture().isValid() should be true when name ==
> "WM/Picture". attribute.type() should be BytesType;
> User can Attribute::toPicture().isValid() on any attribute without
> risk of failure.
> But if user reads picture and then sets attribute as binary data then
> attribute.asPicture().isValid() will return false even if it was
> correct picture buffer. After write - read operations picture will be
> Attribute a(Picture);
> assert(a.toPicture().isValid() == true);
> Attribute b(a.toPicture());
> assert(b.toPicture().isValid() == true);
> Attribute c(a.toBinaryByteVector());
> assert(c.toPicture().isValid() == false);
> About fix.
> File stores data as WORD (unsigned int32)
> WORD fileStoredData;
> int oldSize = (short)(unsigned short)fileStoredData; // gives negative
> values when fileStoredData >= 0x8000
> uint newSize = (unsigned short)fileStoredData; // correct
> Tested on WinVista/Qt 4.6.3(mingw) dll
> Before apply path to HEAD revision please check my spelling in doxygen comments.
> taglib-devel mailing list
> taglib-devel at kde.org
More information about the taglib-devel