[FFmpeg-devel] [PATCH 2/4] cbs: Remove useless initializations

[Andreas Rheinhardt]

Reimar Döffinger:
> On 03.06.2019, at 14:07, Andreas Rheinhardt <andreas.rheinhardt at gmail.com> wrote:
> 
>> Reimar Döffinger:
>>> On 03.06.2019, at 00:37, Andreas Rheinhardt <andreas.rheinhardt at gmail.com> wrote:
>>>
>>>> Up until now, a temporary variable was used and initialized every time a
>>>> value was read in CBS; if reading turned out to be successfull, this
>>>> value was overwritten (without having ever been looked at) with the
>>>> value read if reading was successfull; on failure the variable wasn't
>>>> touched either. Therefore these initializations can be and have been
>>>> removed.
>>>>
>>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
>>>> ---
>>>> And? What did the ancient compilers say?
>>>
>>> Not sure how to read that, but some compilers probably produce "may be used uninitialized" warnings after this change.
>>> IMHO it would be better to need evidence of an advantage to remove variable initialization for non-trivial code, even if they are unnecessary. Your commit message doesn't seem to mention one at least.
>>> But I am happy to let some maintainer have the last word.
>> This comment refers to what Mark said in his review [1] of an earlier
>> version of this patchset. He explicitly mentioned that (if his memory
>> is right) an older compiler warned without the initializations.
>> And I actually thought that there is no need for a further reason to
>> remove unnecessary initializations despite the usual ones: Speed and
>> code size. E.g. the size of my cbs_mpeg2.o went down from 30421 B to
>> 29445 B after this patch; for  cbs_h2645.o the numbers are 249605 B
>> and 242245 B. These macros are used a lot (mostly indirectly via other
>> macros); see the various cbs_*_syntax_template.c files for that.
> 
> Well, your commit message did not mention that.
> I kind of assumed that given the compiler does not emit a "may be used uninitialized" warning it figured out that the initialization was unnecessary and would produce identical code.
> Maybe the compiler just is more conservative about that warning - or the code size increase might be a bug worth reporting. But I admit finding out which might be a bit too much work.
> Either way I would appreciate mentioning explicitly in the commit message code size and speed advantages if you have numbers already anyway.
In case of ff_cbs_read_unsigned and ff_cbs_read_signed the compiler
can't figure out whether the initialization is necessary or not
because these functions live in different translation units. In the
other cases, the compiler can figure it out, but GCC 8.3 often doesn't
do so at O3:
- For AV1, it detects for uvlc and subexp that the initializations are
unnecessary, not for the the other ones.
- For H2645, the uselessness of the initializations in the exponential
golomb macros is not detected.
- For VP9, it detects it for increment, but not for cbs_vp9_read_s.
So let's really see what Mark (the maintainer) says.

- Andreas

PS: When trying to find out what is different wrt. uvlc and subexp
compared to the other ones I found out that both cbs_av1_read_leb128
and cbs_av1_read_subexp call ff_cbs_read_unsigned with a pointer to an
unitialized variable as argument.