[FFmpeg-devel] Suggestion for a centralized language-tag facility in libavformat

[Michael Niedermayer]

On Mon, Apr 20, 2009 at 12:07:47AM +0200, cyril comparon wrote:
> Here is the new implementation and its use in the asf demuxer and muxer.
> The algorithm has become fairly more complicated than originally :)
> but it works perfectly.

[...]
> +static int langTable_compare(const void *lhs, const void *rhs)
> +{
> +    return strcmp(((const LangEntry *)lhs)->str, ((const LangEntry *)rhs)->str);
> +}
> +

> +const char *av_convertLangTo(const char *lang, enum AV_LangCodespace targetCodespace)
> +{
> +    int i;
> +    LangEntry searchedEntry;
> +    const LangEntry *entry = NULL;
> +    const int NB_CODESPACES = sizeof(langTableCounts)/sizeof(*langTableCounts);

> +    assert(targetCodespace >= 0 && targetCodespace < NB_CODESPACES);

I would prefer if this would be a hard check instead of a assert()


> +
> +    av_strlcpy(searchedEntry.str, lang, 4);

this and searchedEntry are redundant


> +    for (i=0; i<NB_CODESPACES; i++) {
> +        void *res = bsearch(&searchedEntry,
> +                            langTable + langTableOffsets[i],
> +                            langTableCounts[i],
> +                            sizeof(LangEntry),
> +                            langTable_compare);
> +        if (res) {
> +            entry = res;
> +            break;
> +        }

the temporary variable res is redundant as well


[...]
> Index: libavformat/asfdec.c
> ===================================================================
> --- libavformat/asfdec.c	(revision 18630)
> +++ libavformat/asfdec.c	(working copy)
> @@ -25,6 +25,7 @@
>  #include "riff.h"
>  #include "asf.h"
>  #include "asfcrypt.h"
> +#include "avlanguage.h"
>  
>  void ff_mms_set_stream_selection(URLContext *h, AVFormatContext *format);
>  
> @@ -76,6 +77,7 @@
>      else PRINT_IF_GUID(g, ff_asf_ext_stream_audio_stream);
>      else PRINT_IF_GUID(g, ff_asf_metadata_header);
>      else PRINT_IF_GUID(g, stream_bitrate_guid);
> +    else PRINT_IF_GUID(g, ff_asf_language_guid);
>      else
>          dprintf(NULL, "(GUID: unknown) ");
>      for(i=0;i<16;i++)

> @@ -403,6 +405,16 @@
>  //                av_log(s, AV_LOG_ERROR, "flags: 0x%x stream id %d, bitrate %d\n", flags, stream_id, bitrate);
>                  asf->stream_bitrates[stream_id]= bitrate;
>              }
> +        } else if (!guidcmp(&g, &ff_asf_language_guid)) {
> +            int stream_count = get_le16(pb);
> +            int j;
> +            for(j = 0; j < stream_count; j++) {
> +                char lang[1024];
> +                uint8_t lang_len;
> +                lang_len = get_byte(pb);
> +                get_str16_nolen(pb, lang_len, lang, sizeof(lang));
> +                asf->stream_languages[j] = av_strdup(lang);
> +            }
>          } else if (!guidcmp(&g, &ff_asf_extended_content_header)) {
>              int desc_count, i;
>  

exploitable


> @@ -455,7 +467,7 @@
>              get_le32(pb); // max-object-size
>              get_le32(pb); // flags (reliable,seekable,no_cleanpoints?,resend-live-cleanpoints, rest of bits reserved)
>              stream_num = get_le16(pb); // stream-num
> -            get_le16(pb); // stream-language-id-index
> +            asf->streams[stream_num].stream_language_index = get_le16(pb); // stream-language-id-index
>              get_le64(pb); // avg frametime in 100ns units
>              stream_ct = get_le16(pb); //stream-name-count
>              payload_ext_ct = get_le16(pb); //payload-extension-system-count

> @@ -534,7 +546,16 @@
>                  av_reduce(&st->sample_aspect_ratio.num,
>                            &st->sample_aspect_ratio.den,
>                            dar[i].num, dar[i].den, INT_MAX);
> -//av_log(s, AV_LOG_ERROR, "dar %d:%d sar=%d:%d\n", dar[i].num, dar[i].den, st->sample_aspect_ratio.num, st->sample_aspect_ratio.den);

unrelated


> +
> +            // copy and convert language codes to the frontend
> +            uint16_t stream_language_index = asf->streams[i].stream_language_index;

mixing of declaration and statement breaks gcc 2.95


> +            const char *rfc1766 = asf->stream_languages[stream_language_index];

the array isnt as large as the index may be


[...]
>  #include <assert.h>
> @@ -364,6 +365,32 @@
>          end_header(pb, hpos);
>      }
>  
> +    /* language list */
> +    hpos = put_header(pb, &ff_asf_language_guid);
> +    {
> +        unsigned int streamsWithSupportedLang = 0;
> +        for(n=0;n<s->nb_streams;n++) {
> +            AVMetadataTag *lang_tag = av_metadata_get(s->streams[n]->metadata, "language", NULL, 0);
> +            if (lang_tag) {
> +                const char *language = av_convertLangTo(lang_tag->value, AV_LANG_ISO639_1);
> +                if (language)
> +                    streamsWithSupportedLang++;
> +            }
> +        }
> +        put_le16(pb, streamsWithSupportedLang);
> +    }
> +    for(n=0;n<s->nb_streams;n++) {
> +        AVMetadataTag *lang_tag = av_metadata_get(s->streams[n]->metadata, "language", NULL, 0);
> +        if (lang_tag) {
> +            const char *language = av_convertLangTo(lang_tag->value, AV_LANG_ISO639_1);
> +            if (language) {
> +                put_byte(pb, 2*(1+strlen(language)));
> +                put_str16_nolen(pb, language);
> +            }
> +        }
> +    }
> +    end_header(pb, hpos);

that looks like it randomly stores languages so that they no longer can be
matched to the streams

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Opposition brings concord. Out of discord comes the fairest harmony.
-- Heraclitus
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090420/e5fd4fea/attachment.pgp>