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

[Måns Rullgård]

Baptiste Coudurier <baptiste.coudurier at gmail.com> writes:

> 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.

So?  I didn't say the function wasn't useful, only that you're using
it incorrectly.  What did you think "seed" meant?

> 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.

The mxf muxer calls it exactly once to obtain _one_ random number, not
a duplicate-free sequence.

-- 
M?ns Rullg?rd
mans at mansr.com