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

[Ronald S. Bultje]

Hi,

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.

Hint: I have a newborn, so likely not. :-p.

Ronald