[FFmpeg-devel] [PATCH] libavformat/mxfenc: write package name metadata

[tomas.hardin at codemill.se]

On 2015-02-13 01:36, Mark Reid wrote:
>  /**
> + * Convert an UTF-8 string to UTF-16BE and write it.
> + * @return number of bytes written.
> + */
> +int avio_put_str16be(AVIOContext *s, const char *str);

You could maybe split this patch up by making the part that adds this 
function a separate patch. Not that I'm super concerned.

> +#define PUT_STR16(type, write) \
> +    int avio_put_str16 ##type(AVIOContext *s, const char *str)\
> +{\
> +    const uint8_t *q = str;\
> +    int ret = 0;\
> +    int err = 0;\
> +    while (*q) {\
> +        uint32_t ch;\
> +        uint16_t tmp;\
> +        GET_UTF8(ch, *q++, goto invalid;)\
> +        PUT_UTF16(ch, tmp, write(s, tmp); ret += 2;)\
> +        continue;\
> +invalid:\
> +        av_log(s, AV_LOG_ERROR, "Invaid UTF8 sequence in
> avio_put_str16" #type "\n");\
> +        err = AVERROR(EINVAL);\
> +    }\
> +    write(s, 0);\

 From the last e-mail:

> The tests need to be updated because avio_put_str16be writes zero 
> terminated strings and
> the muxer previously wasn't.

I was going to object to this on the grounds that it wastes a whopping 
two bytes per UTF-16 local tag, but I suspect the possible savings are 
eaten up by KAG alignment anyway. Code simplicity is favorable in this 
case I think :)

> +    if (err)\
> +    return err;\
> +    ret += 2;\
> +    return ret;\
> +}\
> +
> +PUT_STR16(le, avio_wl16)
> +PUT_STR16(be, avio_wb16)
> +
> +#undef PUT_STR16
> 
>  int ff_get_v_length(uint64_t val)
>  {
> diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
> index 17ad132..c25f5fd 100644
> --- a/libavformat/mxfenc.c
> +++ b/libavformat/mxfenc.c
> @@ -624,14 +624,44 @@ static void mxf_write_preface(AVFormatContext *s)
>  }
> 
>  /*
> - * Write a local tag containing an ascii string as utf-16
> + * Returns the length of the UTF-16 string, in 16-bit characters,
> that would result
> + * from decoding the utf-8 string.
> + */
> +static uint64_t mxf_utf16len(const char *utf8_str)
> +{
> +    const uint8_t *q = utf8_str;
> +    uint64_t size = 0;
> +    while (*q) {
> +        uint32_t ch;
> +        GET_UTF8(ch, *q++, goto invalid;)
> +        if (ch < 0x10000)
> +            size++;
> +        else
> +            size += 2;
> +        continue;
> +invalid:
> +        av_log(NULL, AV_LOG_ERROR, "Invaid UTF8 sequence in 
> mxf_utf16len\n\n");
> +    }
> +    size += 1;
> +    return size;
> +}

Maybe this could be useful elsewhere too? What's the state of UTF-16 
usage in lavf?

> +/*
> + * Write a local tag containing an utf-8 string as utf-16
>   */
>  static void mxf_write_local_tag_utf16(AVIOContext *pb, int tag, const
> char *value)
>  {
> -    int i, size = strlen(value);
> +    int size = mxf_utf16len(value);
>      mxf_write_local_tag(pb, size*2, tag);
> -    for (i = 0; i < size; i++)
> -        avio_wb16(pb, value[i]);
> +    avio_put_str16be(pb, value);
>  }

Wow, this thing was really broken before.

Overall the patch looks fine, apart from maybe splitting it up.

/Tomas