3 ms·

There sure are a lot of hard-coded numbers in that codebase. In many cases it's easy to figure out where the numbers came from, but in others, it's nearly inscr

There sure are a lot of hard-coded numbers in that codebase. In many cases it's easy to figure out where the numbers came from, but in others, it's nearly inscrutable without a named constant or a comment or something. Here's one example: https://github.com/cisco/openh264/blob/master/codec/encoder/core/src/svc_base_layer_md.cpp#L1254 https://github.com/cisco/openh264/blob/master/codec/encoder/... Where does "15" come from? I suppose if I'd written a codec like this before, or if I stared at the code long enough, I could figure it out, but wouldn't it be better to use an enum or a #define?

tetrep 13y ago
while i'm sure there's many other examples, in that particular case it's just the maximum value of an unsigned 8 bit integer, as defined above on line 1252. it would be a bit better if it explicitly had a nice #define but recognizing the values of common powers of two (minus one) is a useful code reading skill.
- josephlord 13y ago
  I think you mean 4bit.
matthewmacleod 13y ago
Huh. I don't think it's just you - that looks like some properly nasty code. Formatting is all over the place, lines are commented out, comments are useless and obviously wrong in places... I don't really speak C++ though, so maybe this is normal.
- nly 13y ago
  It's not because it's C++... this is just really ugly code.
  - midas007 13y ago
    The good point is that other hacked together quasi-compatible implementations now have an "official" reference implementation to browse. It could be a lot worse... I've waded through uncommented OCaml to discover how a certain shall-go-unnamed commercial, non-restful XML API worked. Or, Cisco could've never open-sourced it.
    - nly 13y ago
      I'm not in to video circles anymore, but I was under the impression the existing open source implementations were way better. The reason this code is significant is Cisco is providing licensed binaries.
    - 0x09 13y ago
      AVC/H.264, like many MPEG standards, has an official reference implementation: http://iphome.hhi.de/suehring/tml/ http://iphome.hhi.de/suehring/tml/
