Hi,

On 21 June 2010 02:53, Martin Storsjö <[email protected]> wrote:
> Hi Josh,
>
> For part 1, I don't have too much opinion...
>
> On Sun, 20 Jun 2010, Josh Allmann wrote:
>
>> From dc9586eef1ef936b8fcaca1cbaeb7d5d7690d036 Mon Sep 17 00:00:00 2001
>> From: Josh Allmann <[email protected]>
>> Date: Sun, 20 Jun 2010 12:25:59 -0700
>> Subject: [PATCH 2/2] Decouple MPEG-4 and AAC specific parts from rtsp.c.
>>
>> ---
>>  libavformat/Makefile      |    1 +
>>  libavformat/rtpdec.c      |    8 +--
>>  libavformat/rtpdec_mpeg.c |  123 
>> +++++++++++++++++++++++++++++++++++++++++++++
>>  libavformat/rtpdec_mpeg.h |   38 ++++++++++++++
>>  libavformat/rtsp.c        |   58 ---------------------
>>  5 files changed, 165 insertions(+), 63 deletions(-)
>>  create mode 100644 libavformat/rtpdec_mpeg.c
>>  create mode 100644 libavformat/rtpdec_mpeg.h
>
> I think mpeg4 would be a better/more specific name for this, since it
> doesn't cover mpeg1/2 afaik.
>

Fixed.

> While looks like a good first step, it doesn't remove all of the mpeg4
> stuff from rtsp.c.
>

I was afraid of that.

> There's still the AttrNameMap, attr_names and sdp_parse_fmtp which does
> nothing but AAC-specific things. These routines parse parameters and set
> them in the deceivingly generically named RTPPayloadData, which despite
> the name isn't used by any other format than AAC. (Additionally, this
> struct is allocated and owned by the RTSP layer.) So all of this code
> could be moved out to the dynamic payload handler, and be parsed by a
> normal parse_sdp_a_line just as for all other formats. There's also quite
> a bit of AAC specific parsing code in rtpdec.c that needs to be moved to
> the dynamic payload handler, too.
>

Indeed, wasn't sure if those were used elsewhere, so I left them in.
Will send separate patches to move this out.

> Side note: When looking at that code, there's a small bit of code for
> depacketizing MP2/MP3 and MPEG1VIDEO/MPEG2VIDEO, too, that perhaps could
> be moved out, but I'm not totally sure on how to handle it since they
> aren't dynamic payload formats at all. (The only difference in practice is
> that they have hardcoded RTP payload numbers, but they could just as well
> use the same structure anyway.)
>
> To do this cleanly, I guess it's best to do this in smaller chunks, and as
> such, I think your current patch #2 looks quite ok, except for the naming.

Okay, updated patches attached, with more forthcoming.
(Ignore the numbering; that teaches me to do git rebase instead of git pull...)

Josh
From 405ea5dcaed413fc07898b0a138505e53bd942a2 Mon Sep 17 00:00:00 2001
From: Josh Allmann <[email protected]>
Date: Sun, 20 Jun 2010 12:12:22 -0700
Subject: [PATCH 1/2] Move skip_spaces to libavformat/internal.h

