On 09/02/2011 07:13 PM, Ronald S. Bultje wrote:
> Hi,
>
> On Thu, Sep 1, 2011 at 11:20 PM, David Goldwich
> <[email protected]> wrote:
>> + /* read encoding type byte */
>> + encoding = avio_r8(pb);
>> + taglen--;
>> + switch (encoding) {
>> + case ID3v2_ENCODING_ISO8859:
>> + case ID3v2_ENCODING_UTF8:
>> + get_str = avio_get_str;
>> + break;
>> + case ID3v2_ENCODING_UTF16BE:
>> + case ID3v2_ENCODING_UTF16BOM:
>> + get_str = avio_get_str16be;
>> + break;
>> + default:
>> + av_log(s, AV_LOG_ERROR, "Unknown encoding in GEOB tag.\n");
>> + return;
>> + }
>
> This appears duplicated in various places in this file now, maybe generalize.
I can rework the code below, then hopefully there should be less
redundancy ...
>> + /* peek forward to figure out the length of the content part and decode
>> it */
>> +#define PEEK_DECODE(get, key, encoding) \
>> + pos = avio_tell(pb);\
>> + len = get(pb, taglen, buf, sizeof(buf));\
>> + geob_data->key = av_mallocz(len + 7);\
>> + if (!geob_data->key) {\
>> + av_log(s, AV_LOG_ERROR, "Failed to alloc %d bytes\n", len + 7);\
>> + return;\
>> + }\
>> + avio_seek(pb, pos, SEEK_SET);\
>> + decode_content(s, pb, encoding, geob_data->key, len + 7, len, "GEOB");\
>> + taglen -= len;\
>
> This is admittedly ugly and fails with streaming content, can you
> somehow do this by reallocating the buffer somehow if the orig alloc
> isn't big enough?
Ugly it is! As is ID3. GEOB frames are especially ugly because they
contain several fields of variable length with potentially different
encodings and the only thing you know is that they are separated by null
chars, which, to spice things up, can be one or two bytes depending on
the encoding. So I think, reading it byte by byte and reallocating the
buffer would be the best idea ...
> Is there a typical length for these tags?
You can't say anything about the typical length technically, it's a
general purpose container for binary data. The maximal length is a bit
less than 16 MB for ID3v2.2 and a bit less than 256 MB for .3/4.
> Also doesn't check against overflows for the +7.
The +7 comes from these lines in the original content decoding code
> while (taglen > 1 && q - dst < dstlen - 7) {
so it just ignores the last 7 bytes of dst. To be honest, I have no clue
why it checks for dstlen-7 here, nor was there any documentation about
it. I can't see any reason why it should be like this, but I didn't feel
like breaking something, so I adopted my buffers by making them 7 bytes
bigger. (The ttag parser uses a constant 512 byte buffer which is way
longer than most text tag content, so it's not noticeable there most of
the time).
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
