[FFmpeg-devel] [PATCH 5/5] dnxhddec: replace some 0s by DNXHD_VARIABLE

[tim nicholson]

On 05/10/15 08:51, Christophe Gisquet wrote:
> Hi,
> 
> 2015-10-05 8:44 GMT+02:00 tim nicholson <nichot20-at-yahoo.com at ffmpeg.org>:
>> On 02/10/15 20:00, Christophe Gisquet wrote:
>>> A series of 0 in a table can be confusing, and the corresponding checks
>>> strange, so using a macro instead of that magic is more readable.
>>> ---
>>>  libavcodec/dnxhddata.c | 10 +++++-----
>>>  libavcodec/dnxhddata.h |  3 +++
>>>  libavcodec/dnxhddec.c  |  6 ++++--
>>>  3 files changed, 12 insertions(+), 7 deletions(-)
>>>
>>> [..]
>>> +/** Indicate that a CIDEntry value must be read in the bitstream */
>>> +#define DNXHD_VARIABLE 0
>>> +
>>
>> I'm all for more readable, presumably atm its only ever 0 but may change
>> in the future, in which case some idea of what it represents may be
>> useful, I am sure the spec has more than one variable ;)
> 
> Actually, it's a convenience value, that we put there after noticing
> new samples.
> 
> We don't have access to the DNxHR specs, but it seems a "profile" can
> have differing bitdepths. I haven't yet verified, but the frame header
> probably allows to distinguish them (DNXHD_VARIABLE applies to the HR
> version).
>

That's fine, I just thought there ought to be a better name for it. Just
calling it VARIABLE begs the question "what is variable?" If you are
trying to make the code clearer, give some clue as to what it relates
to. However if atm you aren't quite sure but think it might be handy
later, then I can see that that becomes a bit difficult.

I thought JJ had got hold of a copy of the specs...


> I have, down the line, things that may need to split the HR versions.
> 


-- 
Tim.
Key Fingerprint 38CF DB09 3ED0 F607 8B67 6CED 0C0B FC44 8B0B FC83