[FFmpeg-cvslog] r23302 - trunk/libavformat/oggenc.c
Måns Rullgård
mans
Tue May 25 11:53:21 CEST 2010
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
More information about the ffmpeg-cvslog
mailing list