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

Submitted by Derek Buitenhuis on June 18, 2018, 8:19 p.m.

Details

Message ID 20180618201930.84734-1-derek.buitenhuis@gmail.com
State New
Headers show

Commit Message

Derek Buitenhuis June 18, 2018, 8:19 p.m.
Just one of the many, many ways to store this stuff in the header.

Signed-off-by: Derek Buitenhuis <derek.buitenhuis@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(+)

Comments

Derek Buitenhuis June 18, 2018, 8:28 p.m.
On 18/06/2018 21:19, Derek Buitenhuis wrote:
> Just one of the many, many ways to store this stuff in the header.
> 
> Signed-off-by: Derek Buitenhuis <derek.buitenhuis@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(+)

Sample: http://chromashift.org/samples/210809515_640.jpg

- Derek
Mark Thompson June 18, 2018, 11:34 p.m.
On 18/06/18 21:19, Derek Buitenhuis wrote:
> Just one of the many, many ways to store this stuff in the header.
> 
> Signed-off-by: Derek Buitenhuis <derek.buitenhuis@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
> 

LGTM.

Thanks,

- Mark


(Yay for another case where the Intel VAAPI driver falls over:

$ ./ffmpeg_g -y -hwaccel vaapi -hwaccel_device /dev/dri/renderD128 -i 210809515_640.jpg out.png
ffmpeg version N-91327-gfa79910871 Copyright (c) 2000-2018 the FFmpeg developers
  built with gcc 7 (Debian 7.3.0-12)
  configuration: --enable-debug --disable-optimizations --assert-level=2 --samples=/home/mrt/video/ffmpeg/fate/ --enable-vaapi --enable-vdpau --enable-opencl --enable-libdrm --enable-amf --enable-libmfx --enable-gpl --enable-libx264 --enable-libx265 --enable-libvpx --enable-libaom
  libavutil      56. 18.102 / 56. 18.102
  libavcodec     58. 20.102 / 58. 20.102
  libavformat    58. 17.100 / 58. 17.100
  libavdevice    58.  4.101 / 58.  4.101
  libavfilter     7. 25.100 /  7. 25.100
  libswscale      5.  2.100 /  5.  2.100
  libswresample   3.  2.100 /  3.  2.100
  libpostproc    55.  2.100 / 55.  2.100
Input #0, image2, from '210809515_640.jpg':
  Duration: 00:00:00.04, start: 0.000000, bitrate: 12128 kb/s
    Stream #0:0: Video: mjpeg, yuvj422p(pc, bt470bg/unknown/unknown), 640x480 [SAR 75:75 DAR 4:3], 25 tbr, 25 tbn, 25 tbc
Stream mapping:
  Stream #0:0 -> #0:0 (mjpeg (native) -> png (native))
Press [q] to stop, [?] for help
ffmpeg_g: gen8_mfd.c:2470: gen8_mfd_jpeg_pic_state: Assertion `0' failed.
Aborted

...  I guess that's still marginally better than the similar <https://github.com/intel/intel-vaapi-driver/issues/363>, where it indicates success and returns garbage.)
Michael Niedermayer June 19, 2018, 11:31 a.m.
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@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.

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

[...]
Derek Buitenhuis June 19, 2018, 12:46 p.m.
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@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
Michael Niedermayer June 20, 2018, 12:32 a.m.
On Tue, Jun 19, 2018 at 01:46:27PM +0100, Derek Buitenhuis wrote:
> 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@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.

The absolute values define the bitstream and have to be used


> 
> 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?

with a count of 4, mjpeg is coded in MCUs of 4 8x8 blocks in that direction
all buffers must be sized accordingly so that these blocks fit in.
I suspect that is already done but i could be wrong

[...]
Derek Buitenhuis July 10, 2018, 2:38 p.m.
Hi,

Sorry for the slow response; life is hectic.

On Tue, Jun 19, 2018 at 8:32 PM, Michael Niedermayer
<michael@niedermayer.cc> wrote:
> The absolute values define the bitstream and have to be used

OK, I was unaware.

> with a count of 4, mjpeg is coded in MCUs of 4 8x8 blocks in that direction
> all buffers must be sized accordingly so that these blocks fit in.
> I suspect that is already done but i could be wrong

Near as I can tell, it should already be handled OK?

I would also update the bit you mentioned, but, if I'm honest,
I am not 100% sure it and its subsequent bits of uncommented
bit fiddling do, or why.

- Derek

Patch hide | download patch | download mbox

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