[FFmpeg-cvslog] r23302 - trunk/libavformat/oggenc.c

[Baptiste Coudurier]

On 05/24/2010 04:45 PM, M?ns Rullg?rd wrote:
> bcoudurier<subversion at mplayerhq.hu>  writes:
>
>> Author: bcoudurier
>> Date: Tue May 25 01:37:33 2010
>> New Revision: 23302
>>
>> Log:
>> In ogg muxer, use random serial number of each ogg streams
>>
>> Modified:
>>     trunk/libavformat/oggenc.c
>>
>> Modified: trunk/libavformat/oggenc.c
>> ==============================================================================
>> --- trunk/libavformat/oggenc.c	Mon May 24 23:59:32 2010	(r23301)
>> +++ trunk/libavformat/oggenc.c	Tue May 25 01:37:33 2010	(r23302)
>> @@ -20,6 +20,7 @@
>>    */
>>
>>   #include "libavutil/crc.h"
>> +#include "libavutil/random_seed.h"
>>   #include "libavcodec/xiph.h"
>>   #include "libavcodec/bytestream.h"
>>   #include "libavcodec/flac.h"
>> @@ -50,6 +51,7 @@ typedef struct {
>>       int eos;
>>       unsigned page_count; ///<  number of page buffered
>>       OGGPage page; ///<  current page
>> +    unsigned serial_num; ///<  serial number
>>   } OGGStreamContext;
>>
>>   typedef struct OGGPageList {
>> @@ -80,7 +82,7 @@ static void ogg_write_page(AVFormatConte
>>       put_byte(s->pb, 0);
>>       put_byte(s->pb, page->flags | extra_flags);
>>       put_le64(s->pb, page->granule);
>> -    put_le32(s->pb, page->stream_index);
>> +    put_le32(s->pb, oggstream->serial_num);
>>       put_le32(s->pb, oggstream->page_counter++);
>>       crc_offset = url_ftell(s->pb);
>>       put_le32(s->pb, 0); // crc
>> @@ -280,8 +282,11 @@ static int ogg_write_header(AVFormatCont
>>   {
>>       OGGStreamContext *oggstream;
>>       int i, j;
>> +
>>       for (i = 0; i<  s->nb_streams; i++) {
>>           AVStream *st = s->streams[i];
>> +        unsigned serial_num = i;
>> +
>>           if (st->codec->codec_type == AVMEDIA_TYPE_AUDIO)
>>               av_set_pts_info(st, 64, 1, st->codec->sample_rate);
>>           else if (st->codec->codec_type == AVMEDIA_TYPE_VIDEO)
>> @@ -300,6 +305,18 @@ static int ogg_write_header(AVFormatCont
>>           }
>>           oggstream = av_mallocz(sizeof(*oggstream));
>>           oggstream->page.stream_index = i;
>> +
>> +        if (!(st->codec->flags&  CODEC_FLAG_BITEXACT))
>> +            do {
>> +                serial_num = av_get_random_seed();
>
> This isn't how av_get_random_seed() is meant to be used.  On many
> systems this simply returns the current time, so you're likely to end
> up with a consecutive sequence here regardless.  You're supposed to
> use a seed with one of the PRNGs.
>
>> +                for (j = 0; j<  i; j++) {
>> +                    OGGStreamContext *sc = s->streams[j]->priv_data;
>> +                    if (serial_num == sc->serial_num)
>> +                        break;
>> +                }
>> +            } while (j<  i);
>> +        oggstream->serial_num = serial_num;
>
> This could potentially never terminate.  That is of course exceedingly
> unlikely, but I think such code should be avoided nonetheless.
>
> Please redo this properly, or not at all.  The ogg spec clearly allows
> serial_num = index.  Until that changes, there is no reason whatsoever
> to follow their stupid, made-up rules.
>

IMHO this is good enough. Feel free to submit a better solution.

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