[FFmpeg-soc] [PATCH] xiph packetizer

[Josh Allmann]

On 27 July 2010 15:28, Josh Allmann <joshua.allmann at gmail.com> wrote:
> Hi,
>
> On 27 July 2010 03:03, Martin Storsjö <martin at martin.st> wrote:
>> On Mon, 26 Jul 2010, Josh Allmann wrote:
>>
>>> This version works better, but is not yet complete.
>>>
>>> -The scaffolding for Vorbis is mostly up, but I have to set its SDP properly.
>
> Done.
>
>>> -Theora video has problems with I-frames (or whatever the Theora
>>> equivalent is), and exhibits serious blocking in areas of motion.
>
> Fixed, although on occassion it is a bit wonky. The depacketizer will
> complain about a missing start fragment at a particular position in
> big buck bunny, disrupting the video briefly, but otherwise it is
> fine.
>
>>> I hope this is due to some invalid reads that Valgrind complains about.
>
> Valgrind fixed.
>
>>> -Packing multiple frames in a single packet is another TODO.
>>
>
> Not yet complete, but is not critical for proper operation, either.
>
>> A few comments:
>>
>> +    /* set xiph data type */
>> +    switch (*buff) {
>> +        case 0x01:   // vorbis id
>> +        case 0x05:   // vorbis setup
>> +        case 0x80:   // theora header
>> +        case 0x82:   // theora tables
>> +            xdt = 1; // packed config payload
>> +        case 0x03:   // vorbis comments
>> +        case 0x81:   // theora comments
>> +            xdt = 2; // comment payload
>> +        default:
>> +            xdt = 0; // raw data payload
>> +    }
>>
>> I guess you want break statements in the switch, too...
>>
>
> Fixed.
>
>> +    /* set ident
>> +     * Probably need a non-fixed way of generating
>> +     * this, but it has to be done in SDP and passed in from there. */
>> +    q = s->buf;
>> +    *q++ = 0xfe;
>> +    *q++ = 0xcd;
>> +    *q++ = 0xba;
>>
>> I haven't read the specs, but what's the role of this ident code? Is there
>> any harm in having it hardcoded to a specific value? Is it set in the
>> original stream data somewhere, so that you'd have to parse out the
>> correct value from there? Or is it only used to distinguish streams if you
>> have more than one vorbis/theora stream in the same presentation? In that
>> case, you could use e.g. one hardcoded value for vorbis and another for
>> theora - that would probably be enough for some time at lesat.
>
> As Luca said, it is only used to make sure the extradata doesn't
> change mid-stream. Different streams can have the same ident, eg a
> Vorbis and a Theora can share 0xfecdba. Our depacketizer doesn't
> handle changing the ident anyway.
>
> Revised patch attached. I also had to enlarge the outgoing RTSP buffer
> to handle the SDP extradata.
>

Re-sending, due to this hunk:

@@ -154,6 +156,11 @@ static int rtp_write_header(AVFormatContext *s1)
         }
     case CODEC_ID_AAC:
         s->num_frames = 0;
+    case CODEC_ID_VORBIS:
+    case CODEC_ID_THEORA:
+        if(!s->max_frames_per_packet) s->max_frames_per_packet = 15;
+        s->max_frames_per_packet = av_clip(s->max_frames_per_packet, 1, 15);
+        s->max_payload_size -= 6; // ident+frag+tdt/vdt+pkt_num+pkt_length
     default:
         if (st->codec->codec_type == AVMEDIA_TYPE_AUDIO) {
             av_set_pts_info(st, 32, 1, st->codec->sample_rate);

Apparently AAC and AMR both fall through to the default case. Whether
that's intentional, I don't know, so I moved my stuff to minimize
behavioral changes to existing code.

Josh
-------------- next part --------------
A non-text attachment was scrubbed...
Name: xiph-packetizer.diff
Type: text/x-patch
Size: 11763 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-soc/attachments/20100727/f4e7b8ef/attachment.bin>