[FFmpeg-devel] [PATCH v3 1/2] avformat/rtsp: fix infinite loop with udp transport

[Martin Storsjö]

Hi,

On Thu, 1 Oct 2020, Andriy Gelman wrote:

> On Wed, 30. Sep 12:41, Martin Storsjö wrote:
>> Hi,
>> 
>> On Sun, 27 Sep 2020, Zhao Zhili wrote:
>> 
>> > Fix #8840.
>> > 
>> > Steps to reproduce:
>> > 1. sender:
>> > ./ffmpeg -i test.mp4 -c copy -f rtsp -rtsp_transport udp  rtsp://localhost:12345/live.sdp
>> > 2. receiver:
>> > ./ffmpeg_g -y -rtsp_flags listen -timeout 100 -i rtsp://localhost:12345/live.sdp -c copy test.mp4
>> > ---
>> > v3: mention the ticket.
>> > 
>> > libavformat/rtsp.c    | 2 ++
>> > libavformat/rtsp.h    | 1 +
>> > libavformat/rtspdec.c | 2 +-
>> > 3 files changed, 4 insertions(+), 1 deletion(-)
>> > 
>> > diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c
>> > index 5d8491b74b..597413803f 100644
>> > --- a/libavformat/rtsp.c
>> > +++ b/libavformat/rtsp.c
>> > @@ -2051,6 +2051,8 @@ static int udp_read_packet(AVFormatContext *s, RTSPStream **prtsp_st,
>> >                 if ((ret = parse_rtsp_message(s)) < 0) {
>> >                     return ret;
>> >                 }
>> > +                if (rt->state == RTSP_STATE_TEARDOWN)
>> > +                    return AVERROR_EOF;
>> >             }
>> > #endif
>> >         } else if (n == 0 && ++timeout_cnt >= MAX_TIMEOUTS) {
>> > diff --git a/libavformat/rtsp.h b/libavformat/rtsp.h
>> > index 54a9a30c16..481cc0c3ce 100644
>> > --- a/libavformat/rtsp.h
>> > +++ b/libavformat/rtsp.h
>> > @@ -198,6 +198,7 @@ enum RTSPClientState {
>> >     RTSP_STATE_STREAMING, /**< initialized and sending/receiving data */
>> >     RTSP_STATE_PAUSED,  /**< initialized, but not receiving data */
>> >     RTSP_STATE_SEEKING, /**< initialized, requesting a seek */
>> > +    RTSP_STATE_TEARDOWN,/**< initialized, in teardown state */
>> > };
>> > 
>> > /**
>> > diff --git a/libavformat/rtspdec.c b/libavformat/rtspdec.c
>> > index dfa29913bf..ec786a469a 100644
>> > --- a/libavformat/rtspdec.c
>> > +++ b/libavformat/rtspdec.c
>> > @@ -494,7 +494,7 @@ int ff_rtsp_parse_streaming_commands(AVFormatContext *s)
>> >                               "Public: ANNOUNCE, PAUSE, SETUP, TEARDOWN, "
>> >                               "RECORD\r\n", request.seq);
>> >     } else if (methodcode == TEARDOWN) {
>> > -        rt->state = RTSP_STATE_IDLE;
>> > +        rt->state = RTSP_STATE_TEARDOWN;
>> >         ret       = rtsp_send_reply(s, RTSP_STATUS_OK, NULL , request.seq);
>> >     }
>> >     return ret;
>> > -- 
>> > 2.17.1
>
> Martin, thanks for looking over the patch.
>
>> 
>> I think this approach to fixing it, adding a new state, is a bit of
>> overkill. This usecase actually used to work originally, but I bisected it
>> and it broke in f6161fccf8c5720ceac1ed1df8ba60ff8fed69f5. So with that in
>> mind, it's pretty straightforward to fix it by retaining the original
>> behaviour from before that commit. I'll send an alternative patch that does
>> that.
>
> I made the suggestion to add TEARDOWN state because I thought it's safer than
> relying on the idle state, and it made the code more readable.

Yeah, it's not a bad idea per se - but adding more states is generally 
more risky (more codepaths that might need to take it into account, etc). 
And in this case, as it's a regression, it's easier to fix it as a 
specific fix for that breakage.

> Looking at your patch, I think it's a clean/elegant solution, and also looks
> good to me.

Great, thanks! So if Zhao also is ok with it, I'd push that one, and patch 
2/2 from Zhao's set.

// Martin