[x265] x265: Questions and comments from an outsider

[Steve Borho]

On Thu, Sep 12, 2013 at 6:03 AM, Derek Buitenhuis <
derek.buitenhuis at gmail.com> wrote:

> So rather than break up all these (very general) comments between N
> separate emails,
> inlined in patch reviews, I figured one canonical place to air them should
> be more
> appropriate and conducive to open discussion.
>
> In no particular order, some comments:
>
>
> Code Style (or Lack Thereof)
> ----------------------------
>
> Currently, x265 is a giant mess with no consistent code style, practices,
> or guidelines.
> Certainly nothing is enforced. Below is a list of issues to discuss
> related to this.
>
> 1) Indentation: How many spaces do you indent? This is inconsistent
> throughout the codebase,
>    patches regularily get pushed which randomly change it -- likely due to
> haphazardly copy
>    and pasted code. This should be set and enforced, no? Perhaps a push
> hook that LINTs the
>    patch? This is what Libav does.
>

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.


> 2) Variable & Function Names: Again, inconsistent. some members have a
> MS-style 'm_' prefix,
>    some have no prefix. Some have an 'n' prefix. Some have an 'x265_'
> prefix, which ideally
>    should only be used for public API function namespacing.
>

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.


> 3) References vs. Pointers: Self explanatory -- usage is inconsistent
> between const pointers,
>    references, pointers, etc.
>

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.


> 4) Function Code in Headers: Is this acceptable? Is there policy of when
> to do this? It's done
>    seemingly at random currently, and is very confusing/ugly.
>

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.


> 5) Formatting: Whitespace, code layout etc. Braces on ae line or next?
> When? How to format
>    an 'if' statement? if(x)? if (x)? if( x )? if ( x )? Stuff like that.
> VERY inconsistent.
>

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


> 6) Header Guards: Non-generic header guards are needed so as to not
> contaminate the namespace
>    or conflict with system headers. __LIST__ is a good example of a bad
> guard.
>

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


> Certainly these sorts of things need to be enforced to some extent within
> reason, or the codebase
> is only going to get even more bad/messy.
>


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


Reviews (or Some Are More Equal Than Others)
> --------------------------------------------
>
> Some developers do not have their code reviewed. Some do. This sort of
> thing won't go
> down so well with community contributors, I think.
>
> Also, developers mostly seem to send their code for review, but aside from
> Steve, nobody
> seems to review others' work.
>

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.

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.



> Commit Messages
> ---------------
>
> They're quite bad.
>
> I understand that English is not the native tongue of many of the
> developers, and I
> think that they would have an easier time if there was a clear and concise
> set of
> rules for creating commit messages defined somewhere for them to follow. A
> checklist,
> perhaps. It's been my experience that this would help non-native speakers,
> but that's
> only one man's hearsay.
>

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

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.


Memory Allocation/Reallocation/Freeing
> --------------------------------------
>
> It's a mess. A big one. The codebase is a tangled mess of inconsistently
> used malloc(),
> free(), new, delete, X265_MALLOC(), and X265_FREE(). X265_REALLOC() does
> not exist. So,
> which is it? Make up your minds! This will affect an future C ports.
>

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.

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


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.

Communication
> -------------
>
> Only Steve ever seems to be available on IRC, and since he travels a lot,
> it's very cumbersome
> to interact with the developers in real time. I don't even think the IRC
> channel is listed on
> the wiki page for contributing.
>

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.

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


Intrinsics
> ----------
>
> When will they die? When will new ones stop getting added? This may have
> been answered already,
> but I didn't hear/see anything.
>

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.

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.

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.


Priorities
> ----------
>
> This is by far the most concerning aspect for me, as an outsider. The
> priorities for x265 seem
> to be geared towards demos or a 'cash out'. Rather than making the encoder
> *good*, first priorities
> have gone to making it fast. This seems very backwards. Similarly, the
> most useless ratecontrol
> method is the only method being worked on, since MCW wants to demo HLS, as
> far as I can tell.
>
> Second to this, is that I don't completely trust that MCW will not either
> stop work after
> it demos / cashes out (or x265 is 'good enough'), or won't backseat
> important things for the
> sake of demos, etc.
>

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

I've seen Bad Things™ happen with corporations involved in FOSS time and
> time again, so you
> could say I'm once burned, twice shy.
>

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.

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

Regards,

--
Steve Borho
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/x265-devel/attachments/20130912/8798f26d/attachment-0001.html>