Rav1e Sees New Pre-Release With More Speed-Ups, Monochrome Support

[wswartzendruber]

I asked some people on Reddit to review my first Rust program. They had some useful insight to offer, but also complained that they didn't understand what the _eotf_, _oetf_, and _ootf_ variables meant and were therefore obscure. If you don't know what those are, you shouldn't be working on the code.

[bug77]

I asked some people on Reddit to review my first Rust program. They had some useful insight to offer, but also complained that they didn't understand what the _eotf_, _oetf_, and _ootf_ variables meant and were therefore obscure. If you don't know what those are, you shouldn't be working on the code.

Yes, that's the spirit

You do have a point in that there's always domain specific stuff that doesn't need to be explained left and right, but the attitude of "you can't X you shouldn't be doing Y" is among the most toxic attitudes one can have on a team.
And even for domain specific terms, I'd still link to some wiki page, RFC or something. Obviously not in the source code, but maybe in a readme file or something. It's amazing how often people will come up with their own definitions for the most established of terms (just today I had to point out there is a difference between "acceptance criteria" and "definition of done" - I don't think I got through).

[wswartzendruber]

Yes, that's the spirit

You do have a point in that there's always domain specific stuff that doesn't need to be explained left and right, but the attitude of "you can't X you shouldn't be doing Y" is among the most toxic attitudes one can have on a team.

If doing Y is going to cause the quality of the project to degrade (because you don't understand X), then you can bet your ass I won't have you messing with it.

And even for domain specific terms, I'd still link to some wiki page, RFC or something. Obviously not in the source code, but maybe in a readme file or something. It's amazing how often people will come up with their own definitions for the most established of terms (just today I had to point out there is a difference between "acceptance criteria" and "definition of done" - I don't think I got through).

The source file in question is here: https://github.com/wswartzendruber/p...er/src/main.rs

So there are citations in the source itself telling you exactly where these values come from. EOTF, OETF, and OOTF are universally understood terms within video DSP.

[polarathene]

So there are citations in the source itself telling you exactly where these values come from. EOTF, OETF, and OOTF are universally understood terms within video DSP.

Same as with comments on code quality about Rav1e here, it's common domain knowledge for anyone who would actually be capable of contributing, otherwise they'll need to acquire that knowledge prior to being able to contribute regardless, at which point those terms would already stick out and not seem as obscure as they may be to someone who's not working on such.

When working on projects that are higher level, making up non-standard variable names for being terse with no explanation is where it can be a real problem, because it's not tied to a specific field. There is a big difference there that people with an issue about such don't seem to get.

But to be fair, you were asking for people to review your Rust code, and without that domain knowledge they may have a harder time grokking what is going on, they're not going to want to look too deeply into it and acquire domain knowledge they have no need for themselves. Your code is all in a single main.rs file, so it wouldn't be too much of an issue to guide the reviewers a bit more? Those 3 functions and their tests could even be extracted to a separate file/mod to import, as that appears to be where the majority of domain jargon with your naming is.

I tried rustfmt on the input/output vars at line 87, and the output was pretty bad.. At first it looked a bit weird to me with the ending indentation from grouping `})` to a single line, but I guess what you did reads fine/better than splitting the `)` to the next line or reducing the indentation.

If you don't know what those are, you shouldn't be working on the code.

Just to re-iterate here, and probably obvious, but they're just reviewing rust, they're not working on the code so domain knowledge can't be expected, it may be clear that's what it is, but it makes it difficult to review and may just be glanced over as "noise" with a CBF attitude.

[milkylainen]

There is a ton of inline assembler like this https://github.com/xiph/rav1e/blob/m...4/loopfilter.S that of course looks like ass, but there is no real way around that, these projects do need assembler as they are full of performance-critical code.

But for the actual high-level code like for example this https://github.com/xiph/rav1e/blob/m...rc/quantize.rs I don't see much of the problems. For example yes it uses a a b c d as symbol names, but it's internal variables of a 5-line function doing a specific job, for longer functions it uses decent variable names. There is also verbose comments explaining what is going on.

At least from my quick look and limited programming experience, it does not look bad. Or at least I've seen so awful shit in PHP and java that this looks great in comparison.

It was nothing against rav1e specifically. But a general comment towards a lot of encoders.
They tend to look like a hot mess (still very subjective), irrespective of language.
My comment with with rav1e was more that "Rust does not make the code prettier"... Which is a rather stupid analysis.

The line of thoughts was something like:
"Hmm. I wonder if breaking with old languages would also give a "prettier" situation (beside the touted advantages). Hmm. No."
In truth, yes you are right, it is better than a lot of other encoders, so perhaps unnecessarily harsh with the words.

But still, to my eyes, they look like a mess.
Maybe encoders just don't lend themselves to being written as pretty code (subjective)?

[milkylainen]

What is wrong with the indentation? It's using the language formatter, so it should be pretty standard if you work on Rust code. You can always have that transformed to an appropriate indentation you're comfortable with while viewing or working on it, not a difficult thing to do.

Naming of variables and symbols? See prior post explaining a few snippets someone else pointed out as seeming poor quality. It's quite likely the same applies to whatever you're not happy about, lack of domain knowledge. I don't know some of it but it wasn't hard to figure out, and once I did those names made sense and I'd know them if I had the skills to contribute to such, so non-issue. Perhaps you saw something where that doesn't apply though?

Not sure what you're referring to by non-obvious/hiding control flow. Looks fairly standard to me at a glance, could it just be you're not that familiar with rust so it looks "off" to you?

Code structure issues in what way? Breaking logic into smaller functions that are composed instead of nested or utilizing something like inheritance? Or are you also referring to file structure/hierarchy (which seems fairly decent at a glance imo). Again, it's possible you're viewing this with some personal bias of what is right is what you're comfortable with, and it may not necessarily apply well to a project like this one.

I've seen horrid shit before, and this is not that.

Yes. I was a bit harsh with the words. Also, it was not a comment against rav1e specifically.
Not more than "Writing it in Rust does not seem to help my view on what encoders look like".
I still think that most encoders fail miserably at looking (subjectively) like good code to me.

I have a good base in signal math education and I have no trouble understanding most concepts in encoders.
I've also worked quite a bit with media type problems, mostly streaming and encoding.

And it might be that my personal bias of what good code should look like does not apply here.
Or to this type of problem in general (encoders in general).
Maybe I should spend some time writing my own encoder instead.
I'm actually looking for a very specific type of AVC encoder (not HVEC) to solve a very specific problem in software,
but there is none... So maybe a window of opportunity?

[polarathene]

I still think that most encoders fail miserably at looking (subjectively) like good code to me.

I have a good base in signal math education and I have no trouble understanding most concepts in encoders.
I've also worked quite a bit with media type problems, mostly streaming and encoding.

Well, I'm sure you could take the time to address this concern you have by doing a refactor here and there at a small scale introducing improvements to the codebase that would satisfy your expectations of code quality? Then send a PR, see how it goes, perhaps they love what you did and really appreciate it, merge it, and if you like you can continue to contribute iteratively like that.

[starshipeleven]

It was nothing against rav1e specifically. But a general comment towards a lot of encoders.
They tend to look like a hot mess (still very subjective), irrespective of language.

Unless you want to specify what is that looks "bad" in your eyes, you can save the time posting the same thing over and over again.

You said it looks bad to you, which per-se means nothing. We understand you said that and asked for clarifications. Repeating the same concept 5 times in another post isn't an answer to that.