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

[Martin Storsjö]

On Sat, 1 Jan 2011, Ronald S. Bultje wrote:

> On Sat, Jan 1, 2011 at 5:34 PM, Martin Storsj? <martin at martin.st> wrote:
> > 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?
> 
> If you feel it's a lot of work, just commit this, I'll see if I can
> bring up the energy to do it later.

Ok, applied.

// Martin