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

2020-10-01 Thread 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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

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

2020-09-30 Thread Andriy Gelman 
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.

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

-- 
Andriy
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

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

2020-09-30 Thread Martin Storsjö 

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


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.


// Martin

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

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

2020-09-27 Thread Zhao Zhili 
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

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".