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

[Reynaldo H. Verdejo Pinochet]

Hi

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.

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

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

Useless ()

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

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

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

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

--
Reynaldo