[FFmpeg-devel] [PATCH] RTP depacketizer for QCELP

[Martin Storsjö]

Hi Reynaldo,

On Tue, 30 Nov 2010, Reynaldo H. Verdejo Pinochet wrote:

> On 11/29/2010 02:19 PM, Martin Storsj? wrote:
> > [..]
> > I refactored this a bit, making both this code path and the old one much 
> > cleaner, see attached. The actual depacketizer is unchanged from the 
> > previous patchset.
> > 
> > // Martin
> >[..]
> >+struct InterleavePacket {
> >+    int pos;
> >+    int size;
> >+    /* The largest frame is 35 bytes, only 10 frames are allowed per
> >+     * packet, and we return the first one immediately, so allocate
> >+     * space for 9 frames */
> >+    uint8_t data[35*9];
> >+};
> >+
> >+struct PayloadContext {
> >+    int interleave_size;
> >+    int interleave_index;
> >+    struct InterleavePacket group[6];
> >+    int group_finished;
> >+
> >+    /* The maximum packet size, 10 frames of 35 bytes each, and one
> >+     * packet header byte. */
> >+    uint8_t  next_data[1 + 35*10];
> >+    int      next_size;
> >+    uint32_t next_timestamp;
> >+};
> 
> I'd proly go typedef those and avoid the repetitive 'struct XX' latter on.

Changed to a typedef

> >+static PayloadContext *qcelp_new_context(void)
> >+{
> >+    return av_mallocz(sizeof(PayloadContext));
> >+}
> >+
> >+static void qcelp_free_context(PayloadContext *data)
> >+{
> >+    av_free(data);
> >+}
> 
> Dunno what others might think but I'd just use av_mallocz/av_free
> when needed.

Uhm, could you repeat this, a bit more verbosely? I don't really 
understand what the issue here would be.

> >+    interleave_size  = (buf[0] >> 3) & 7;
> 
> Useless ()

Removed

> >[..]
> >+            for (; data->interleave_index <= data->interleave_size;
> >+                 data->interleave_index++)
> >+                data->group[data->interleave_index].size = 0;
> 
> At this point data->interleave_size == interleave_size isn't it?
> if that's correct you might want to change the stop condition.

Yes, at this point, they can be used interchangeably. Changed to use the 
local variable.

> You sure you didn't mean (data->group[data->interleave_index).size?

No, we need to zero all the slots up to the size, if interleave_size is 
e.g. 5 and we only had received packets up to slot 3, and then receive one 
belonging to the next interleave group, we need to zero out slots 4 and 5 
and return all the frames from the previous interleave group.

> >--- a/libavformat/rtpdec.c
> >+++ b/libavformat/rtpdec.c
> >@@ -27,6 +27,7 @@
> >[..]
> >+    RTPDynamicProtocolHandler *handler;
> >+    for (handler = RTPFirstDynamicPayloadHandler;
> >+         handler; handler = handler->next) {
> >+        if (!strcasecmp(name, handler->enc_name) &&
> >+            codec_type == handler->codec_type) {
> >+            return handler;
> >+        }
> >+    }
> 
> Maybe drop the braces on for and if.

Done

> >[..]
> >+RTPDynamicProtocolHandler *ff_rtp_dynamic_payload_handler_find_by_id(
> >+                               int id, enum AVMediaType codec_type)
> >+{
> >+    RTPDynamicProtocolHandler *handler;
> >+    for (handler = RTPFirstDynamicPayloadHandler;
> >+         handler; handler = handler->next) {
> >+        if (handler->static_payload_id && handler->static_payload_id
> >== id &&
> >+            codec_type == handler->codec_type) {
> >+            return handler;
> >+        }
> >+    }
> 
> Same

Done

What do you or others think about shortening those function names, btw? 
Would ff_rtp_handler_find_by_name be better? Or with semi-hierarchical 
naming, ff_rtp_find_handler_by_name?

// Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-rtpdec-Add-a-dynamic-payload-handler-for-the-x-Purev.patch
Type: text/x-diff
Size: 12098 bytes
Desc: 
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101201/b33b7b36/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-rtpdec-Allow-dynamic-payload-handlers-to-handle-stat.patch
Type: text/x-diff
Size: 996 bytes
Desc: 
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101201/b33b7b36/attachment-0001.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-rtpdec-Add-functions-for-finding-depacketizers-by-na.patch
Type: text/x-diff
Size: 2700 bytes
Desc: 
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101201/b33b7b36/attachment-0002.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-rtsp-Factorize-code-for-initializing-the-rtp-payload.patch
Type: text/x-diff
Size: 2371 bytes
Desc: 
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101201/b33b7b36/attachment-0003.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-rtsp-Look-for-RTP-payload-handlers-for-static-payloa.patch
Type: text/x-diff
Size: 1323 bytes
Desc: 
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101201/b33b7b36/attachment-0004.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0006-rtpdec_qcelp-Use-the-depacketizer-for-static-payload.patch
Type: text/x-diff
Size: 907 bytes
Desc: 
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101201/b33b7b36/attachment-0005.patch>