[x265] x265: Questions and comments from an outsider

[Derek Buitenhuis]

On 9/13/2013 1:02 AM, Steve Borho wrote:
> Our coding style is 4-spaces, no tabs, unix eoln.  The main wiki page has a link to here:
> 
> https://bitbucket.org/multicoreware/x265/wiki/Coding
> 
> The short gist of that is we have defined a style using uncrustify, and mechanically enforce it, just not well enough at the moment.  My intent was to (at some point) hash out the contents of the uncrustify configuration file on IRC, to arrive at some middle ground that doesn't make anyone's eyes bleed, and then enforce that globally.
> 
> If you're only reviewing the patch submissions on the ML, it will look like the style is quite random because we have not been disciplined enough about *writing* code that matches the coding style, we've been making follow-on commits which enforce the style afterwards.  The code itself is fairly consistent wrt white-space.
> 
> For a while, I was forcing everyone to run uncrustify before sending patches, but that just added lots of white-space changes that made patches harder to review.  Our team is just getting used to submitting work as patches, now we need to get up to speed on hand-crafting those patches.

\o/


> The prefixing scheme is as follows:
> 
> m_ - class members
> s_  - class static members
> g_  - global variables
> b    - boolean
> 
> We don't use m_ prefixes for struct members, though I know we violate that in a few places.

OK.

> Yes, the HM was quite erratic about this, and we haven't added any clarity.
> 
> My personal policy on this is to use references where the caller is not allowed to pass a NULL, both because this makes their function crash and not yours and because it is sometimes slightly more efficient, but this has not been enforced anywhere other than the code I wrote.

[...]

> The (totally undocumented) policy on this is to use functions in headers when they are trivial and could be meaningfully inlined by the compiler, and their content is utterly predictable from the function name (no surprises).  If you find bad examples, point them out and I will fix them.

[...]

> These are easy problems, I might be one of the fastest vim slingers around.

You lost me at how this was related?

> Again, the code is fairly consistent but the patches have not been.  Our current style is if (x) and braces on their own lines (ANSI style, I think).

OK.

> Thanks for pointing this out; we can clean this up globally and use _X265_FOO_ for all the header guards.

Excellent.

> Yeah, I like your idea for making this more mechanically enforced.  So I would like to do this in three steps:
> 
> 1 - Review our uncrustify config on IRC/email
> 2 - Apply it globally to the source code (I've done this a few times in the past already)
> 3 - Write a commit hook to enforce the style, reject patches which violate the style

This seems entirely reasonable.


> Starting today, my patches and Deepthi's patches (the two people with push rights to the main repo) will also go through the mailing list.  As a general rule, we'll give 24 hours for a review, but if we get ACKs to the email or on IRC then we'll go ahead and push.

Reasonable, as well.

> This has been in the works for some time; I was trying to get through this big carnage of adding rudimentary rate control, rudimentary lookahead, and frame parallelism all at once.  Now that this is in, I think we can afford to slow the change cadence down and allow full review of all patches.

[...]

> Our contribute wiki has the following text in it:
> 
> 1) Make a meaningful commit message. The first line should be a quick summary. If necessary, add a blank line and then a longer explanation.
> 
> 2) Small coherent commits are better than one large commit, for many reasons a) they are easier to review b) they are easier to backout, should it be necessary c) they make the revision history (particularly annotations) more valuable
> 
> 3) Separate white-space changes from logic changes. Commits should never do both

I must have missed that; I apologize.

> But in general these guidelines will only be effective if patches are rejected because of them; which I will start doing.
> 
> For those with English difficulties, I often clean up commit messages when I queue them.

OK.
> The HM code base used new/delete, malloc/free, and xMalloc/xFree macros where memory alignment was explicitly required.
> 
> What we've done so far (for the most part) has been to replace xMalloc/xFree with X265_MALLOC/x265_FREE which have more portable implementations.
> 
> Memory allocations are something that is on my short-term radar.  We need to consolidate the vast numbers of small mallocs that the HM classes perform into one alloc per object (both for locality and for memory xfer properties).  When this happens we should be able to get this down to new/delete and X265_MALLOC/X265_FREE.

Why are there any new uses of new/delete being added at all?

> I don't think we use a realloc currently, but when we do we'll be certain to make a portable X265_REALLOC

