Patch: ASF picture parser

[Anton Sergunov]

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

If it's normal behaviour in your interface - all fine.
But it's innatural in c++ practice.
You mixed two different patterns:
1. ShearedPointer - exactly what you have.
2. Lazy copy - every modificator (non const function) should create
copy of internal private data if ref count != 1. /See QString

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

Valid means that Picture data was readed correctly from file. Binary
data satisfies Picture format. It still can have empty description,
mime and pictureData, but it was correctly parsed from file or
ByteVector.
Thats why user can't create invalid picture.

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

done. Just missed.

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

Just keep in mind. I select tagLib because it's interface realy close
to native tag format. It means I can set Picture or LirycsSynced or
every native tag field even if tagLib does not support it! So I can
give to advanced user way to select how exactly it want to store tag.

For example:
OggVorbis have 3 undocumented but popular ways to store album art. I
want to be able to use any of thus ways.

Picture frame has binary type in asfFile and in WinSdk so let it be binary.

But Sdk says that ASF tag is compatable with ID3v2 so may be to create
interface to access ASF tag that ierahits MPEG::ID3v2::Tag and get it
from ASF::File...

Please try to be close to native tag format in 2.0

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

ok :)

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

I use Tag interface just to read common tag from any supported file.
And show specified tags in "advanced" tab. May be you should to add
some fields like albumArtist. In general I want to have way just
get\set an Album as String without needs to keep in mind multilangual
or any other futures of some tags.

I have some expirience of converting one tag type to another. My
best-worked solution was to define list of roles like
enum AtributeRole {
Album,
Artist,
Title,
AlbumArt,
AlbumArtist,
etc...
};

Common interface to acces tag field;
Then every tag type has some mapping from roles to native tag fields.
Methods to determinate role of native tag and to enumerate set of tag
fields by role.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: asfpics.zip
Type: application/zip
Size: 6767 bytes
Desc: not available
Url : http://mail.kde.org/pipermail/taglib-devel/attachments/20100905/2f9b9c87/attachment.zip