[FFmpeg-devel] [PATCH] RTSP muxer, round 2

[Ronald S. Bultje]

Hi,

On Mon, Jan 11, 2010 at 9:32 AM, Martin Storsj? <martin at martin.st> wrote:
> Here's the second round of the RTSP muxer. Now it uses chained muxers,
> so no changes to the RTP muxer was needed, and the number of patches is
> much smaller.
[..]
> +++ b/libavformat/rtsp.c
> @@ -926,9 +926,11 @@ static int rtsp_read_reply(AVFormatContext *s, RTSPMessageHeader *reply,
>      return 0;
>  }
>
> -static void rtsp_send_cmd_async(AVFormatContext *s,
> -                                const char *cmd, RTSPMessageHeader *reply,
> -                                unsigned char **content_ptr)
> +static void rtsp_send_cmd_async_content(AVFormatContext *s,
> +                                        const char *cmd, RTSPMessageHeader *reply,
> +                                        unsigned char **content_ptr,
> +                                        unsigned char *send_content,

const unsigned char *. Also, I'd call it send_cmd_with_content_async()
and send_cmd_with_content(), but that's just me. With these changes,
I'm OK with this one.

> Subject: [PATCH 03/10] Add a RTSPClientState for recording

is OK.

> Subject: [PATCH 04/10] Add a flag for storing the direction of the rtsp session

I guess if you feel this is best, go for it.

Patch 5 & 6 (example from patch 6):
> +        if (rt->is_output) {
> +            av_strlcat(transport, ";mode=receive", sizeof(transport));
> +        } else {
>          if (rt->server_type == RTSP_SERVER_REAL ||
>              rt->server_type == RTSP_SERVER_WMS)
>              av_strlcat(transport, ";mode=play", sizeof(transport));
> +        }

This recurs in several patches, but please put if () { .. } else if ()
.. rather than this (so no closing bracket, and no else { if () .. }).
Rest is OK.

Patch 8:
> @@ -1327,7 +1327,7 @@ static int rtsp_write_record(AVFormatContext *s)
>      return 0;
>  }
>
> -static int rtsp_read_header(AVFormatContext *s,
> +static int rtsp_open(AVFormatContext *s,
>                              AVFormatParameters *ap)
>  {
>      RTSPState *rt = s->priv_data;

The ap argument serves no function anymore.

> @@ -1506,6 +1506,14 @@ redirect:
>      return err;
>  }
>
> +static int rtsp_read_header(AVFormatContext *s,
> +                            AVFormatParameters *ap)
> +{
> +    RTSPState *rt = s->priv_data;
> +    rt->is_output = 0;
> +    return rtsp_open(s, ap);
> +}
> +
>  static int udp_read_packet(AVFormatContext *s, RTSPStream **prtsp_st,

rtsp_open() is also too generic a name. Why not keep it read_header(),
and add this read_header_input and read_header_output for your future
function? Or something else that actually describes what the function
does.

Patch 9 makes these functions way too big, I'd say split the functions
rather (isn't that read_header anyway?), e.g. instead of patch 8,
split the function in 2, the input-only part and the generic part, and
put the output only part in read_header_output (or so).

Ronald