On 10/02/18 16:29, Luca Barbato wrote:
> On 10/02/2018 16:59, Diego Biurrun wrote:
>> Looks OK in general.
>>
>> On Fri, Feb 09, 2018 at 10:51:36AM +0100, Luca Barbato wrote:
>>> --- /dev/null
>>> +++ b/libavcodec/libaom.c
>>> @@ -0,0 +1,90 @@
>>> +
>>> +#define HIGH_DEPTH(fmt) \
>>> + case AOM_IMG_FMT_I ## fmt ## 16: switch (depth) { \
>>> + case 10: return AV_PIX_FMT_YUV ## fmt ## P10; \
>>> + case 12: return AV_PIX_FMT_YUV ## fmt ## P12; \
>>> + default: return AV_PIX_FMT_NONE; \
>>
>> Move the switch statement to the next line for better readability please.
>
> Sure
>
>>
>>> +enum AVPixelFormat ff_aom_imgfmt_to_pixfmt(aom_img_fmt_t img, int depth)
>>> +{
>>> + switch (img) {
>>> + case AOM_IMG_FMT_RGB24: return AV_PIX_FMT_RGB24;
>>> + case AOM_IMG_FMT_RGB565: return AV_PIX_FMT_RGB565BE;
>>> + case AOM_IMG_FMT_RGB555: return AV_PIX_FMT_RGB555BE;
>>> + case AOM_IMG_FMT_UYVY: return AV_PIX_FMT_UYVY422;
>>> + case AOM_IMG_FMT_YUY2: return AV_PIX_FMT_YUYV422;
>>> + case AOM_IMG_FMT_YVYU: return AV_PIX_FMT_YVYU422;
>>> + case AOM_IMG_FMT_BGR24: return AV_PIX_FMT_BGR24;
>>> + case AOM_IMG_FMT_ARGB: return AV_PIX_FMT_ARGB;
>>> + case AOM_IMG_FMT_ARGB_LE: return AV_PIX_FMT_BGRA;
>>> + case AOM_IMG_FMT_RGB565_LE: return AV_PIX_FMT_RGB565LE;
>>> + case AOM_IMG_FMT_RGB555_LE: return AV_PIX_FMT_RGB555LE;
>>> + case AOM_IMG_FMT_I420: return AV_PIX_FMT_YUV420P;
>>> + case AOM_IMG_FMT_I422: return AV_PIX_FMT_YUV422P;
>>> + case AOM_IMG_FMT_I444: return AV_PIX_FMT_YUV444P;
>>> + case AOM_IMG_FMT_444A: return AV_PIX_FMT_YUVA444P;
>>> + case AOM_IMG_FMT_I440: return AV_PIX_FMT_YUV440P;
>>
>> I'd break those lines.
>
> I can run uncrustify to break it, is that the outcome you'd expect?
>
>>> +/* case AOM_IMG_FMT_I42016: return AV_PIX_FMT_YUV420P16;
>>> + case AOM_IMG_FMT_I42216: return AV_PIX_FMT_YUV422P16;
>>> + case AOM_IMG_FMT_I44416: return AV_PIX_FMT_YUV444P16; */
>>
>> Why is this commented out?
>
> I should just remove it, thanks for reminding me.
>
>>> +aom_img_fmt_t ff_aom_pixfmt_to_imgfmt(enum AVPixelFormat pix)
>>> +{
>>> + switch (pix) {
>>> + case AV_PIX_FMT_RGB24: return AOM_IMG_FMT_RGB24;
>>> + case AV_PIX_FMT_RGB565BE: return AOM_IMG_FMT_RGB565;
>>> + case AV_PIX_FMT_RGB555BE: return AOM_IMG_FMT_RGB555;
>>> + case AV_PIX_FMT_UYVY422: return AOM_IMG_FMT_UYVY;
>>> + case AV_PIX_FMT_YUYV422: return AOM_IMG_FMT_YUY2;
>>> + case AV_PIX_FMT_YVYU422: return AOM_IMG_FMT_YVYU;
>>> + case AV_PIX_FMT_BGR24: return AOM_IMG_FMT_BGR24;
>>> + case AV_PIX_FMT_ARGB: return AOM_IMG_FMT_ARGB;
>>> + case AV_PIX_FMT_BGRA: return AOM_IMG_FMT_ARGB_LE;
>>> + case AV_PIX_FMT_RGB565LE: return AOM_IMG_FMT_RGB565_LE;
>>> + case AV_PIX_FMT_RGB555LE: return AOM_IMG_FMT_RGB555_LE;
>>> + case AV_PIX_FMT_YUV420P: return AOM_IMG_FMT_I420;
>>> + case AV_PIX_FMT_YUV422P: return AOM_IMG_FMT_I422;
>>> + case AV_PIX_FMT_YUV444P: return AOM_IMG_FMT_I444;
>>> + case AV_PIX_FMT_YUVA444P: return AOM_IMG_FMT_444A;
>>> + case AV_PIX_FMT_YUV440P: return AOM_IMG_FMT_I440;
>>> + case AV_PIX_FMT_YUV420P10: return AOM_IMG_FMT_I42016;
>>> + case AV_PIX_FMT_YUV422P10: return AOM_IMG_FMT_I42216;
>>> + case AV_PIX_FMT_YUV444P10: return AOM_IMG_FMT_I44416;
>>> + case AV_PIX_FMT_YUV420P12: return AOM_IMG_FMT_I42016;
>>> + case AV_PIX_FMT_YUV422P12: return AOM_IMG_FMT_I42216;
>>> + case AV_PIX_FMT_YUV444P12: return AOM_IMG_FMT_I44416;
>>
>> same
>
> Ok.
If you feel like tearing down the bikeshed and building a different one: I
think this would be nicer as a single table with the two functions reading it,
rather than two functions which duplicate a lot of the information.
(Or ignore this if you prefer how the bikeshed is currently made...)
- Mark
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
