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

[Martin Storsjö]

On Fri, 19 Feb 2010, Ronald S. Bultje wrote:

> 1, 3-6 applied.

Fantastic, thanks!

> For 2:
> 
> +                    av_write_trailer(rtpctx);
> +                    url_fclose(rtpctx->pb);
> +                    av_free(rtpctx->streams[0]);
> +                    av_free(rtpctx);
> [..]
> +    /* Remove the local codec, link to the original codec
> +     * context instead, to give the rtp muxer access to
> +     * codec parameters. */
> +    av_free(rtpctx->streams[0]->codec);
> +    rtpctx->streams[0]->codec = st->codec;
> 
> Is that right? I think this could lead to memleaks of other things
> potentially allocated in the AVCodecContext. In both cases, I'd
> recommend not calling av_free() directly on the AVStream, but having
> the default function take care of that, and just setting
> rtpctx->streams[0]->codec to NULL before free'ing the inside?

What default function would that be, for freeing an AVStream? E.g. in 
ffmpeg.c, lines 418-422, the streams are freed in the same way...

> Also, is
> it possible that there's extradata and should you free that in the
> second part of the quoted code?

Theoretically, perhaps, but the AVCodecContext was just allocated by the 
av_new_stream above and never touched.

> For 7:
> 
>   fail:
>      rtsp_close_streams(s);
>      url_close(rt->rtsp_hd);
> -    if (reply->status_code >=300 && reply->status_code < 400) {
> +    if (reply->status_code >=300 && reply->status_code < 400 && s->iformat) {
>          av_strlcpy(s->filename, reply->location, sizeof(s->filename));
>          av_log(s, AV_LOG_INFO, "Status %d: Redirecting to %s\n",
>                 reply->status_code,
> 
> (bottom) appears unrelated to the rest, could probably be a separate
> commit? (No need for a new patch, just yes/no is fine.)

Yeah, could probably be a separate commit as well.

// Martin