On 20 June 2010 14:13, Ronald S. Bultje <rsbultje@gmail.com> wrote:

Hi,

On Sun, Jun 20, 2010 at 3:46 PM, Josh Allmann <joshua.allmann@gmail.com> wrote:

...

$subj

skip_spaces() is small enough that I think a static inline might make more sense (in lavf/internal.h).

Fixed. Can you also check and make sure that

the rtp*.c files do not have copies of this functions lying around? I thought this was copied 1-2 times...

ffserver has a copy, but I grepped for skip_spaces -- if it is named differently, I missed it. There is still quite a bit of boilerplate in the sdp/fmtp parsing code shared among rtp*, Martin added another todo for that on the wiki. Josh

Hi, On 21 June 2010 02:53, Martin Storsjö <martin@martin.st> 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 <joshua.allmann@gmail.com> 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

Hi, On Mon, Jun 21, 2010 at 5:28 PM, Luca Barbato <lu_zero@gentoo.org> wrote:

0029 not sure if hex_to_data might have other usages and thus should be put in the header as well.

I looked at that previously, and no, not yet. We'll move it to utils.c once others need it. Ronald