_wmd 13y ago
Encountered this topic recently in the office.. I'm from the side of the fence that doesn't overly care about code tidyness, so long as it performs its overall function. 10 years ago this kind of thing might have bothered me a lot more, but at some point crossed a threshold where I realized _all code generated to the present day_ is pretty ugly and long term unmaintainable (but that's a story for a rather large and rather boring essay). The tl;dr is simply that if you obsess over minor details on this level, a lot of brainpower is wasted that could be used for bigger problems you should be much more worried about. Playing devil's advocate, in this case the if() is obviously a guard for the subsequent switch. Moving just the constant '15' into a #define would make it read more like some magical sentinel value, unless you also #defined all the literal values used in the switch, at which point you've introduced a wholly bullshit layer of abstraction to what was otherwise incredibly concrete and explicit code. Let's assume you have a great reason for doing that. OK. So what do you call these constants? Well, the code appears to be branching to special cases based on the width of some integer. So we instead have what, WIDTH_1_BIT, WIDTH_2_BITS, ..., WIDTH_15_BITS? Now we've pulled those constants out, you stare at the block of #defines, and think, damn, this is so ugly since most of the value space isn't fully defined! So some kindly maintenance programmer comes along and pads out the rest, producing a perfectly beautiful block of utter line noise. That is arguably considerably less readable than what we started with
- hughlomas 13y ago
  A simple //comment would have sidestepped all of that and still clarified the number's purpose.
  - lyndonh 13y ago
    It's all very well when it's a simple variable but quite often I have come up against variables that would take a whole paragraph of text to explain their purpose and don't have a concise or obvious name.
    - JasonFruit 13y ago
      I find quite often a little hard to believe. Occasionally, maybe; and why not provide a paragraph of text? Sometimes it's the right thing to do.
ChuckMcM 13y ago
As with many things it seems like knowing how an H264 codec works before reading the code is essential. I don't someone has an approachable reference for the standard?
andrewf 13y ago
IMO it doesn't make sense to read code like this without a copy of the H.264 spec in hand. And once you've got that spec, why paraphrase a fraction of it, poorly, in the source code? I do think that comments at the function level indicating which part of the spec you should be reading would be nice. They might not be an issue for people who are indoctrinated into the code though.
- baq 13y ago
  // spec 12.3.4.5 would certainly help if you had a spec open.

tetrep 13y ago

while i'm sure there's many other examples, in that particular case it's just the maximum value of an unsigned 8 bit integer, as defined above on line 1252. it would be a bit better if it explicitly had a nice #define but recognizing the values of common powers of two (minus one) is a useful code reading skill.

josephlord 13y ago
I think you mean 4bit.

matthewmacleod 13y ago

Huh. I don't think it's just you - that looks like some properly nasty code. Formatting is all over the place, lines are commented out, comments are useless and obviously wrong in places... I don't really speak C++ though, so maybe this is normal.

nly 13y ago
It's not because it's C++... this is just really ugly code.
- midas007 13y ago
  The good point is that other hacked together quasi-compatible implementations now have an "official" reference implementation to browse. It could be a lot worse... I've waded through uncommented OCaml to discover how a certain shall-go-unnamed commercial, non-restful XML API worked. Or, Cisco could've never open-sourced it.
  - nly 13y ago
    I'm not in to video circles anymore, but I was under the impression the existing open source implementations were way better. The reason this code is significant is Cisco is providing licensed binaries.
  - 0x09 13y ago
    AVC/H.264, like many MPEG standards, has an official reference implementation: http://iphome.hhi.de/suehring/tml/ http://iphome.hhi.de/suehring/tml/

_wmd 13y ago

Encountered this topic recently in the office.. I'm from the side of the fence that doesn't overly care about code tidyness, so long as it performs its overall function. 10 years ago this kind of thing might have bothered me a lot more, but at some point crossed a threshold where I realized _all code generated to the present day_ is pretty ugly and long term unmaintainable (but that's a story for a rather large and rather boring essay). The tl;dr is simply that if you obsess over minor details on this level, a lot of brainpower is wasted that could be used for bigger problems you should be much more worried about. Playing devil's advocate, in this case the if() is obviously a guard for the subsequent switch. Moving just the constant '15' into a #define would make it read more like some magical sentinel value, unless you also #defined all the literal values used in the switch, at which point you've introduced a wholly bullshit layer of abstraction to what was otherwise incredibly concrete and explicit code. Let's assume you have a great reason for doing that. OK. So what do you call these constants? Well, the code appears to be branching to special cases based on the width of some integer. So we instead have what, WIDTH_1_BIT, WIDTH_2_BITS, ..., WIDTH_15_BITS? Now we've pulled those constants out, you stare at the block of #defines, and think, damn, this is so ugly since most of the value space isn't fully defined! So some kindly maintenance programmer comes along and pads out the rest, producing a perfectly beautiful block of utter line noise. That is arguably considerably less readable than what we started with

hughlomas 13y ago
A simple //comment would have sidestepped all of that and still clarified the number's purpose.
- lyndonh 13y ago
  It's all very well when it's a simple variable but quite often I have come up against variables that would take a whole paragraph of text to explain their purpose and don't have a concise or obvious name.
  - JasonFruit 13y ago
    I find quite often a little hard to believe. Occasionally, maybe; and why not provide a paragraph of text? Sometimes it's the right thing to do.

ChuckMcM 13y ago

As with many things it seems like knowing how an H264 codec works before reading the code is essential. I don't someone has an approachable reference for the standard?

andrewf 13y ago

IMO it doesn't make sense to read code like this without a copy of the H.264 spec in hand. And once you've got that spec, why paraphrase a fraction of it, poorly, in the source code? I do think that comments at the function level indicating which part of the spec you should be reading would be nice. They might not be an issue for people who are indoctrinated into the code though.

baq 13y ago
// spec 12.3.4.5 would certainly help if you had a spec open.