On Mon, Feb 16, 2015 at 4:07 AM, <tomas.har...@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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