This was specifically given as a reason for use of free/malloc
when I reviewed this patch:

http://mailman.videolan.org/pipermail/x265-devel/2013-September/000714.html

>     Reimplementations of STL and Related Identity Crises
>     ----------------------------------------------------
> 
>     Why are you reimplementing STL classes like std::list? I could understand if it was in C,
>     so as to aid in the promised future port to C, but it's being done in C++, using new and
>     delete, etc. This makes no sense. There's no reason to do this other than some misguided
>     Not In House syndrome or obsession with the resulting binary size. So, uh, what?
> 
> STL implementations between different compilers (even between different compiler versions) cause serious headaches because it is half-header and half-lib.  Since a lot of users will use x265 as a static library, it behooves us to avoid this pain.

If they differ, then either these compilers are not spec-compliant at all,
or you're using them in undefined ways. I do think if you choose to
re-implement the wheel, that:

1) An actual example of the differences between STL implementations of the
   functionality being reimplemented should be given in the commit message.

2) They should be segregated to their own subfolder or lib.

> The wiki part, at least, is easily addressed.  The contribute page now links to https://bitbucket.org/multicoreware/x265/wiki/IRC
> I'll have our engineers add themselves to that list.  The wiki is open, so other interested developers should also add themselves.

OK.

> From the project's start we've been using a Skype group chat to coordinate; I'm pushing this into IRC, but there is some predictable reluctance (shyness) that will take some time to be overcome.

Thanks. It's quiet frustrating to have all these decisions / coordination
happen being closed doors.


> As soon as we have assembly for a given primitive, the intrinsic version(s) will be deleted.  It can be done in the same patch as far as I'm concerned.

OK.

> After returning from VDD I told the team that we are moving towards exclusive ASM development.  But since we had no experience within our team with YASM or x264's asm environment, there has been a ramp-up period.  I expect to see our first ASM patches to appear in the next couple days.  Needless to say we will appreciate all help on this front.

OK. Using x86inc.asm should ease this a lot -- it's very easy to use
and abstracts away most of what makes writing assembly painful.

Thanks for the update.

> Short term, you might see us port some vector class primitives to intrinsic primitives in order to remove the vector class library; and there might be an occasional optimization made to a primitive; but the direction is firmly towards ASM development.  I would love to get rid of this "sum of all compiler bugs" pile of code.

Wasn't the vector class stuff imported from somewhere else? This is
probably off-topic.

> I can see how you can get this impression from an external view.  I think a more accurate (or more informed) description would be that the initial work was under very tight time constraints, we had limited initial encoder experience for many team members, and the initial goals that we were obligated to meet were performance related.  Expedience forced us into a lot of early decisions (starting with the HM, using compiler intrinsics, etc).
> 
> We added ABR first because it uses simple frame-level QP adjustment and could be added at the same time as we were making many other substantial changes, and I hoped it would easier to debug than CRF at this stage.  Crawling before walking.  I think we're all a bit spoiled by x264's current state.  No one will mistake x265 for x264 today, but I hope we have at least given it a substantial jump start.
> 
> I see encoder programming as a kind of extreme sport, you guys are hard core.  And x265 won't be grow to become dominant without the long-term "cathedral" style obsession over the code base that only open-source development can provide.  I'm hoping that this messy birth phase of the project is over and we can now transition to the maturation and continual improvement phase (extending the metaphor, these adolescent stages are probably going to be bumpy for a while).  We absolutely need a thriving community for the project to live up to its name, I completely understand that.
> 
> And I would love feedback on the relative priority of items in the TODO list.  Tell us which parts you want us to work on next, I have a *lot* of flexibility in that regard.  Also, tell us what is missing (or add it yourself, the wiki is open).

The "big" one is proper ratecontrol, IMO. This is by far the most important
one, I believe. It makes or breaks an encoding library (look at how
terrible DivX's ratecontrol (or at least its usage of MC's lib) is...).

> I've seen it happen myself as well, and we at MCW have certainly dug ourselves a hole with our initial involvement with this community.  All I can do is try to live up to my word and build trust.  I am in this, personally, for the long haul.

Good to know.

> And I take the fact that you've taken the time to come rant at us as a good thing.

Hah, OK.

- Derek