[FFmpeg-devel] [PATCH] movenc.c: fix metadata writing.

[Baptiste Coudurier]

Hi,

On 03/03/2011 06:31 PM, Ronald S. Bultje wrote:
> Fix metadata writing, broken in commit:
> 0abdb2931719d96dee725e555e9b46b2b2f8a6be
> because some characters in the string are>=0x80 and therefore the
> shift on characters in the fourcc broke down because of sign extension.
> ---
>   libavformat/movenc.c |   58 +++++++++++++++++++++++++-------------------------
>   1 files changed, 29 insertions(+), 29 deletions(-)
>
> diff --git a/libavformat/movenc.c b/libavformat/movenc.c
> index 231976b..1e6aec9 100644
> --- a/libavformat/movenc.c
> +++ b/libavformat/movenc.c
> @@ -1472,12 +1472,12 @@ static int mov_write_string_data_tag(AVIOContext *pb, const char *data, int lang
>       }
>   }
>
> -static int mov_write_string_tag(AVIOContext *pb, const char *name, const char *value, int lang, int long_style){
> +static int mov_write_string_tag(AVIOContext *pb, uint32_t name, const char *value, int lang, int long_style){
>       int size = 0;
>       if (value&&  value[0]) {
>           int64_t pos = url_ftell(pb);
>           avio_wb32(pb, 0); /* size */
> -        ffio_wfourcc(pb, name);
> +        avio_wl32(pb, name);
>           mov_write_string_data_tag(pb, value, lang, long_style);
>           size= updateSize(pb, pos);
>       }
> @@ -1485,7 +1485,7 @@ static int mov_write_string_tag(AVIOContext *pb, const char *name, const char *v
>   }
>
>   static int mov_write_string_metadata(AVFormatContext *s, AVIOContext *pb,
> -                                     const char *name, const char *tag,
> +                                     uint32_t name, const char *tag,
>                                        int long_style)
>   {
>       int l, lang = 0, len, len2;
> @@ -1537,23 +1537,23 @@ static int mov_write_ilst_tag(AVIOContext *pb, MOVMuxContext *mov,
>       int64_t pos = url_ftell(pb);
>       avio_wb32(pb, 0); /* size */
>       ffio_wfourcc(pb, "ilst");
> -    mov_write_string_metadata(s, pb, "\251nam", "title"    , 1);
> -    mov_write_string_metadata(s, pb, "\251ART", "artist"   , 1);
> -    mov_write_string_metadata(s, pb, "aART", "album_artist", 1);
> -    mov_write_string_metadata(s, pb, "\251wrt", "composer" , 1);
> -    mov_write_string_metadata(s, pb, "\251alb", "album"    , 1);
> -    mov_write_string_metadata(s, pb, "\251day", "date"     , 1);
> -    mov_write_string_tag(pb, "\251too", LIBAVFORMAT_IDENT, 0, 1);
> -    mov_write_string_metadata(s, pb, "\251cmt", "comment"  , 1);
> -    mov_write_string_metadata(s, pb, "\251gen", "genre"    , 1);
> -    mov_write_string_metadata(s, pb, "\251cpy", "copyright", 1);
> -    mov_write_string_metadata(s, pb, "\251grp", "grouping" , 1);
> -    mov_write_string_metadata(s, pb, "\251lyr", "lyrics"   , 1);
> -    mov_write_string_metadata(s, pb, "desc",    "description",1);
> -    mov_write_string_metadata(s, pb, "ldes",    "synopsis" , 1);
> -    mov_write_string_metadata(s, pb, "tvsh",    "show"     , 1);
> -    mov_write_string_metadata(s, pb, "tven",    "episode_id",1);
> -    mov_write_string_metadata(s, pb, "tvnn",    "network"  , 1);
> +    mov_write_string_metadata(s, pb, MKTAG(0xa9,'n','a','m'), "title"    , 1);
> +    mov_write_string_metadata(s, pb, MKTAG(0xa9,'A','R','T'), "artist"   , 1);
> +    mov_write_string_metadata(s, pb, MKTAG('a' ,'A','R','T'), "album_artist", 1);
> +    mov_write_string_metadata(s, pb, MKTAG(0xa9,'w','r','t'), "composer" , 1);
> +    mov_write_string_metadata(s, pb, MKTAG(0xa9,'a','l','b'), "album"    , 1);
> +    mov_write_string_metadata(s, pb, MKTAG(0xa9,'d','a','y'), "date"     , 1);
> +    mov_write_string_tag(pb, MKTAG(0xa9,'t','o','o'), LIBAVFORMAT_IDENT, 0, 1);
> +    mov_write_string_metadata(s, pb, MKTAG(0xa9,'c','m','t'), "comment"  , 1);
> +    mov_write_string_metadata(s, pb, MKTAG(0xa9,'g','e','n'), "genre"    , 1);
> +    mov_write_string_metadata(s, pb, MKTAG(0xa9,'c','p','y'), "copyright", 1);
> +    mov_write_string_metadata(s, pb, MKTAG(0xa9,'g','r','p'), "grouping" , 1);
> +    mov_write_string_metadata(s, pb, MKTAG(0xa9,'l','y','r'), "lyrics"   , 1);
> +    mov_write_string_metadata(s, pb, MKTAG('d' ,'e','s','c'), "description",1);
> +    mov_write_string_metadata(s, pb, MKTAG('l' ,'d','e','s'), "synopsis" , 1);
> +    mov_write_string_metadata(s, pb, MKTAG('t' ,'v','s','h'), "show"     , 1);
> +    mov_write_string_metadata(s, pb, MKTAG('t' ,'v','e','n'), "episode_id",1);
> +    mov_write_string_metadata(s, pb, MKTAG('t' ,'v','n','n'), "network"  , 1);

I don't think continuing in this direction does any good. Please revert 
and reintroduce put_tag.
This makes the code more difficult to maintain and read.

-- 
Baptiste COUDURIER
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
FFmpeg maintainer                                  http://www.ffmpeg.org