[FFmpeg-devel] [PATCH] ogg muxer

[Baptiste Coudurier]

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.

>>+    init_checksum(&s->pb, av_crc04C11DB7_update, 0);
>>+    put_tag(&s->pb, "OggS");
>>+    put_byte(&s->pb, 0);
>>+    put_byte(&s->pb, flags);
>>+    put_le64(&s->pb, granule);
>>+    put_le32(&s->pb, stream_index);
>>+    put_le32(&s->pb, oggstream->page_counter++);
>>+    crc_offset = url_ftell(&s->pb);
>>+    put_le32(&s->pb, 0); // crc
>>+    put_byte(&s->pb, page_segments);
>>+    for (i = 0; i < page_segments-1; i++)
>>+        put_byte(&s->pb, 255);
>>+    if (size) {
>>+        put_byte(&s->pb, size - (page_segments-1)*255);
>>+        put_buffer(&s->pb, data, size);
>>+    }
>>+    ogg_update_checksum(s, crc_offset);
>>+    put_flush_packet(&s->pb);
>>+    return size;
>>+}
>>+
>>+static int ogg_write_header(AVFormatContext *s)
>>+{
>>+    int i, j;
>>+    for (i = 0; i < s->nb_streams; i++) {
>>+        AVStream *st = s->streams[i];
>>+        if (st->codec->codec_type == CODEC_TYPE_AUDIO)
>>+            av_set_pts_info(st, 64, 1, st->codec->sample_rate);
>>+        else if (st->codec->codec_type == CODEC_TYPE_VIDEO)
>>+            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.

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

>>+            OGGStreamContext *oggstream;
>>+            if (!st->codec->extradata || !st->codec->extradata_size) {
>>+                av_log(s, AV_LOG_ERROR, "No extradata present\n");
>>+                return -1;
>>+            }
>>+            oggstream = av_mallocz(sizeof(OGGStreamContext));
> 
> 
> sizeof(*oggstream)

Ok.

> [...]
> 
> 80 columns.  A comment explaining what that number is would be useful
> too.  BTW, can that value be trusted?  I guess we have no choice but
> to trust the encoder.

Added comment.
I think we have to trust it, according to theora specs.

> [...]
>
>>+        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.

> 
>>+        ptr  += ret; size -= ret;
> 
> 
> Split that line.
> 

I prefer it this way, those 2 lines are reflecting one logical action.

-- 
Baptiste COUDURIER                              GnuPG Key Id: 0x5C1ABAAA
SMARTJOG S.A.                                    http://www.smartjog.com
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
Phone: +33 1 49966312
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: ogg_muxer.patch
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20071028/4e6891b9/attachment.asc>