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

[tomas.hardin at codemill.se]

On 2015-02-16 20:24, Mark Reid wrote:
> 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.

Another thing: since this is adds another function to the API maybe the 
minor version should be bumped? I can't recall if the avio_* functions 
are strictly internal. If they are then there might not be a need.

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

Good call :)

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

Possibly, but I don't think we need to extend the API if nothing else 
needs 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

No problem

/Tomas