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

[Michael Niedermayer]

On Fri, Apr 24, 2009 at 08:38:20PM +0200, cyril comparon wrote:
> >> + ? ? ? ? ? ? ? ?get_str16_nolen(pb, lang_len, lang, sizeof(lang));
> >> + ? ? ? ? ? ? ? ?if (j < 128)
> >> + ? ? ? ? ? ? ? ? ? ?asf->stream_languages[j] = av_strdup(lang);
> >> + ? ? ? ? ? ?}
> >
> > instead of av_strdup() the initial array could be correctly allocated
> > also this code leaks, if there are 2 ff_asf_language_guids
> >
> 
> Fixed
> 
> >> + ? ? ? ?}
> >> + ? ? ? ?end_header(pb, hpos);
> >> + ? ?}
> >
> > trailing whitespace
> >
> 
> Fixed
> 
> I attach only the impacted patch.
> Cyril

>  Changelog            |    2 +
>  libavformat/Makefile |    4 +--
>  libavformat/asf.c    |    3 ++
>  libavformat/asf.h    |    5 ++++
>  libavformat/asfdec.c |   27 +++++++++++++++++++++++-
>  libavformat/asfenc.c |   57 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 95 insertions(+), 3 deletions(-)
> 0a5929058a3c77f935d23caa76325af7123c9f41  2_asf_languages-d.patch

> Index: Changelog
> ===================================================================
> --- Changelog	(revision 18680)
> +++ Changelog	(working copy)

> @@ -13,6 +13,8 @@
>  - RTP packetization of H.263
>  - RTP packetization of AMR
>  - RTP depacketization of Vorbis
> +- introduced avlanguage helpers in libavformat

that was the other patch ...


> +- per-stream language-tags extraction/storage in asfdec/asfenc
[...]
> @@ -85,6 +88,7 @@
>      int asfid2avid[128];                 ///< conversion table from asf ID 2 AVStream ID
>      ASFStream streams[128];              ///< it's max number and it's not that big
>      uint32_t stream_bitrates[128];       ///< max number of streams, bitrate for each (for streaming)
> +    char stream_languages[128][6];       ///< max number of streams, language for each (RFC1766, e.g. en-US)
>      /* non streamed additonnal info */
>      uint64_t nb_packets;                 ///< how many packets are there in the file, invalid if broadcasting
>      int64_t duration;                    ///< in 100ns units
> @@ -157,6 +161,7 @@
[...]
> @@ -455,7 +468,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

this looks bad, could you please review your patch for security issues, it
seems you take it very lightly if some data from the file is written outside
an array. I assume you also store your money and blank and signed pieces of
paper in front of your door assuming noone would do something nasty with it


[...]
> @@ -909,6 +933,7 @@
>      asf->packet_time_start = 0;
>  
>      for(i=0; i<s->nb_streams; i++){
> +        av_free(asf->stream_languages[i]);
>          asf_st= s->streams[i]->priv_data;
>          av_free_packet(&asf_st->pkt);
>          asf_st->frag_offset=0;

did you test your patch at all?

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

While the State exists there can be no freedom; when there is freedom there
will be no State. -- Vladimir Lenin
-------------- 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/20090425/4a789fa5/attachment.pgp>