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

Wed Jan 20 11:36:28 CET 2010

Hi Ronald,

Thanks for the new review!

On Tue, 19 Jan 2010, Ronald S. Bultje wrote:

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

Ok, changed. However, this gives a warning:

libavformat/rtsp.c:1018: warning: passing argument 2 of 'url_write' 
discards qualifiers from pointer target type

(since url_write takes a non-const pointer)

If I add a cast to the url_write call, I get this warning instead:

libavformat/rtsp.c:1018: warning: cast discards qualifiers from pointer 
target type

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

Ok, renamed the functions.

> 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 () .. }).

Ok, restructured the if clauses.

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

It was used for the last part of the rtsp_open function in that version, 
for determining if rtsp_read_play should be called. I've restructured this 
in the latest version, though, so it isn't needed 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.

What about rtsp_connect? I guess "read_header" comes from the general 
demuxer naming scheme, and I feel it doesn't really fit into the output 
case. But choose anyone you like.

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

Ok, I managed to split it in this way, and it does indeed make the diffs 
smaller and nicer, requires much less reindenting and makes the individual 
functions shorter.

Updated patch series attached (the first patch of the previous series was 
already applied, so the numbers for the similar patches have decreased by 
1).

0001 has the renamings you asked for, and added the const qualifier.

0002 and 0003 are unmodified.

0004 and 0005 has the if/else clauses restructured as requested, which 
made the diff even smaller.

0006 splits out the input-specific part from rtsp_read_header.
0007 splits rtsp_read_header into rtsp_connect, with the input specific 
rtsp_read_play call left in rtsp_read_header
0008 adds the output specific function needed for rtsp_connect.
0009 hooks it up (and adds the no-redirect check at the end of 
rtsp_connect, which I'm not totally sure if it really is needed)
0010 adds the rest of the output specific functions and makes the muxer 
available to use.

// Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-internal-rtsp-functions-for-doing-requests-that-.patch
Type: text/x-diff
Size: 3256 bytes
Desc: 
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100120/ee94eb11/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Add-a-RTSPClientState-for-recording.patch
Type: text/x-diff
Size: 855 bytes
Desc: 
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100120/ee94eb11/attachment-0001.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-Add-a-flag-for-storing-the-direction-of-the-rtsp-ses.patch
Type: text/x-diff
Size: 752 bytes
Desc: 
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100120/ee94eb11/attachment-0002.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-Create-AVFormatContext-objects-as-private-transport-.patch
Type: text/x-diff
Size: 3612 bytes
Desc: 
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100120/ee94eb11/attachment-0003.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-Add-proper-mode-to-output-setup-requests.patch
Type: text/x-diff
Size: 1062 bytes
Desc: 
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100120/ee94eb11/attachment-0004.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0006-Split-out-the-input-specific-part-of-rtsp_read_heade.patch
Type: text/x-diff
Size: 3621 bytes
Desc: 
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100120/ee94eb11/attachment-0005.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0007-Split-rtsp_read_header-into-a-reusable-function-rtsp.patch
Type: text/x-diff
Size: 2038 bytes
Desc: 
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100120/ee94eb11/attachment-0006.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0008-Add-a-function-rtsp_setup_output_streams-for-announc.patch
Type: text/x-diff
Size: 2353 bytes
Desc: 
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100120/ee94eb11/attachment-0007.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0009-Enable-rtsp_connect-to-work-for-output-too.patch
Type: text/x-diff
Size: 1140 bytes
Desc: 
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100120/ee94eb11/attachment-0008.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0010-Add-an-RTSP-muxer.patch
Type: text/x-diff
Size: 7311 bytes
Desc: 
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100120/ee94eb11/attachment-0009.patch>