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

[Mark Reid]

On Mon, Feb 16, 2015 at 4:07 AM, <tomas.hardin at codemill.se> wrote:

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


will do.


>
>
>  +#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
> :)
>
> I wasn't to thrilled about the 2 bytes either, but seeing that the
function is part of the public api, I didn't want to break anything.


>
>  +    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?


I couldn't find too much stuff writing utf-16 strings,  id3v2, mmst and
subtiles. I think most of them calculate the length after writing.
would utils/avstring.c be good place to put it?


>
>  +/*
>> + * 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.
>

 Okay, I'll split it into two patches and send a new set, thanks for taking
the time to review