[vlc-devel] [PATCH] omxil: use OMX chroma type instead of VLC type in GetVlcChromaSizes.

[Martin Storsjö]

> 2013/10/23 Martin Storsjö <martin at martin.st>
>
>       However, what I'm unsure of is which case this actually fixes.
>       Is there any current config that is broken that is fixed by this
>       patch?
> 
> No, but it looks like the problem can happen for a generic OMXIL module
> (omxil.c). For instance with a
> GetVlcChromaFormat(OMX_TI_COLOR_FormatYUV420PackedSemiPlanar, ...) followed
> by a GetVlcChromaSizes(...).

Right. Yes, OMX_TI_COLOR_FormatYUV420PackedSemiPlanar seemed to have an 
incorrect value in the table, which I fixed now. Thanks for pointing this 
out!

>       The only place in chroma_format_table where one VLC chroma has
>       got differing values depending on the OMX chroma format seems to
>       be the values for VLC_CODEC_NV12, and the value for
>       OMX_TI_COLOR_FormatYUV420PackedSemiPlanar might actually be
>       wrong, since they should all have the same value for
>       i_line_chroma_div.
> 
> Can someone double check the entries in the chroma_format_table?
> Is it also true for i_size_mul and i_line_mul? If this is the case then it
> seems confusing and error-prone to have one table with VLC type, OMX type
> and dimensions.
> If the dimensions only depends on the VLC type then we could have a separate
> table mapping a VLC type to dimensions information.

Well, yes, for all the current types this should be the case. Note that 
QOMX_COLOR_FormatYUV420PackedSemiPlanar64x32Tile2m8ka is handled quite 
differently from the other ones (see CopyOmxPicture), so in general you're 
right - each OMX pixel format _might_ need special individual treatment. 
But if they need special treatment they might need quite a bit of more 
handling just as we have in CopyOmxPicture right now.

So as things are right now, refactoring it to have a separate table for 
these parameters might be correct (or then you might even be able to use 
some vlc generic chroma format info functions to avoid having to hardcode 
it at all), but I'm not sure it's worth the effort, since it might have to 
be undone if we'd need extra care for some really odd pixel format (in 
which case we might want to merge the patch you submitted now).

// Martin