| Message ID | 20180618201930.84734-1-derek.buitenhuis@gmail.com |
|---|---|
| State | New |
| Headers | show |
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
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.)
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 [...]
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
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 [...]
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
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
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(+)