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

Måns Rullgård mans
Tue May 25 02:22:37 CEST 2010


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.

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



More information about the ffmpeg-cvslog mailing list