[FFmpeg-devel] [PATCH] ogg muxer

[Måns Rullgård]

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

> M?ns Rullg?rd wrote:
>> Baptiste Coudurier <baptiste.coudurier at smartjog.com> writes:
>> 
>> 
>>>Hi
>>>
>>>$subject.
>> 
>> 
>> Neat, but why?  Ogg is IMHO such a nasty format that its use should be
>> actively discouraged.  Yet, there may be some reason to want this.
>> 
>
> Sake of completeness, I'd say.
>
>> [...]
>> 
>> page_segments = (size + 254) / 255;
>
> I prefer (size/255)+1 which illustrates better
> what the code below does.

Rethinking it, what I suggested is wrong for size%255 == 0.  What you
want is page_segments = size/255 + !!size.

>>>+            av_set_pts_info(st, 64, st->codec->time_base.num, st->codec->time_base.den);
>> 
>> 
>> Please try to keep lines less than 80 characters in length.
>
> I prefer not to split lines at 80 columns when lines are reasonably long
> but please I see no point starting to troll about it.

I personally find lines that don't wrap much easier to read.  That
said, I do make exceptions when there is no natural place to split the
line.

>>>+        if (st->codec->codec_id == CODEC_ID_VORBIS || st->codec->codec_id == CODEC_ID_THEORA) {
>> 
>> 
>> Consider using a switch statement here to ease addition of more codecs.
>
> I'll change it when muxer supports more codecs.

Why not now?  At the very least, split the line after the ||.

>> [...]
>>
>>>+        granule = pkt->pts + pkt->duration;
>> 
>> 
>> This is wrong.  The granule number refers to the last sample of the
>> page, which is one less than this.
>> 
>
> Vorbis specs:
>
> "The granule position of a page represents the end PCM sample position
> of the last packet completed on that page."
>
> Ogg specs:
>
> absolute granule position
>
> "(This is packed in the same way the rest of Ogg data is packed; LSb of
> LSB first. Note that the 'position' data specifies a 'sample' number
> (eg, in a CD quality sample is four octets, 16 bits for left and 16 bits
> for right; in video it would likely be the frame number. It is up to the
> specific codec in use to define the semantic meaning of the granule
> position value). The position specified is the total samples encoded
> after including all packets finished on this page (packets begun on this
> page but continuing on to the next page do not count). The rationale
> here is that the position specified in the frame header of the last page
> tells how long the data coded by the bitstream is."
>
> "Example: samplestamp (Vorbis)
>
> Frame counting is insufficient in codecs such as Vorbis where an audio
> frame [packet] encodes a variable number of samples. In Vorbis's case,
> the granule position is a count of the number of raw samples from the
> beginning of stream; the absolute time of a granule position is
> [granule_position] / [samples_per_second]."
>
> I find it quite confusing, and it seems current muxer using libogg
> writes similar granule values.

Hmm, seems I misremembered.  Sorry about that.

>> 
>>>+        ptr  += ret; size -= ret;
>> 
>> 
>> Split that line.
>> 
>
> I prefer it this way, those 2 lines are reflecting one logical action.

I guess we disagree.

All else aside, you've forgotten to update allformats.c

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