---
 libavformat/internal.h |   13 +++++++++++++
 libavformat/rtsp.c     |   13 -------------
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/libavformat/internal.h b/libavformat/internal.h
index 4489ffe..4ca5c55 100644
--- a/libavformat/internal.h
+++ b/libavformat/internal.h
@@ -174,4 +174,17 @@ void ff_sdp_write_media(char *buff, int size, AVCodecContext *c,
 int ff_write_chained(AVFormatContext *dst, int dst_stream, AVPacket *pkt,
                      AVFormatContext *src);
 
+#define SPACE_CHARS " \t\r\n"
+/* we use memchr() instead of strchr() here because strchr() will return
+ * the terminating '\0' of SPACE_CHARS instead of NULL if c is '\0'. */
+#define redir_isspace(c) memchr(SPACE_CHARS, c, 4)
+static inline void skip_spaces(const char **pp)
+{
+    const char *p;
+    p = *pp;
+    while (redir_isspace(*p))
+        p++;
+    *pp = p;
+}
+
 #endif /* AVFORMAT_INTERNAL_H */
diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c
index 9801fc9..a56bf44 100644
--- a/libavformat/rtsp.c
+++ b/libavformat/rtsp.c
@@ -53,19 +53,6 @@ int rtsp_default_protocols = (1 << RTSP_LOWER_TRANSPORT_UDP);
 #define READ_PACKET_TIMEOUT_S 10
 #define MAX_TIMEOUTS READ_PACKET_TIMEOUT_S * 1000 / SELECT_TIMEOUT_MS
 
-#define SPACE_CHARS " \t\r\n"
-/* we use memchr() instead of strchr() here because strchr() will return
- * the terminating '\0' of SPACE_CHARS instead of NULL if c is '\0'. */
-#define redir_isspace(c) memchr(SPACE_CHARS, c, 4)
-static void skip_spaces(const char **pp)
-{
-    const char *p;
-    p = *pp;
-    while (redir_isspace(*p))
-        p++;
-    *pp = p;
-}
-
 static void get_word_until_chars(char *buf, int buf_size,
                                  const char *sep, const char **pp)
 {
-- 
1.7.0.4

From 272e3a256fcb93a3d77250335d6794d08fd35000 Mon Sep 17 00:00:00 2001
From: Josh Allmann <[email protected]>
Date: Sun, 20 Jun 2010 12:25:59 -0700
Subject: [PATCH 29/32] Decouple MPEG-4 and AAC specific parts from rtsp.c.

---
 libavformat/Makefile       |    1 +
 libavformat/rtpdec.c       |    8 +--
 libavformat/rtpdec_mpeg4.c |  123 ++++++++++++++++++++++++++++++++++++++++++++
 libavformat/rtpdec_mpeg4.h |   38 ++++++++++++++
 libavformat/rtsp.c         |   58 ---------------------
 5 files changed, 165 insertions(+), 63 deletions(-)
 create mode 100644 libavformat/rtpdec_mpeg4.c
 create mode 100644 libavformat/rtpdec_mpeg4.h

diff --git a/libavformat/Makefile b/libavformat/Makefile
index 156cd14..2977673 100644
--- a/libavformat/Makefile
+++ b/libavformat/Makefile
@@ -228,6 +228,7 @@ OBJS-$(CONFIG_SDP_DEMUXER)               += rtsp.o        \
                                             rtpdec_asf.o  \
                                             rtpdec_h263.o \
                                             rtpdec_h264.o \
+                                            rtpdec_mpeg4.o \
                                             rtpdec_xiph.o
 OBJS-$(CONFIG_SEGAFILM_DEMUXER)          += segafilm.o
 OBJS-$(CONFIG_SHORTEN_DEMUXER)           += raw.o id3v2.o
diff --git a/libavformat/rtpdec.c b/libavformat/rtpdec.c
index 0d2df59..0b316e3 100644
--- a/libavformat/rtpdec.c
+++ b/libavformat/rtpdec.c
@@ -34,6 +34,7 @@
 #include "rtpdec_asf.h"
 #include "rtpdec_h263.h"
 #include "rtpdec_h264.h"
+#include "rtpdec_mpeg4.h"
 #include "rtpdec_xiph.h"
 
 //#define DEBUG
@@ -50,9 +51,6 @@
 /* statistics functions */
 RTPDynamicProtocolHandler *RTPFirstDynamicPayloadHandler= NULL;
 
-static RTPDynamicProtocolHandler mp4v_es_handler= {"MP4V-ES", AVMEDIA_TYPE_VIDEO, CODEC_ID_MPEG4};
-static RTPDynamicProtocolHandler mpeg4_generic_handler= {"mpeg4-generic", AVMEDIA_TYPE_AUDIO, CODEC_ID_AAC};
-
 void ff_register_dynamic_payload_handler(RTPDynamicProtocolHandler *handler)
 {
     handler->next= RTPFirstDynamicPayloadHandler;
@@ -61,8 +59,8 @@ void ff_register_dynamic_payload_handler(RTPDynamicProtocolHandler *handler)
 
 void av_register_rtp_dynamic_payload_handlers(void)
 {
-    ff_register_dynamic_payload_handler(&mp4v_es_handler);
-    ff_register_dynamic_payload_handler(&mpeg4_generic_handler);
+    ff_register_dynamic_payload_handler(&ff_mp4v_es_dynamic_handler);
+    ff_register_dynamic_payload_handler(&ff_mpeg4_generic_dynamic_handler);
     ff_register_dynamic_payload_handler(&ff_amr_nb_dynamic_handler);
     ff_register_dynamic_payload_handler(&ff_amr_wb_dynamic_handler);
     ff_register_dynamic_payload_handler(&ff_h263_1998_dynamic_handler);
diff --git a/libavformat/rtpdec_mpeg4.c b/libavformat/rtpdec_mpeg4.c
new file mode 100644
index 0000000..83a74c6
--- /dev/null
+++ b/libavformat/rtpdec_mpeg4.c
@@ -0,0 +1,123 @@
+/**
+ * Common code for the RTP depacketization of MPEG formats.
+ * Copyright (c) 2010 the FFmpeg developers
+ *
+ * This file is part of FFmpeg.
+ *
+ * FFmpeg is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * FFmpeg is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with FFmpeg; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+/**
+ * @file
+ * @brief MPEG4 / RTP Code
+ * @author the FFmpeg developers
+ */
+
+#include "rtpdec_mpeg4.h"
+#include "internal.h"
+#include "libavutil/avstring.h"
+
+/* return the length and optionally the data */
+static int hex_to_data(uint8_t *data, const char *p)
+{
+    int c, len, v;
+
+    len = 0;
+    v = 1;
+    for (;;) {
+        skip_spaces(&p);
+        if (*p == '\0')
+            break;
+        c = toupper((unsigned char) *p++);
+        if (c >= '0' && c <= '9')
+            c = c - '0';
+        else if (c >= 'A' && c <= 'F')
+            c = c - 'A' + 10;
+        else
+            break;
+        v = (v << 4) | c;
+        if (v & 0x100) {
+            if (data)
+                data[len] = v;
+            len++;
+            v = 1;
+        }
+    }
+    return len;
+}
+
+static int parse_fmtp_config(AVCodecContext * codec, char *value)
+{
+    /* decode the hexa encoded parameter */
+    int len = hex_to_data(NULL, value);
+    if (codec->extradata)
+        av_free(codec->extradata);
+    codec->extradata = av_mallocz(len + FF_INPUT_BUFFER_PADDING_SIZE);
+    if (!codec->extradata)
+        return AVERROR(ENOMEM);
+    codec->extradata_size = len;
+    hex_to_data(codec->extradata, value);
+    return 0;
+}
+
+static int parse_sdp_line(AVFormatContext *s, int st_index,
+                                PayloadContext *data, const char *line)
+{
+    const char *p;
+    char value[4096], attr[25];
+    int res = 0;
+    AVCodecContext* codec = s->streams[st_index]->codec;
+
+    if (av_strstart(line, "fmtp:", &p)) {
+        // remove protocol identifier
+        while (*p && *p == ' ') p++; // strip spaces
+        while (*p && *p != ' ') p++; // eat protocol identifier
+        while (*p && *p == ' ') p++; // strip trailing spaces
+
+        while (ff_rtsp_next_attr_and_value(&p,
+                                           attr, sizeof(attr),
+                                           value, sizeof(value))) {
+            if (!strcmp(attr, "config")) {
+                res = parse_fmtp_config(codec, value);
+
+                if (res < 0)
+                    return res;
+            }
+        }
+    }
+
+    return 0;
+
+}
+
+RTPDynamicProtocolHandler ff_mp4v_es_dynamic_handler = {
+    .enc_name           = "MP4V-ES",
+    .codec_type         = AVMEDIA_TYPE_VIDEO,
+    .codec_id           = CODEC_ID_MPEG4,
+    .parse_sdp_a_line   = parse_sdp_line,
+    .open               = NULL,
+    .close              = NULL,
+    .parse_packet       = NULL
+};
+
+RTPDynamicProtocolHandler ff_mpeg4_generic_dynamic_handler = {
+    .enc_name           = "mpeg4-generic",
+    .codec_type         = AVMEDIA_TYPE_AUDIO,
+    .codec_id           = CODEC_ID_AAC,
+    .parse_sdp_a_line   = parse_sdp_line,
+    .open               = NULL,
+    .close              = NULL,
+    .parse_packet       = NULL
+};
diff --git a/libavformat/rtpdec_mpeg4.h b/libavformat/rtpdec_mpeg4.h
new file mode 100644
index 0000000..d67d440
--- /dev/null
+++ b/libavformat/rtpdec_mpeg4.h
@@ -0,0 +1,38 @@
+/**
+ * Common code for the RTP depacketization of MPEG formats.
+ * Copyright (c) 2010 the FFmpeg developers
+ *
+ * This file is part of FFmpeg.
+ *
+ * FFmpeg is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * FFmpeg is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with FFmpeg; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+#ifndef AVFORMAT_RTPDEC_MPEG4_H
+#define AVFORMAT_RTPDEC_MPEG4_H
+
+#include "rtpdec.h"
+
+/**
+ * MPEG-4 Video RTP callbacks.
+ */
+extern RTPDynamicProtocolHandler ff_mp4v_es_dynamic_handler;
+
+/**
+ * AAC RTP callbacks.
+ */
+extern RTPDynamicProtocolHandler ff_mpeg4_generic_dynamic_handler;
+
+#endif /* AVFORMAT_RTPDEC_MPEG4_H */
+
diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c
index a56bf44..ee77e9c 100644
--- a/libavformat/rtsp.c
+++ b/libavformat/rtsp.c
@@ -158,59 +158,6 @@ static int sdp_parse_rtpmap(AVFormatContext *s,
     return 0;
 }
 
-/* return the length and optionally the data */
-static int hex_to_data(uint8_t *data, const char *p)
-{
-    int c, len, v;
-
-    len = 0;
-    v = 1;
-    for (;;) {
-        skip_spaces(&p);
-        if (*p == '\0')
-            break;
-        c = toupper((unsigned char) *p++);
-        if (c >= '0' && c <= '9')
-            c = c - '0';
-        else if (c >= 'A' && c <= 'F')
-            c = c - 'A' + 10;
-        else
-            break;
-        v = (v << 4) | c;
-        if (v & 0x100) {
-            if (data)
-                data[len] = v;
-            len++;
-            v = 1;
-        }
-    }
-    return len;
-}
-
-static void sdp_parse_fmtp_config(AVCodecContext * codec, void *ctx,
-                                  char *attr, char *value)
-{
-    switch (codec->codec_id) {
-    case CODEC_ID_MPEG4:
-    case CODEC_ID_AAC:
-        if (!strcmp(attr, "config")) {
-            /* decode the hexa encoded parameter */
-            int len = hex_to_data(NULL, value);
-            if (codec->extradata)
-                av_free(codec->extradata);
-            codec->extradata = av_mallocz(len + FF_INPUT_BUFFER_PADDING_SIZE);
-            if (!codec->extradata)
-                return;
-            codec->extradata_size = len;
-            hex_to_data(codec->extradata, value);
-        }
-        break;
-    default:
-        break;
-    }
-    return;
-}
-
 typedef struct {
     const char *str;
     uint16_t    type;
@@ -263,16 +210,11 @@ static void sdp_parse_fmtp(AVStream *st, const char *p)
     char value[4096];
     int i;
     RTSPStream *rtsp_st = st->priv_data;
-    AVCodecContext *codec = st->codec;
     RTPPayloadData *rtp_payload_data = &rtsp_st->rtp_payload_data;
 
     /* loop on each attribute */
     while (ff_rtsp_next_attr_and_value(&p, attr, sizeof(attr),
                                        value, sizeof(value))) {
-        /* grab the codec extra_data from the config parameter of the fmtp
-         * line */
-        sdp_parse_fmtp_config(codec, rtsp_st->dynamic_protocol_context,
-                              attr, value);
         /* Looking for a known attribute */
         for (i = 0; attr_names[i].str; ++i) {
             if (!strcasecmp(attr, attr_names[i].str)) {
-- 
1.7.0.4

_______________________________________________
FFmpeg-soc mailing list
[email protected]
https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-soc

Reply via email to