[x264-devel] x265: Questions and comments from an outsider

[Derek Buitenhuis]

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.

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.

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

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.

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.

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.

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.


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.


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.


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.


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?


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.


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.


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


</list>

I am sure I am missing some things, so any community members should add any I forgot.

- Derek