Patch: ASF picture parser

[Lukáš Lalinský]

On Sun, Sep 5, 2010 at 6:32 AM, Anton Sergunov <setosha at gmail.com> wrote:
>>  - 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.
>
> done. But I'm not shure about code like
> Picture p;
> p->setDesctiption("ddd");
> Picture p2 = p;
> p2->setDescription("d2d2d2");
> p->description();
>
> Is this expected behavior?

The idea is to detach the shared data. You can see an example of this
in ByteVector::detach().

I'm still not sure about the explicit tracking of valid flag. If you
create an empty picture (no data, no mime type), it's clearly not
valid, but it has the valid flag set. I think I'd prefer to determine
whether the picture is valid by checking whether the size of the
picture data or something like that. But I don't have a strong
preference either way.

Btw, thanks to the data sharing in ByteVector/String/etc., you can
change methods like:

const String& description() const;

to:

String description() const;

Making a copy of such objects is very cheap and no other code in
TagLib returns const references.

>>  - 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".
>
> Now Picture parses from byte vector.
> It still clears data vector in parser if picture loaded. I think
> picture can have realy big size and I don't want to store it twice.
>
> If let render() to be public Picture can be removed from Attribute.
>
> by http://msdn.microsoft.com/en-us/library/dd743745(v=VS.85).aspx
> can be five types of binary attribute by atribute name:
> WM_PICTURE "WM/Picture"
> WM_STREAM_TYPE_INFO "WM/StreamTypeInfo"
> WM_SYNCHRONISED_LYRICS "WM/Lyrics_Synchronised"
> WM_USER_TEXT "WM/Text"
> WM_USER_WEB_URL "WM/UserWebURL"

I think it's fine for now. In TagLib 2, we might want to consider
making ASF::Attribute a private class and expose API similar to
MPEG::ID3v2::Frame.

> I still don't have idea how to refuse to set picture to anything but
> WM\Picture.
> ASF::Attribute don't know own name
> ASF::Tag::setAttribute and ASF::Tag::addAttribute don't have return code.

Changed my mind, I think it's ok. :) We already give the users the
possibility to set e.g "WM/AlbumArtist" to random binary data, not
string, so having the same for ASF::Picture should be fine.

> In general, what do you think about adding minimal picture
> functionality to TagLib::Tag? All modern programs shows cover art in
> interface.

Adding something to TagLib::Tag is tricky, because it can't be done in
version 1.x, unless you add a non-virtual method and a bunch of
dynamic_casts. Personally, I also really don't like the Tag interface,
so I'd just prefer to replace it with something different in version
2.0. There are two main reasons:

 1. Especially in setters, it has deals with each field on its own.
This is a problem if it has to share an ID3 frame with some other
field.
 2. It doesn't expose enough fields to be useful for me.

For my own purposes, I'll soon need a generic tag interface that
exposes more fields than what TagLib::Tag provides. I was planning to
write a new interface (initially not integrated in TagLib, but can be
added if people like it) that basically imports and exports data from
the native TagLib file classes. The main data would be stored as a
String -> List<String> map. The exporters would try to put everything
they can here (conductors, performers on various instruments, etc.).
This would make it possible to support more than one tag mapping (e.g.
in a tagger application, Foobar2000 users want the albumartist field
to be saved differently than iTunes users). There could be a simple
TagLib::Tag-like interface over the exported data. And as a part of
this, there would be a generic cover art list.

Lukas