[FFmpeg-soc] [PATCH] RTCP BYE

[Ronald S. Bultje]

Hi,

On Fri, Aug 27, 2010 at 4:57 PM, Josh Allmann <joshua.allmann at gmail.com> wrote:
> On 25 August 2010 02:54, Martin Storsjö <martin at martin.st> wrote:
>> #5 raises a few questions and concerns, though. Earlier rtp_parse_packet
>> returned <0 for errors or RTCP (no output packet), 0 for ok packet, 1 for
>> ok packet and more packets following. Now it suddenly returns RTCP_BYE for
>> such RTCP packets (and returns 0 for RTCP_SR packets). This also makes
>> rtsp_fetch_packet return normally (as if a packet was returned, even if
>> none was) if a RTCP packet was received, instead of retrying to get a
>> proper data packet.
>
> True. I thought about that, but I figured since the RTCP return types
> (200-204) aren't being used anywhere else, it was safe to propagate
> back up to the RTSP layer.
>
>> One way of fixing this would be to make rtcp_parse_packet return < 0 for
>> all cases, and return -RTCP_BYE for byes. The handling code would then
>> move up one step in rtsp_fetch_packet, to be within the "Either bad
>> packet, or a RTCP packet" block, after checking the first_rtcp_ntp_time
>> field.
>
> Yes, this method is a bit more consistent with the current code. Fixed.
>
>> Also, I think it would feel a bit more robust if nb_byes was reset in
>> rtsp_read_play instead of in seek.
>
> Indeed, that simplified things; the nb_byes = 0 initialization in
> ff_rtsp_connect is no longer needed.
>
>>> Autoexit does not trip under my tests, perhaps because with this
>>> patch, the EOF never makes it back to ffplay. Instead,
>>> av_read_frame_internal tries to return some (bogus?) final packets
>>> (utils.c:1119).
>>
>> That's due to the parser, which still may have outstanding data to return.
>> After returning this data, we go back to av_read_packet on the next call,
>> but then we still block on waiting for more packets in rtsp_fetch_packet.
>> You could add a
>>
>>    if (rt->nb_byes == rt->nb_rtsp_streams)
>>        return AVERROR_EOF;
>>
>> to the start of rtsp_fetch_packet, to take care of that case when the
>> function is called after returning eof once.
>
> That worked, thanks.
[..]
> @@ -1777,6 +1778,9 @@ static int rtsp_fetch_packet(AVFormatContext *s, AVPacket *pkt)
>      uint8_t buf[10 * RTP_MAX_PACKET_LENGTH];
>      RTSPStream *rtsp_st;
>
> +    if (rt->nb_byes == rt->nb_rtsp_streams)
> +            return AVERROR_EOF;
> +
>      /* get next frames from the same RTP packet */
>      if (rt->cur_transport_priv) {
>          if (rt->transport == RTSP_TRANSPORT_RDT) {

<nag> weird indent </nag>, but patch fine otherwise, feel free to
apply with this fixed. Did I say thanks, nice work yet?

Thanks! (Seriously, you've done great work!)

Ronald