[Icecast-dev] [PATCHES] Smartjog PatchDump

[Niv Sardi]

(answering both mails in one, as they call back each other)

Michael Smith <msmith at xiph.org> writes:
> Can you file bugs and attach the patches to them in our bugtracking
> system? http://trac.xiph.org/

Sorry, I really don't feel like creating 3x tickets right now =)

> Mail dumps of patches are pretty much guaranteed to not get merged.

Understandable. 

> From a very quick look at a couple of them: in amongst the actual
> changes, there are lots of formatting changes, and things like
> additions of "XXX: ..." comments that don't say anything helpful -
> these will need removing before the patches are really reviewable.

hum, I tried to keep those as separated as I could, but might be, it's
about 2 months of developement that is dumped here.

> A higher-level description of what you're attempting to accomplish
> with these patchsets would also be very helpful - but much more detail
> than "performance and reliability".

I actually dumped these patches at my last day working for SmartJog, but
they already have a new guy (Remi) to take over maintainership. The main
idea was to be able to continue working on IceCast while out of
contract. I still have a lot of vectors where I'd like to see the
IceCast code go:
* Add refbuff pools.
* Use libCURL for everything (drop httpp).
* Implement a better stats system (stats are actually a HUGE perf
bottleneck). We can do stats without locking.
* Merge connection.c and slave.c (it's not so far off now).

There are a few more patches needing review to add new formats, but
these need to be released later on. The first thing I tried to do was to
get get_buffer() to have some context stability when called from a
source or a slave (especially the first call), especially for formats
that need to parse a header there. Before the connection re-vamp, what
we got in the refbuf at first call was more sheer luck than anything.

Re-working the connection path, got me to break the way ShoutCast was
handled, I then re-implemented the support in a less hackery way.

A bit later down the track, I needed POST support for testing, that was
easily implemented (patch 14).

Then I got interested in improving the number of accept() per second. In
order to do so, I splited out the connection process in 2:
* accept() and push to a queue.
* process connection and pass it over once auth'd and validated (2 pass:
http validation, and logical validation)

right now, the process phase is timeouted and re-entrant for the first
pass only, the parts of the second one that can be slow (I'm not sure
there are) should get a liftup.

> On Fri, Jul 30, 2010 at 10:43 AM, Romain Beauxis <toots at rastageeks.org> wrote:
>> Le vendredi 30 juillet 2010 12:25:48, Michael Smith a écrit :
[…]

> New contributors would be very welcome.

Great !

>> If no one has time to review the patch, perhaps giving Niv Sardi a commit
>> access is the right answer ? Not necessarily for an immediate stable release,
>> but, hey, commiting them somewhere would help users testing, reporting, etc..
>
> Given the number of issues with the patches as they stand, directly
> giving access right now would seem counterproductive.

Agreed, more on that later.

> In Niv is
> interested in becoming part of the icecast community (rather than just
> dumping a pile of patches on us), then in the medium term that would
> be great.

As Romain said, SmartJog is interested in maintaining the patches (they
use IceCast daily), are truely opensource friendly, and will have a guy
(Remi) on the job. On my side, I'd love to see this work merged, and to
continue getting things in IceCast.

Can we cut things up a bit ?

the first patch is a direct feature patch, (not even originaly from me),
maybe that can go in ?
 [PATCH 01/31] Hack to support IPhone streaming

Then you can ignore this one, it's actually only needed much after.
[Thread connection]
 [PATCH 02/31] introduce thread_cond_init and refrase thread_cond_create.

This one is clean:
[Get rid of the {WARN,INFO,DEBUG,..}n macros]
 [PATCH 03/31] LOGGING add non arg-counted macros.

This part is the first cleanup:
[simplify header parsing, inspired by source.c code]
 [PATCH 04/31] UTIL, add find_eos_delim and use it to simplify util_read_header (pending con)
 [PATCH 05/31] connection: make process_request_queue use util_find_eos_delim
 [PATCH 06/31] connection.c: use util_find_eos_delim to simplify _handle_shoutcast_compatible

Can we look into getting those ones in ? it's a feature and a bit of cleanups

Cheers,
--
Niv Sardi