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

[Martin Storsjö]

On Thu, 30 Dec 2010, Martin Storsj? wrote:

> 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.

Any opinions on this? Is it worthwhile to do all that refactoring and 
adding extra code for mapping enums to method names, just to avoid the 
occasional strcmp in performance insensitive code?

// Martin