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

[Baptiste Coudurier]

On 05/24/2010 05:22 PM, M?ns Rullg?rd wrote:
> Baptiste Coudurier<baptiste.coudurier at gmail.com>  writes:
>
>> On 05/24/2010 05:08 PM, M?ns Rullg?rd wrote:
>>> Baptiste Coudurier<baptiste.coudurier at gmail.com>   writes:
>>>
>>>> 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
>>>>>>
>>>>>> +        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.
>>>
>>> IMHO you are being lazy.  At the very least, could you provide some
>>> rationale for this idiotic change?
>>>
>>
>> Nothing more than:
>> http://www.xiph.org/ogg/doc/rfc3533.txt
>>
>>     "Whole pages are taken in order
>>     from multiple logical bitstreams multiplexed at the page level.  The
>>     logical bitstreams are identified by a unique serial number in the
>>     header of each page of the physical bitstream.  This unique serial
>>     number is created randomly and does not have any connection to the
>>     content or encoder of the logical bitstream it represents."
>
> 0, 1, 2, 3 is a perfectly valid random sequence.  If the intent is to
> disallow consecutive numbers, the spec should say so.  As is, this
> code is nothing but obfuscation and abuse.  If you insist, use a
> proper PRNG, but stop torturing av_get_random_seed.  This may be your
> code, but not even you may misuse other functions.
>

Well, I've created av_get_random_seed after michael's request.
Michael suggested me to use get_random_seed to create the UMID for MXF 
and said it _was_ enough. I don't see why it wouldn't for OGG.

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