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

[Ronald S. Bultje]

Hi,

On Fri, Feb 19, 2010 at 6:42 PM, Martin Storsj? <martin at martin.st> wrote:
> On Fri, 19 Feb 2010, Ronald S. Bultje wrote:
>> 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.

Hm, ok, I missed this. Allright then, patch OK, go test your SVN account. :-).

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

Let's go test your SVN account with this one also.

Patch 8 imo needs doxy comments for the functions since they are now
non-static. Could you add these?

I'll review the last patch (9) afterwards.

Ronald