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

[Ronald S. Bultje]

Hi Martin,

On Sun, Jan 31, 2010 at 6:59 AM, Martin Storsj? <martin at martin.st> wrote:
> Updated patch series attached. Except for rebasing it onto the latest
> version and adding the documentation mentioned above, I check the lower
> transport mask in 0009, since only UDP output is supported at the moment.

OK, let's go get it in.

1) is OK, I'll apply (hopefully).

For 2, 3 an following. Do we really need RECORDING? My impression is
that PLAYING implies is_output == 0 and RECORDING is the same, with
is_output == 1. In other words, we have duplicate info here. I
understand that we need is_output because we could be PAUSED, or
STOPPED. But RECORDING vs. PLAYING (although terminalogically ok)
seems to stem from miswording. I'd prefer a patch renaming PLAYING to
something else, or better, add a define that makes RECORDING a
duplicate of PLAYING if you really want to.

4-5) is OK, will apply after 2-3 get adapted.

6)

-    if (ret < 0) {
-        err = AVERROR_INVALIDDATA;
+    ret = rtsp_setup_input_streams(s);
+    if (ret) {
+        err = ret;
         goto fail;
     }

Should be:

err = rtsp_setup...
if (err)
    goto fail;

7) OK

8) see 10

9)

+    if (rt->is_output) {
+        /* Only UDP output is supported at the moment. */
+        lower_transport_mask &= 1 << RTSP_LOWER_TRANSPORT_UDP;
+        if (!lower_transport_mask) {
+            av_log(s, AV_LOG_ERROR, "Unsupported lower transport method\n");
+            err = AVERROR(EINVAL);
+            goto fail;
+        }
+    }

Needs more detail in the log msg. E.g. only UDP supported, output, etc.

-    ret = rtsp_setup_input_streams(s);
+    if (!rt->is_output)
+        ret = rtsp_setup_input_streams(s);
+    else
+        ret = rtsp_setup_output_streams(s);

cosmetic and functional changes combined.

10) - I'd prefer a separate file for now.

Are you going to maintain all this code? If so, I think now would be
an excellent time to start getting you SVN access. :-).

Ronald