[FFmpeg-devel] [PATCH] rtsp: Store some parsed values straight to RTSPState

[Martin Storsjö]

On Wed, 29 Dec 2010, Ronald S. Bultje wrote:

> On Mon, Dec 27, 2010 at 4:04 AM, Martin Storsj? <martin at martin.st> wrote:
> > Hi,
> >
> > In the review of the patch for adding support for the Content-Base header
> > some weeks ago, Ronald pointed out that storing large strings (a URL) in
> > the RTSPMessageHeader struct isn't really optimal. (This struct is stored
> > on the stack in many places.)
> >
> > Attached is a patchset that changes ff_rtsp_parse_line to receive a
> > pointer to the RTSPState, together with the method name (since we might
> > want to handle some headers differently depending on which method it was a
> > reply to), and lastly changes the parsing of Content-Base to store the
> > result directly in RTSPState.
> >
> > This is a slight change in the architecture of the RTSP header parsing -
> > initially it never took any action, it simply parsed out the reply headers
> > into a struct that the caller inspected, who chose what to do with it.
> > With this in place, the parser itself can change state, too. (The http
> > auth parsing was an exception before, which also updated state directly.)
> >
> > With this in place, parsing of the RTP-Info header is much more
> > straightforward - if done without this, RTSPMessageHeader would grow a
> > lot, if storing all the urls of all streams.
> 
> It looks good to me. I'm not a huge lover of free strings, can we
> somehow change the const char *method and make it an enum? That way,
> we're not calling str(case)cmp(), but simply comparing integers, which
> may be slightly faster and also needs no NULL check (simply pass
> RTSP_METHOD_DONTCARE or so).

I agree that this would be simpler, but this would instead need changes to 
all callers of ff_rtsp_send_cmd, where we currently do 
ff_rtsp_send_cmd(..., "DESCRIBE", ...). This would need to be changed to 
add an enum parameter in addition to the current string, or change the 
string parameter into an enum with a mapping back to a string within 
ff_rtsp_send_cmd_with_content_async.

Do you or others think that this is worthwhile? I'm a bit hesitant since 
we need the method as a string in other places anyway.

// Martin