[FFmpeg-devel] [PATCH] id3v2 unsynchronisation support

[Reimar Döffinger]

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.

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

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

> -        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.
Also the dstlen - 7 is wrong here, there's no
PUT_UTF8 here.
Using the len from the FFMIN actually should work fine here still
for both cases.