[FFmpeg-devel] [PATCH] id3v2 unsynchronisation support

[Alexander Kojevnikov]

On 24 July 2010 18:48, Reimar D?ffinger <Reimar.Doeffinger at gmx.de> wrote:
> On Sat, Jul 24, 2010 at 03:54:18PM +1000, Alexander Kojevnikov wrote:
>> --- a/libavformat/aviobuf.c
>> +++ b/libavformat/aviobuf.c
>> @@ -387,6 +387,17 @@ void init_checksum(ByteIOContext *s,
>> ? ? ?}
>> ?}
>>
>> +int peek_byte(ByteIOContext *s)
>> +{
>> + ? ?int byte;
>> + ? ?unsigned char *buf_ptr;
>> +
>> + ? ?buf_ptr = s->buf_ptr;
>> + ? ?byte = get_byte(s);
>> + ? ?s->buf_ptr = buf_ptr;
>> + ? ?return byte;
>> +}
>> +
>
> I don't think this is valid in the EOF case.
> I think the two options are: duplicating the code
> (maybe even using a macro since url_fgetc already
> duplicates it) or making a peek function and basing
> the get one on it (might decrease performance
> if the compiler does not inline).
> Maybe Michael should say if he prefers one.

I duplicated the code, let me know if you prefer a macro or basing
everything on the peek function.

>> -static void read_ttag(AVFormatContext *s, int taglen, const char *key)
>> +static int get_byte_resync (ByteIOContext *s, int *len)
>> +{
>> + ? ?int val;
>> +
>> + ? ?if (--*len < 0)
>> + ? ? ? ?return 0;
>> + ? ?val = get_byte (s);
>> + ? ?if (val == 0xff && !peek_byte (s)) {
>> + ? ? ? ?if (--*len < 0)
>> + ? ? ? ? ? ?return 0;
>> + ? ? ? ?get_byte (s);
>> + ? ?}
>> + ? ?return val;
>> +}
>
> I don't think 0 should be returned when 0xff is the last
> in the tag.
> Also I don't think len should be decreased when nothing is read.
> And the spaces after the function name are inconsistent with all
> other code in FFmpeg.
>
> if (*len <= 0)
> ? ?return 0;
> val = get_byte(s);
> *len--;
> if (val == 0xff && *len > 0 && !peek_byte(s)) {
> ? ?get_byte(s);
> ? ?*len--;
> }
> return val;

Done, thanks for the snippet.

On a side note, I'm seeing bogus "value computed is not used" warnings
with this code (gcc 4.5.0), should something be done about them?

>> @@ -95,9 +122,17 @@ static void read_ttag(AVFormatContext *s, int taglen, const char *key)
>>
>> ? ? ?case 0: ?/* ISO-8859-1 (0 - 255 maps directly into unicode) */
>> ? ? ? ? ?q = dst;
>> + ? ? ? ?byte = 0;
>> ? ? ? ? ?while (taglen-- && q - dst < dstlen - 7) {
>> ? ? ? ? ? ? ?uint8_t tmp;
>> - ? ? ? ? ? ?PUT_UTF8(get_byte(s->pb), tmp, *q++ = tmp;)
>> + ? ? ? ? ? ?if (resync && byte == 0xff) {
>> + ? ? ? ? ? ? ? ?byte = get_byte (s->pb);
>> + ? ? ? ? ? ? ? ?if (!byte)
>> + ? ? ? ? ? ? ? ? ? ?continue; // unsynchronisation byte, skip it
>> + ? ? ? ? ? ?} else {
>> + ? ? ? ? ? ? ? ?byte = get_byte (s->pb);
>> + ? ? ? ? ? ?}
>> + ? ? ? ? ? ?PUT_UTF8(byte, tmp, *q++ = tmp;)
>
>
> I can't see why you are not using get_byte_resync here.

Done.

>> - ? ? ? ?len = FFMIN(taglen, dstlen);
>> - ? ? ? ?get_buffer(s->pb, dst, len);
>> - ? ? ? ?dst[len] = 0;
>> + ? ? ? ?q = dst;
>> + ? ? ? ?byte = 0;
>> + ? ? ? ?while (taglen-- && q - dst < dstlen - 7) {
>> + ? ? ? ? ? ?if (resync && byte == 0xff) {
>> + ? ? ? ? ? ? ? ?byte = get_byte (s->pb);
>> + ? ? ? ? ? ? ? ?if (!byte)
>> + ? ? ? ? ? ? ? ? ? ?continue;
>> + ? ? ? ? ? ?} else {
>> + ? ? ? ? ? ? ? ?byte = get_byte (s->pb);
>> + ? ? ? ? ? ?}
>> + ? ? ? ? ? ?*q++ = byte;
>> + ? ? ? ?}
>> + ? ? ? ?*q = 0;
>
> Same here.

Done.

> Also the dstlen - 7 is wrong here, there's no
> PUT_UTF8 here.

Done, at least now I know where 7 comes from.

> Using the len from the FFMIN actually should work fine here still
> for both cases.

Done.

Thanks for the review!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-aviobuf-add-peek_byte.patch
Type: text/x-patch
Size: 1448 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100724/f3d0f23c/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-id3v2-unsynchronisation-support.patch
Type: text/x-patch
Size: 5163 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100724/f3d0f23c/attachment-0001.bin>