Changes to write ID3v2.3 tags and unsynchronization

[Lukáš Lalinský]

On Sat, Oct 24, 2009 at 1:12 AM, Jonathan Potter <listsub at lss.com.au> wrote:
> 1. id3v2.3 support
>
> The default id3v2 version is set via the ID3DEFVERSION define. The basic
> idea is:
>
> - If ID3DEFVERSION is defined as 4, a version 4 tag will always be written -
> this is the existing behaviour
> - If ID3DEFVERSION is defined as 3, a version 3 tag will be written UNLESS
> the file already had a version 4 tag, in which case the version 4 tag will
> be preserved
>
> When saving a version 3 tag, any version 4-specific frames will be converted
> back to version 3 frames if possible - currently this only happens with the
> TDRC frame which is converted back to TYER. If ID3VER3DUMPVER4TAGS is
> defined then any version 4 frames which couldn't be converted will be
> dropped from the tag.
>
> Note that ID3DEFVERSION is currently defined twice, in id3v2frame.h and
> id3v2header.h (this is due to the seeming lack of a common header file for
> the whole project).

I'm sorry, but I must say that I don't like this. I don't think having
a compile-time option is a good idea. The library should support both
versions and the decision whether to save ID2v2.3 or ID3v2.4 should be
a run-time option.

Also, the way you downgrade ID3v2.4 frames to ID3v2.3 is incomplete.
For TDRC, you should use TYER + TDAT + TIME, not just TYER. See
http://bzr.oxygene.sk/picard/trunk/picard/formats/mutagenext/compatid3.py#l-152
for a more complete implementation.

Regarding text encoding, I think instead of checkEncoding, there
should be a function on Tag that downgrades tags to ID3v2.3 and this
would include changing UTF-8 to UTF-16. You forgot to check the
version-aware encoding in unsynchronizedlyricsframe,
synchronizedlyricsframe and generalencapsulatedobjectframe (which
currently doesn't call checkEncoding at all).

And the downgrade code should also make sure that all text frames have
only a single value. Saving text frames with multiple values is not
supported by ID3v2.3.

Some other comments about the code:
 - to render ID3v2.3 header size, use ByteVector::fromUInt instead of
a custom code.
 - the code doesn't follow the style in the rest of TagLib
 - no unit tests

> 2. Unsynchronisation
>
> - If ID3VER4UNSYNC is defined, unsynchronisation will be performed on a
> per-frame basis if needed (when writing a version 4 tag)
> - If ID3VER3UNSYNC is defined, unsynchronisation will be performed on a
> whole-tag basis if needed
>
> Added a number of functions in id3v2synchdata.cpp to support this

TagLib never encoded the frame data. Is there a strong reason to add
it? TagLib used to not even support reading synch-encoded tags until
1.5. I think I'd prefer to keep the data without encoding for
compatibility reasons.

> 3. Fixed (mostly by casting) some minor compile issues affecting Windows
>
> - A number of minor compile warnings from Visual Studio were fixed
> - A number of x64 (64 bit) compile problems were also fixed

It would be nice to submit these separately, as they can go to 1.6.1.
The v2.3 support can't.

> 4. Added utility function in mpegfile.cpp (replaceFrame) to automatically
> replace any existing frame(s) with a new one.

-- 
Lukas Lalinsky
lalinsky at gmail.com