rtsp: Spin in a separate function the rtsp message parsing

Message ID 20170314164445.14773-1-lu_zero@gentoo.org
State New
Headers show

Commit Message

Luca Barbato March 14, 2017, 4:44 p.m.
Makes easier manage the polling function pending the
threading support.
---

 libavformat/rtsp.c | 57 ++++++++++++++++++++++++++++++------------------------
 1 file changed, 32 insertions(+), 25 deletions(-)

--
2.9.2

Comments

Vittorio Giovara March 14, 2017, 7:24 p.m. | #1
On Tue, Mar 14, 2017 at 12:44 PM, Luca Barbato <lu_zero@gentoo.org> wrote:
> Makes easier manage the polling function pending the
> threading support.
> ---

rtsp: Move message parsing to a separate function

Makes it easier to handle the polling function before we implement
full threading support.
Martin Storsjö March 16, 2017, 9:27 a.m. | #2
On Tue, 14 Mar 2017, Luca Barbato wrote:

> Makes easier manage the polling function pending the
> threading support.
> ---
>
> libavformat/rtsp.c | 57 ++++++++++++++++++++++++++++++------------------------
> 1 file changed, 32 insertions(+), 25 deletions(-)
>
> diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c
> index 9839aba..226b46a 100644
> --- a/libavformat/rtsp.c
> +++ b/libavformat/rtsp.c
> @@ -1933,12 +1933,39 @@ static void *udp_read_loop(void *arg)
>     return NULL;
> }
>
> +static int parse_rtsp_message(AVFormatContext *s)
> +{
> +    RTSPState *rt = s->priv_data;
> +    int ret;
> +
> +    if (rt->rtsp_flags & RTSP_FLAG_LISTEN) {
> +        if (rt->state == RTSP_STATE_STREAMING) {
> +            if (!ff_rtsp_parse_streaming_commands(s))
> +                return AVERROR_EOF;
> +            else
> +                av_log(s, AV_LOG_WARNING,
> +                       "Unable to answer to TEARDOWN\n");
> +        } else
> +            return 0;
> +    } else {
> +        RTSPMessageHeader reply;
> +        ret = ff_rtsp_read_reply(s, &reply, NULL, 0, NULL);
> +        if (ret < 0)
> +            return ret;
> +        /* XXX: parse message */
> +        if (rt->state != RTSP_STATE_STREAMING)
> +            return 0;
> +    }
> +
> +    return 0;
> +}
> +
> static int udp_read_packet(AVFormatContext *s, RTSPStream **prtsp_st,
>                            uint8_t *buf, int buf_size, int64_t wait_end)
> {
>     RTSPState *rt = s->priv_data;
>     RTSPStream *rtsp_st;
> -    int n, i, ret, tcp_fd, timeout_cnt = 0;
> +    int n, i, ret, timeout_cnt = 0;
>     struct pollfd *p = rt->p;
>     int *fds = NULL, fdsnum, fdsidx;
>
> @@ -1952,11 +1979,8 @@ static int udp_read_packet(AVFormatContext *s, RTSPStream **prtsp_st,
>             return AVERROR(ENOMEM);
>
>         if (rt->rtsp_hd) {
> -            tcp_fd = ffurl_get_file_handle(rt->rtsp_hd);
> -            p[rt->max_p].fd = tcp_fd;
> +            p[rt->max_p].fd = ffurl_get_file_handle(rt->rtsp_hd);
>             p[rt->max_p++].events = POLLIN;
> -        } else {
> -            tcp_fd = -1;
>         }
>         for (i = 0; i < rt->nb_rtsp_streams; i++) {
>             rtsp_st = rt->rtsp_streams[i];
> @@ -1987,7 +2011,7 @@ static int udp_read_packet(AVFormatContext *s, RTSPStream **prtsp_st,
>             return AVERROR(EAGAIN);
>         n = poll(p, rt->max_p, POLL_TIMEOUT_MS);
>         if (n > 0) {
> -            int j = 1 - (tcp_fd == -1);
> +            int j = 1 - (!!rt->rtsp_hd);

Isn't this the wrong way around?

To keep the existing logic, you could make it "1 - !rt->rtsp_hd", but you 
could also make it just "j = !!rt->rtsp_hd", or "j = rt->rtsp_hd ? 1 : 0" 
to make it extra clear. Using !! to make it {0,1} here is pretty confusing 
already, since it's about picking a start index for the array...

The rest of it looks mostly ok.

// Martin
Luca Barbato March 16, 2017, 12:13 p.m. | #3
On 16/03/2017 10:27, Martin Storsjö wrote:
> On Tue, 14 Mar 2017, Luca Barbato wrote:
> 
>> Makes easier manage the polling function pending the
>> threading support.
>> ---
>>
>> libavformat/rtsp.c | 57
>> ++++++++++++++++++++++++++++++------------------------
>> 1 file changed, 32 insertions(+), 25 deletions(-)
>>
>> diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c
>> index 9839aba..226b46a 100644
>> --- a/libavformat/rtsp.c
>> +++ b/libavformat/rtsp.c
>> @@ -1933,12 +1933,39 @@ static void *udp_read_loop(void *arg)
>>     return NULL;
>> }
>>
>> +static int parse_rtsp_message(AVFormatContext *s)
>> +{
>> +    RTSPState *rt = s->priv_data;
>> +    int ret;
>> +
>> +    if (rt->rtsp_flags & RTSP_FLAG_LISTEN) {
>> +        if (rt->state == RTSP_STATE_STREAMING) {
>> +            if (!ff_rtsp_parse_streaming_commands(s))
>> +                return AVERROR_EOF;
>> +            else
>> +                av_log(s, AV_LOG_WARNING,
>> +                       "Unable to answer to TEARDOWN\n");
>> +        } else
>> +            return 0;
>> +    } else {
>> +        RTSPMessageHeader reply;
>> +        ret = ff_rtsp_read_reply(s, &reply, NULL, 0, NULL);
>> +        if (ret < 0)
>> +            return ret;
>> +        /* XXX: parse message */
>> +        if (rt->state != RTSP_STATE_STREAMING)
>> +            return 0;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> static int udp_read_packet(AVFormatContext *s, RTSPStream **prtsp_st,
>>                            uint8_t *buf, int buf_size, int64_t wait_end)
>> {
>>     RTSPState *rt = s->priv_data;
>>     RTSPStream *rtsp_st;
>> -    int n, i, ret, tcp_fd, timeout_cnt = 0;
>> +    int n, i, ret, timeout_cnt = 0;
>>     struct pollfd *p = rt->p;
>>     int *fds = NULL, fdsnum, fdsidx;
>>
>> @@ -1952,11 +1979,8 @@ static int udp_read_packet(AVFormatContext *s,
>> RTSPStream **prtsp_st,
>>             return AVERROR(ENOMEM);
>>
>>         if (rt->rtsp_hd) {
>> -            tcp_fd = ffurl_get_file_handle(rt->rtsp_hd);
>> -            p[rt->max_p].fd = tcp_fd;
>> +            p[rt->max_p].fd = ffurl_get_file_handle(rt->rtsp_hd);
>>             p[rt->max_p++].events = POLLIN;
>> -        } else {
>> -            tcp_fd = -1;
>>         }
>>         for (i = 0; i < rt->nb_rtsp_streams; i++) {
>>             rtsp_st = rt->rtsp_streams[i];
>> @@ -1987,7 +2011,7 @@ static int udp_read_packet(AVFormatContext *s,
>> RTSPStream **prtsp_st,
>>             return AVERROR(EAGAIN);
>>         n = poll(p, rt->max_p, POLL_TIMEOUT_MS);
>>         if (n > 0) {
>> -            int j = 1 - (tcp_fd == -1);
>> +            int j = 1 - (!!rt->rtsp_hd);
> 
> Isn't this the wrong way around?
> 
> To keep the existing logic, you could make it "1 - !rt->rtsp_hd", but
> you could also make it just "j = !!rt->rtsp_hd", or "j = rt->rtsp_hd ? 1
> : 0" to make it extra clear. Using !! to make it {0,1} here is pretty
> confusing already, since it's about picking a start index for the array...
> 
> The rest of it looks mostly ok.

Locally changed to be j = rt->rtsp_hd ? 1 : 0;

Thank you :)

Patch

diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c
index 9839aba..226b46a 100644
--- a/libavformat/rtsp.c
+++ b/libavformat/rtsp.c
@@ -1933,12 +1933,39 @@  static void *udp_read_loop(void *arg)
     return NULL;
 }

+static int parse_rtsp_message(AVFormatContext *s)
+{
+    RTSPState *rt = s->priv_data;
+    int ret;
+
+    if (rt->rtsp_flags & RTSP_FLAG_LISTEN) {
+        if (rt->state == RTSP_STATE_STREAMING) {
+            if (!ff_rtsp_parse_streaming_commands(s))
+                return AVERROR_EOF;
+            else
+                av_log(s, AV_LOG_WARNING,
+                       "Unable to answer to TEARDOWN\n");
+        } else
+            return 0;
+    } else {
+        RTSPMessageHeader reply;
+        ret = ff_rtsp_read_reply(s, &reply, NULL, 0, NULL);
+        if (ret < 0)
+            return ret;
+        /* XXX: parse message */
+        if (rt->state != RTSP_STATE_STREAMING)
+            return 0;
+    }
+
+    return 0;
+}
+
 static int udp_read_packet(AVFormatContext *s, RTSPStream **prtsp_st,
                            uint8_t *buf, int buf_size, int64_t wait_end)
 {
     RTSPState *rt = s->priv_data;
     RTSPStream *rtsp_st;
-    int n, i, ret, tcp_fd, timeout_cnt = 0;
+    int n, i, ret, timeout_cnt = 0;
     struct pollfd *p = rt->p;
     int *fds = NULL, fdsnum, fdsidx;

@@ -1952,11 +1979,8 @@  static int udp_read_packet(AVFormatContext *s, RTSPStream **prtsp_st,
             return AVERROR(ENOMEM);

         if (rt->rtsp_hd) {
-            tcp_fd = ffurl_get_file_handle(rt->rtsp_hd);
-            p[rt->max_p].fd = tcp_fd;
+            p[rt->max_p].fd = ffurl_get_file_handle(rt->rtsp_hd);
             p[rt->max_p++].events = POLLIN;
-        } else {
-            tcp_fd = -1;
         }
         for (i = 0; i < rt->nb_rtsp_streams; i++) {
             rtsp_st = rt->rtsp_streams[i];
@@ -1987,7 +2011,7 @@  static int udp_read_packet(AVFormatContext *s, RTSPStream **prtsp_st,
             return AVERROR(EAGAIN);
         n = poll(p, rt->max_p, POLL_TIMEOUT_MS);
         if (n > 0) {
-            int j = 1 - (tcp_fd == -1);
+            int j = 1 - (!!rt->rtsp_hd);
             timeout_cnt = 0;
             for (i = 0; i < rt->nb_rtsp_streams; i++) {
                 rtsp_st = rt->rtsp_streams[i];
@@ -2003,25 +2027,8 @@  static int udp_read_packet(AVFormatContext *s, RTSPStream **prtsp_st,
                 }
             }
 #if CONFIG_RTSP_DEMUXER
-            if (tcp_fd != -1 && p[0].revents & POLLIN) {
-                if (rt->rtsp_flags & RTSP_FLAG_LISTEN) {
-                    if (rt->state == RTSP_STATE_STREAMING) {
-                        if (!ff_rtsp_parse_streaming_commands(s))
-                            return AVERROR_EOF;
-                        else
-                            av_log(s, AV_LOG_WARNING,
-                                   "Unable to answer to TEARDOWN\n");
-                    } else
-                        return 0;
-                } else {
-                    RTSPMessageHeader reply;
-                    ret = ff_rtsp_read_reply(s, &reply, NULL, 0, NULL);
-                    if (ret < 0)
-                        return ret;
-                    /* XXX: parse message */
-                    if (rt->state != RTSP_STATE_STREAMING)
-                        return 0;
-                }
+            if (rt->rtsp_hd && p[0].revents & POLLIN) {
+                return parse_rtsp_message(s);
             }
 #endif
         } else if (n == 0 && ++timeout_cnt >= MAX_TIMEOUTS) {