[FFmpeg-devel] [PATCH v3] avcodec: add Actimagine VX video decoder

[Andreas Rheinhardt]

Florian Nouwt:
> Andreas Rheinhardt:
> 
> - Why are these tables not internal to actimagine_vx.c?
> 
> I separated the data because for parsing the vx container files I will
> need a parser, which will be in a separate file from the decoder and
> requires those tables.
> 
> - You wasted an opportunity to add a space before '=' (this code is
> old and does not match the currently preferred style).
> 
> Just to have it clear, when it comes to the tables, is the preferred
> style with the brace on the next line or on the same line? And am I
> supposed to fix all style errors in that file?

I don't have a preference for next line vs same line, but I don't like
something like
}
};
as happens in ff_h264_cavlc_coeff_token_*.
When you change a line, you should fix style issues in said line at the
same time. You are not supposed to touch other lines just to fix style
issues. Such things should be in a separate commit, if at all (after
all, they make using git blame harder).

> 
> - In case the actimagine_vx decoder is disabled and only the H.264
> decoder is enabled (I expect this to happen for lots of slim builds),
> one does not need to use an ff_thread_once() here at all, because
> ff_h264_decode_init_vlc is already guarded this way. Can you add
> compile-time checks for this?
> 
> It would remove some safety ofc. But I guess I can put a comment in
> the header file that tells any potential future people that might use
> it to not forget to change it.
> 

Fine.

- Andreas