[FFmpeg-devel] [PATCH] mjpegdec: Support 4x1, 2x1, 2x1 notation for 4:2:2 chroma subsampling

[Derek Buitenhuis]

On 19/06/2018 12:31, Michael Niedermayer wrote:
> On Mon, Jun 18, 2018 at 09:19:30PM +0100, Derek Buitenhuis wrote:
>> Just one of the many, many ways to store this stuff in the header.
>>
>> Signed-off-by: Derek Buitenhuis <derek.buitenhuis at gmail.com>
>> ---
>> Related reading, but not exactly the same type:
>>      * https://github.com/libjpeg-turbo/libjpeg-turbo/issues/92
>>      * https://github.com/libjpeg-turbo/libjpeg-turbo/commit/8ce2c9119a995ef6280f8bba375aac7effb9b571
>> ---
>>   libavcodec/mjpegdec.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c
>> index d1dca84d36..e5888ed548 100644
>> --- a/libavcodec/mjpegdec.c
>> +++ b/libavcodec/mjpegdec.c
>> @@ -568,6 +568,7 @@ int ff_mjpeg_decode_sof(MJpegDecodeContext *s)
>>           }
>>           break;
>>       case 0x21111100:
>> +    case 0x41212100:
>>           if (s->component_id[0] == 'Q' && s->component_id[1] == 'F' && s->component_id[2] == 'A') {
>>               if (s->bits <= 8) s->avctx->pix_fmt = AV_PIX_FMT_GBRP;
>>               else
> 
> either this is wrong or
> "
>   -    /* NOTE we do not allocate pictures large enough for the possible
>   -     * padding of h/v_count being 4 */
>   "
> this is outdated in some way.

I don't think this was ever a valid comment, since h_coutn and v_count are
not supposed to be interpreted as absolute values to be applied? That is,
you need to look at the ratio.

Further, I am not quite sure what 'padding' refers to in this specific instance,
since this has to do with subsampling? I'm maybe missing something JPEG-specific?

The comments, code, and git history for this whole chunk of code are a confusing
mess and I couldn't easily discern what was done and why. It seems very overly
'clever'...

> in fact we do at least in mjpeg_decode_scan() check w/h and skip writing
> outside. So this may be ok but then it would be simpler
> to update
>       if (!(pix_fmt_id & 0xD0D0D0D0))
>           pix_fmt_id -= (pix_fmt_id & 0xF0F0F0F0) >> 1;
>       if (!(pix_fmt_id & 0x0D0D0D0D))
>           pix_fmt_id -= (pix_fmt_id & 0x0F0F0F0F) >> 1;
> 
> to also work with the count=4 cases

I still don't know why we're mangling these into a single number, to be
honest, or why we're applying bitmasks instead of just directly interpreting
them...

- Derek