[FFmpeg-soc] [PATCH] decouple mpeg4/aac from rtsp

[Josh Allmann]

Hi,

On 21 June 2010 02:53, Martin Storsjö <martin at 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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Move-skip_spaces-to-libavformat-internal.h.patch
Type: text/x-patch
Size: 1974 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-soc/attachments/20100621/6ddb9106/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0029-Decouple-MPEG-4-and-AAC-specific-parts-from-rtsp.c.patch
Type: text/x-patch
Size: 10519 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-soc/attachments/20100621/6ddb9106/attachment-0001.bin>