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

[Martin Storsjö]

Hi Ronald,

On Wed, 17 Feb 2010, Ronald S. Bultje wrote:

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

Sounds very reasonable. Any suggestion on a name for the combined 
playing/recording state? Streaming, running? Otherwise, a define is ok, 
too, but that's a bit messier.

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

Ah, of course.

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

Ok, will fix.

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

Sure - I've got ideas on how to split this file into rtsp.c, rtspdec.c, 
rtspenc.c relatively cleanly. It's easier to do the split once the muxer 
code is in place, though, so is it ok for you to first apply this, then do 
the split?

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

Yes, I'm ready to maintain it, and I do agree with the development policy 
etc. :-)

I'll get back with an updated patch series soon, in a hurry ATM.

// Martin