Patch: ASF picture parser

[Lukáš Lalinský]

Hi Anton,

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.

Lukas

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
> valid.
>
> 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
> https://mail.kde.org/mailman/listinfo/taglib-devel
>
>