We should also add something to the parser that if it goes to READ_AHEAD_CRLF and encounters something else, that it then errors out or resyncs... instead of waiting forever.

Can you prepare a patch for that, and also a unit test that checks different weird and correct responses?

I agree with you. current parsing logic is little dangerous.

Anyhow I'll try to verify the other case based on RFC.


     Response    =     Status-Line         ; Section 7.1
                 *(    general-header      ; Section 5
                 |     response-header     ; Section 7.1.2
                 |     entity-header )     ; Section 8.1
                       CRLF
                       [ message-body ]    ; Section 4.3

headache..

Absolutely, this packet breaks standard.
This is consist of two packets according to standard
and the second packet is neither request nor response -> error case.

0000   52 54 53 50 2f 31 2e 30 20 32 30 30 20 4f 4b 0d  RTSP/1.0 200 OK.
0010   0a 43 53 65 71 3a 20 37 0d 0a 43 6f 6e 74 65 6e  .CSeq: 7..Conten
0020   74 2d 4c 65 6e 67 74 68 3a 20 30 0d 0a 43 6f 6e  t-Length: 0..Con
0030   74 65 6e 74 2d 54 79 70 65 3a 20 74 65 78 74 2f  tent-Type: text/
0040   70 61 72 61 6d 65 74 65 72 73 0d 0a 53 65 73 73  parameters..Sess
0050   69 6f 6e 3a 20 31 33 38 36 31 34 38 33 33 0d 0a  ion: 138614833..
0060   0d 0a 66 72 61 6d 65 72 61 74 65 3a 20 33 30 0d  ..framerate: 30.
0070   0a                                               .

Now we need to decide whether ignoring error(keep playback) or breaking the pipeline.
I also checked how live555 handling it and the result was
They keep playing RTSP stream and send "405 Method Not Allowed"
- actually they have also parsing problem, but playback never stop.

In my opinion, gstreamer also able to play this stream.

1. reset state

If recv timeout occurred but still not found ending characters(\r\n\r\n, \r\n\n, r\r...), reset state to initial.
Let build_next ignore it. (ETIMEOUT)

2. timeout case..
In this situation (RTSP message doesn't contain ending characters),
We have to wait until recv timeout which configured by upper layer (ex. rtspsrc) or RTSP response (timeout=)
If timeout is large, we waste time and subsequent request may be pended. 

Typically, RTSP response packed in one TCP packet so that socket buffer will store all response characters.  it's very high possibility.

So if we check remaining bytes in socket ahead of read, waiting time will be reduced.

and some ambiguous stuff..
- GST_RTSP_EPARSE handling
Dose GST_RTSP_EPARSE is critical error for RTSP *CLIENT*?
Now it's handled as receive_error so it stop playback.

Created attachment 314278 [details] [review]
patch2

Can you explain in the commit message (and/or comments) what exactly this does and why?

If I understand this correctly it will read remaining data from the socket if available, and depending on what it is reset the reading state. But how does this solve the problem?

Did u see my comment4?

Yes, but I didn't understand it :) How is all this related to the timeout? Also this should be explained in code or commit message too.

The parser should fail and reset itself, behaviour should be:
- Content-Length: 0 => except 0 bytes body and then \r\n\r\n
- See \r\n
- See other stuff that is not an \r followed by an \n => "Method not supported" and reset parser state
- See other stuff ("framerate: 30" in your case) with clean parser state that is not a RTSP message => Skip over it and keep clean parser state. Report "Method not supported" unless we already did that since the last valid RTSP message start we found (so not in your case)

Hum..our perspective seem to little difference. may be my view is narrow then your view.
Could you define range of "parser"?

Please consider that my brain doesn't support multi-core. it can process only one task at single time and now all interrupts are disabled T_T..
after finishing read_line() problem, let me handle the other case.

My lat patch focus on "read_line()" only.
and it's about parsing message which not contain READ_AHEAD_EOH characters.

Firstly, we need to remind this issue again.

1. parsing "framerate: 30\r\n"

*2*. read_ahead stay in READ_AHEAD_CRLF because there is no more characters to transit READ_AHEAD_EOH state or end of line. 
\r or \n -> can progress to next state (READ_AHEAD_CRLFCR or READ_AHEAD_EOH)
other char -> end of line.

at this time, all message was received but read_line() couldn't detect EOH so it expecting next character.

3. receive next RTSP message for new keep-alive request: "RTSP/1.0 200 OK\r\n"
read_line() thinks.. wow we got the 'T'. finally we made one line hehe..
build_next() parse with only 'R' character.

4. GST_RTSP_EPARSE returned and rtspsrc send event EOS -> stop the pipeline.

So I think the root cause is detecting End Of Header.
Current scheme \r\n(Request line or Response Status-Line)  + \r\n (Ending request, response header) is cool. it perfect meet to RFC 2326.

But another absolute condition is poll timeout when read socket buffer.
that's why I add below line:
 
      if (G_UNLIKELY (res != GST_RTSP_OK)) {
        /* could not reach final state but there is no more data. reset to initial state. */
        if (GST_RTSP_ETIMEOUT == res)
          conn->read_ahead = 0;

        return res;
      }

You may curios why zero instead of READ_AHEAD_EOH and why return error value(GST_RTSP_ETIMEOUT) not a GST_RTSP_OK?

yes, logically READ_AHEAD_EOH is better but build_next() is not thinks so.
It silently ignore error in read_line() and parse_line() but dose not allow error in first line (parse_response_status, parse_request_line)
and I don't want that sending EOS event from rtspsrc to pipeline. that's all.

You may know about the reason of g_socket_get_available_bytes() addition.

- My brain ready to load next scheduled task. -

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-base/issues/237.