<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Sep 12, 2013 at 6:03 AM, Derek Buitenhuis <span dir="ltr"><<a href="mailto:derek.buitenhuis@gmail.com" target="_blank">derek.buitenhuis@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">So rather than break up all these (very general) comments between N separate emails,<br>
inlined in patch reviews, I figured one canonical place to air them should be more<br>
appropriate and conducive to open discussion.<br>
<br>
In no particular order, some comments:<br>
<br>
<br>
Code Style (or Lack Thereof)<br>
----------------------------<br>
<br>
Currently, x265 is a giant mess with no consistent code style, practices, or guidelines.<br>
Certainly nothing is enforced. Below is a list of issues to discuss related to this.<br>
<br>
1) Indentation: How many spaces do you indent? This is inconsistent throughout the codebase,<br>
patches regularily get pushed which randomly change it -- likely due to haphazardly copy<br>
and pasted code. This should be set and enforced, no? Perhaps a push hook that LINTs the<br>
patch? This is what Libav does.<br></blockquote><div><br></div><div>Our coding style is 4-spaces, no tabs, unix eoln. The main wiki page has a link to here:</div><div><br></div><div><a href="https://bitbucket.org/multicoreware/x265/wiki/Coding">https://bitbucket.org/multicoreware/x265/wiki/Coding</a><br>
</div><div><br></div><div>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.</div>
<div><br></div><div>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.</div>
<div><br></div><div>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
2) Variable & Function Names: Again, inconsistent. some members have a MS-style 'm_' prefix,<br>
some have no prefix. Some have an 'n' prefix. Some have an 'x265_' prefix, which ideally<br>
should only be used for public API function namespacing.<br></blockquote><div><br></div><div>The prefixing scheme is as follows:</div><div><br></div><div>m_ - class members</div><div>s_ - class static members</div><div>
g_ - global variables</div><div>b - boolean</div><div><br></div><div>We don't use m_ prefixes for struct members, though I know we violate that in a few places.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
3) References vs. Pointers: Self explanatory -- usage is inconsistent between const pointers,<br>
references, pointers, etc.<br></blockquote><div><br></div><div>Yes, the HM was quite erratic about this, and we haven't added any clarity.</div><div><br></div><div>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
4) Function Code in Headers: Is this acceptable? Is there policy of when to do this? It's done<br>
seemingly at random currently, and is very confusing/ugly.<br></blockquote><div><br></div><div>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
5) Formatting: Whitespace, code layout etc. Braces on ae line or next? When? How to format<br>
an 'if' statement? if(x)? if (x)? if( x )? if ( x )? Stuff like that. VERY inconsistent.<br></blockquote><div><br></div><div>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).</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
6) Header Guards: Non-generic header guards are needed so as to not contaminate the namespace<br>
or conflict with system headers. __LIST__ is a good example of a bad guard.<br></blockquote><div><br></div><div>Thanks for pointing this out; we can clean this up globally and use _X265_FOO_ for all the header guards.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Certainly these sorts of things need to be enforced to some extent within reason, or the codebase<br>
is only going to get even more bad/messy.<br></blockquote><div><br></div><div><br></div><div>Yeah, I like your idea for making this more mechanically enforced. So I would like to do this in three steps:</div><div><br></div>
<div>1 - Review our uncrustify config on IRC/email</div><div>2 - Apply it globally to the source code (I've done this a few times in the past already)</div><div>3 - Write a commit hook to enforce the style, reject patches which violate the style</div>
<div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Reviews (or Some Are More Equal Than Others)<br>
--------------------------------------------<br>
<br>
Some developers do not have their code reviewed. Some do. This sort of thing won't go<br>
down so well with community contributors, I think.<br>
<br>
Also, developers mostly seem to send their code for review, but aside from Steve, nobody<br>
seems to review others' work.<br></blockquote><div><br></div><div>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.<br>
</div><div><br></div><div>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.</div>
<div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Commit Messages<br>
---------------<br>
<br>
They're quite bad.<br>
<br>
I understand that English is not the native tongue of many of the developers, and I<br>
think that they would have an easier time if there was a clear and concise set of<br>
rules for creating commit messages defined somewhere for them to follow. A checklist,<br>
perhaps. It's been my experience that this would help non-native speakers, but that's<br>
only one man's hearsay.<br></blockquote><div><br></div><div>Our contribute wiki has the following text in it:</div><br>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.<br>
<br>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<br>
<br>3) Separate white-space changes from logic changes. Commits should never do both</div><div class="gmail_quote"><br></div><div class="gmail_quote">But in general these guidelines will only be effective if patches are rejected because of them; which I will start doing.</div>
<div class="gmail_quote"><br></div><div class="gmail_quote">For those with English difficulties, I often clean up commit messages when I queue them.<br><div> </div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Memory Allocation/Reallocation/Freeing<br>
--------------------------------------<br>
<br>
It's a mess. A big one. The codebase is a tangled mess of inconsistently used malloc(),<br>
free(), new, delete, X265_MALLOC(), and X265_FREE(). X265_REALLOC() does not exist. So,<br>
which is it? Make up your minds! This will affect an future C ports.<br></blockquote><div><br></div><div>The HM code base used new/delete, malloc/free, and xMalloc/xFree macros where memory alignment was explicitly required.</div>
<div><br></div><div>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.</div><div><br></div><div>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.</div>
<div><br></div><div>I don't think we use a realloc currently, but when we do we'll be certain to make a portable X265_REALLOC</div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Reimplementations of STL and Related Identity Crises<br>
----------------------------------------------------<br>
<br>
Why are you reimplementing STL classes like std::list? I could understand if it was in C,<br>
so as to aid in the promised future port to C, but it's being done in C++, using new and<br>
delete, etc. This makes no sense. There's no reason to do this other than some misguided<br>
Not In House syndrome or obsession with the resulting binary size. So, uh, what?<br></blockquote><div><br></div><div>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.<br>
</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Communication<br>
-------------<br>
<br>
Only Steve ever seems to be available on IRC, and since he travels a lot, it's very cumbersome<br>
to interact with the developers in real time. I don't even think the IRC channel is listed on<br>
the wiki page for contributing.<br></blockquote><div><br></div><div>The wiki part, at least, is easily addressed. The contribute page now links to <a href="https://bitbucket.org/multicoreware/x265/wiki/IRC">https://bitbucket.org/multicoreware/x265/wiki/IRC</a></div>
<div>I'll have our engineers add themselves to that list. The wiki is open, so other interested developers should also add themselves.</div><div><br></div><div>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.</div>
<div> </div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Intrinsics<br>
----------<br>
<br>
When will they die? When will new ones stop getting added? This may have been answered already,<br>
but I didn't hear/see anything.<br></blockquote><div><br></div><div>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.</div>
<div><br></div><div>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.</div>
<div><br></div><div>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.</div>
<div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Priorities<br>
----------<br>
<br>
This is by far the most concerning aspect for me, as an outsider. The priorities for x265 seem<br>
to be geared towards demos or a 'cash out'. Rather than making the encoder *good*, first priorities<br>
have gone to making it fast. This seems very backwards. Similarly, the most useless ratecontrol<br>
method is the only method being worked on, since MCW wants to demo HLS, as far as I can tell.<br>
<br>
Second to this, is that I don't completely trust that MCW will not either stop work after<br>
it demos / cashes out (or x265 is 'good enough'), or won't backseat important things for the<br>
sake of demos, etc.<br></blockquote><div><br></div><div>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).</div>
<div><br></div><div>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.</div>
<div><br></div><div>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.</div>
<div><br></div><div>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).</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
I've seen Bad Things™ happen with corporations involved in FOSS time and time again, so you<br>
could say I'm once burned, twice shy.<br></blockquote><div><br></div><div>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.</div>
<div><br></div><div>And I take the fact that you've taken the time to come rant at us as a good thing.</div><div><br></div><div>Regards,</div><div><br></div><div>--</div><div>Steve Borho</div></div></div></div>