[FFmpeg-devel] [PATCH v3 0/3] RGB mjpeg fixes (with FATE tests)

[FFmpeg-devel] [PATCH v3 0/3] RGB mjpeg fixes (with FATE tests)

[Leo Izen]

This is the same patchset as v2, with an additional patch to fix TRAC issue #10910
which was broken before any of the recent RGB mjpeg patches.

Leo Izen (3):
  avcodec/mjpegdec: fix non-subsampled RGB JPEGs
  avcodec/mjpeg: fix weird RGB-subsampled baseline JPEGs
  fate: add tests for RGB jpegs

 libavcodec/mjpegdec.c              | 9 ++++++++-
 tests/fate/image.mak               | 9 +++++++++
 tests/ref/fate/jpg-rgb-221         | 6 ++++++
 tests/ref/fate/jpg-rgb-baseline    | 6 ++++++
 tests/ref/fate/jpg-rgb-progressive | 6 ++++++
 5 files changed, 35 insertions(+), 1 deletion(-)
 create mode 100644 tests/ref/fate/jpg-rgb-221
 create mode 100644 tests/ref/fate/jpg-rgb-baseline
 create mode 100644 tests/ref/fate/jpg-rgb-progressive

-- 
2.40.0

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH v3 1/3] avcodec/mjpegdec: fix non-subsampled RGB JPEGs

[Leo Izen]

The change introduced in b18a9c29713abc3a1b081de3f320ab53a47120c6
created a regression for non-subsampled progressive RGB jpegs. This
should fix that.
---
 libavcodec/mjpegdec.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c
index 01537d4774..1e3ddb72fb 100644
--- a/libavcodec/mjpegdec.c
+++ b/libavcodec/mjpegdec.c
@@ -1698,7 +1698,8 @@ int ff_mjpeg_decode_sos(MJpegDecodeContext *s, const uint8_t *mb_bitmask,
         s->h_scount[i]  = s->h_count[index];
         s->v_scount[i]  = s->v_count[index];
 
-        if(nb_components == 3 && s->nb_components == 3 && s->avctx->pix_fmt == AV_PIX_FMT_GBRP)
+        if((nb_components == 3 || nb_components == 1) && s->nb_components == 3
+                && s->avctx->pix_fmt == AV_PIX_FMT_GBRP && !s->progressive)
             index = (index+2)%3;
 
         s->comp_index[i] = index;
-- 
2.40.0

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH v3 2/3] avcodec/mjpeg: fix weird RGB-subsampled baseline JPEGs

[Leo Izen]

These were broken before any of the recent RGB JPEG commits. This is an
additional fix to bring them in line with the rest of the RGB jpegs,
which should now all work after the previous patch.

Fixes #10190.
---
 libavcodec/mjpegdec.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c
index 1e3ddb72fb..330c5da5a8 100644
--- a/libavcodec/mjpegdec.c
+++ b/libavcodec/mjpegdec.c
@@ -576,6 +576,12 @@ int ff_mjpeg_decode_sof(MJpegDecodeContext *s)
             if (s->adobe_transform == 0 || s->component_id[0] == 'R' &&
                     s->component_id[1] == 'G' && s->component_id[2] == 'B') {
                 s->avctx->pix_fmt = AV_PIX_FMT_GBRP;
+                if (!s->progressive) {
+                    for (int i = 0; i < 2; i++) {
+                        FFSWAP(int, s->upscale_h[i], s->upscale_h[(i + 2) % 3]);
+                        FFSWAP(int, s->upscale_v[i], s->upscale_v[(i + 2) % 3]);
+                    }
+                }
             } else {
                 s->avctx->pix_fmt = s->cs_itu601 ? AV_PIX_FMT_YUV444P : AV_PIX_FMT_YUVJ444P;
                 s->avctx->color_range = s->cs_itu601 ? AVCOL_RANGE_MPEG : AVCOL_RANGE_JPEG;
-- 
2.40.0

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH v3 3/3] fate: add tests for RGB jpegs

[Leo Izen]

Added framecrc tests for RGB JPEGs to verify future changes to the
decoder.
---
 tests/fate/image.mak               | 9 +++++++++
 tests/ref/fate/jpg-rgb-221         | 6 ++++++
 tests/ref/fate/jpg-rgb-baseline    | 6 ++++++
 tests/ref/fate/jpg-rgb-progressive | 6 ++++++
 4 files changed, 27 insertions(+)
 create mode 100644 tests/ref/fate/jpg-rgb-221
 create mode 100644 tests/ref/fate/jpg-rgb-baseline
 create mode 100644 tests/ref/fate/jpg-rgb-progressive

diff --git a/tests/fate/image.mak b/tests/fate/image.mak
index 42dd90feaa..93bc715ca3 100644
--- a/tests/fate/image.mak
+++ b/tests/fate/image.mak
@@ -337,6 +337,15 @@ fate-jpg-12bpp: CMD = framecrc -idct simple -i $(TARGET_SAMPLES)/jpg/12bpp.jpg -
 FATE_JPG += fate-jpg-jfif
 fate-jpg-jfif: CMD = framecrc -idct simple -i $(TARGET_SAMPLES)/jpg/20242.jpg
 
+FATE_JPG += fate-jpg-rgb-baseline
+fate-jpg-rgb-baseline: CMD = framecrc -idct simple -i $(TARGET_SAMPLES)/jpg/george-insect-rgb-baseline.jpg
+
+FATE_JPG += fate-jpg-rgb-progressive
+fate-jpg-rgb-progressive: CMD = framecrc -idct simple -i $(TARGET_SAMPLES)/jpg/george-insect-rgb-progressive.jpg
+
+FATE_JPG += fate-jpg-rgb-221
+fate-jpg-rgb-221: CMD = framecrc -idct simple -i $(TARGET_SAMPLES)/jpg/george-insect-rgb-xyb.jpg
+
 FATE_JPG_TRANSCODE-$(call TRANSCODE, MJPEG, MJPEG IMAGE_JPEG_PIPE, IMAGE_PNG_PIPE_DEMUXER PNG_DECODER SCALE_FILTER) += fate-jpg-icc
 fate-jpg-icc: CMD = transcode png_pipe $(TARGET_SAMPLES)/png1/lena-int_rgb24.png mjpeg "-vf scale" "" "-show_frames"
 
diff --git a/tests/ref/fate/jpg-rgb-221 b/tests/ref/fate/jpg-rgb-221
new file mode 100644
index 0000000000..32250db493
--- /dev/null
+++ b/tests/ref/fate/jpg-rgb-221
@@ -0,0 +1,6 @@
+#tb 0: 1/25
+#media_type 0: video
+#codec_id 0: rawvideo
+#dimensions 0: 64x43
+#sar 0: 0/1
+0,          0,          0,        1,     8256, 0x81617757
diff --git a/tests/ref/fate/jpg-rgb-baseline b/tests/ref/fate/jpg-rgb-baseline
new file mode 100644
index 0000000000..f7ff29e9a2
--- /dev/null
+++ b/tests/ref/fate/jpg-rgb-baseline
@@ -0,0 +1,6 @@
+#tb 0: 1/25
+#media_type 0: video
+#codec_id 0: rawvideo
+#dimensions 0: 64x43
+#sar 0: 0/1
+0,          0,          0,        1,     8256, 0x98ad8863
diff --git a/tests/ref/fate/jpg-rgb-progressive b/tests/ref/fate/jpg-rgb-progressive
new file mode 100644
index 0000000000..b0d118d21a
--- /dev/null
+++ b/tests/ref/fate/jpg-rgb-progressive
@@ -0,0 +1,6 @@
+#tb 0: 1/25
+#media_type 0: video
+#codec_id 0: rawvideo
+#dimensions 0: 64x43
+#sar 0: 0/1
+0,          0,          0,        1,     8256, 0xbb6e8830
-- 
2.40.0

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v3 0/3] RGB mjpeg fixes (with FATE tests)

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 1080 bytes --]

On Wed, Apr 19, 2023 at 02:11:23PM -0400, Leo Izen wrote:
> This is the same patchset as v2, with an additional patch to fix TRAC issue #10910
> which was broken before any of the recent RGB mjpeg patches.
> 
> Leo Izen (3):
>   avcodec/mjpegdec: fix non-subsampled RGB JPEGs
>   avcodec/mjpeg: fix weird RGB-subsampled baseline JPEGs
>   fate: add tests for RGB jpegs

as you are working on jpeg, i wanted to mention there are a few  old
patches which i run into atm while looking at the ML greping subjects
which seem not to have been reviewed or applied

211754 N   0328  0:35 Carl Eugen Hoyo (2,7K) [FFmpeg-devel] [PATCH]lavc/mjpegdec: Support pixel format 0x11311100
211755 N   0328  0:36 Carl Eugen Hoyo (2,8K) [FFmpeg-devel] [PATCH]lavc/mjpegdec: Support pixel format 0x41211100
211757 N   0328  0:35 Carl Eugen Hoyo (4,0K) [FFmpeg-devel] [PATCH]lavc/mjpegdec: Support pixel format 0x11412100

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

No snowflake in an avalanche ever feels responsible. -- Voltaire

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v3 3/3] fate: add tests for RGB jpegs

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 1033 bytes --]

On Wed, Apr 19, 2023 at 02:11:26PM -0400, Leo Izen wrote:
> Added framecrc tests for RGB JPEGs to verify future changes to the
> decoder.
> ---
>  tests/fate/image.mak               | 9 +++++++++
>  tests/ref/fate/jpg-rgb-221         | 6 ++++++
>  tests/ref/fate/jpg-rgb-baseline    | 6 ++++++
>  tests/ref/fate/jpg-rgb-progressive | 6 ++++++
>  4 files changed, 27 insertions(+)
>  create mode 100644 tests/ref/fate/jpg-rgb-221
>  create mode 100644 tests/ref/fate/jpg-rgb-baseline
>  create mode 100644 tests/ref/fate/jpg-rgb-progressive

more tests, LGTM

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Modern terrorism, a quick summary: Need oil, start war with country that
has oil, kill hundread thousand in war. Let country fall into chaos,
be surprised about raise of fundamantalists. Drop more bombs, kill more
people, be surprised about them taking revenge and drop even more bombs
and strip your own citizens of their rights and freedoms. to be continued

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v3 1/3] avcodec/mjpegdec: fix non-subsampled RGB JPEGs

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 1335 bytes --]

On Wed, Apr 19, 2023 at 02:11:24PM -0400, Leo Izen wrote:
> The change introduced in b18a9c29713abc3a1b081de3f320ab53a47120c6
> created a regression for non-subsampled progressive RGB jpegs. This
> should fix that.
> ---
>  libavcodec/mjpegdec.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c
> index 01537d4774..1e3ddb72fb 100644
> --- a/libavcodec/mjpegdec.c
> +++ b/libavcodec/mjpegdec.c
> @@ -1698,7 +1698,8 @@ int ff_mjpeg_decode_sos(MJpegDecodeContext *s, const uint8_t *mb_bitmask,
>          s->h_scount[i]  = s->h_count[index];
>          s->v_scount[i]  = s->v_count[index];
>  
> -        if(nb_components == 3 && s->nb_components == 3 && s->avctx->pix_fmt == AV_PIX_FMT_GBRP)
> +        if((nb_components == 3 || nb_components == 1) && s->nb_components == 3
> +                && s->avctx->pix_fmt == AV_PIX_FMT_GBRP && !s->progressive)
>              index = (index+2)%3;

Why is progressive/!progressive special cased in all the new RGB code ?

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Whats the most studid thing your enemy could do ? Blow himself up
Whats the most studid thing you could do ? Give up your rights and
freedom because your enemy blew himself up.


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v3 1/3] avcodec/mjpegdec: fix non-subsampled RGB JPEGs

[Leo Izen]

On 4/19/23 14:58, Michael Niedermayer wrote:
> On Wed, Apr 19, 2023 at 02:11:24PM -0400, Leo Izen wrote:
>> The change introduced in b18a9c29713abc3a1b081de3f320ab53a47120c6
>> created a regression for non-subsampled progressive RGB jpegs. This
>> should fix that.
>> ---
>>   libavcodec/mjpegdec.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c
>> index 01537d4774..1e3ddb72fb 100644
>> --- a/libavcodec/mjpegdec.c
>> +++ b/libavcodec/mjpegdec.c
>> @@ -1698,7 +1698,8 @@ int ff_mjpeg_decode_sos(MJpegDecodeContext *s, const uint8_t *mb_bitmask,
>>           s->h_scount[i]  = s->h_count[index];
>>           s->v_scount[i]  = s->v_count[index];
>>   
>> -        if(nb_components == 3 && s->nb_components == 3 && s->avctx->pix_fmt == AV_PIX_FMT_GBRP)
>> +        if((nb_components == 3 || nb_components == 1) && s->nb_components == 3
>> +                && s->avctx->pix_fmt == AV_PIX_FMT_GBRP && !s->progressive)
>>               index = (index+2)%3;
> 
> Why is progressive/!progressive special cased in all the new RGB code ?
> 

With progressive, I decode RGB in RGB-order, and then pivot it into 
GBR-order, whereas baseline is just decoded directly into GBR-order. If 
you decode progressive directly in GBR-order the buffers will be the 
wrong size and it will overrun the subsampled buffer when filling it 
with a non-subsampled one. See the allocation block on line 766 of 
mjpegdec.c. This depends on h_count and v_count, which cannot be changed 
or pivoted as if you do so, progressive JPEGs will fail to decode at all 
(invalid VLC entries, etc.)

Ideally, you'd just alloc them the right size, but s->component_index[i] 
won't refer to the right index for many progressive files, depending on 
whether the SOS marker has 1 or 3 components. If you have SOS markers 
with one component it will not properly pivot the colors.

Initially, I didn't have the checks and just always decoded in RGB order 
and then pivoted, but that broke some baseline files like the ones in 
Trac #4045. I used some casework so I could handle all files I tested 
with this.

If anyone has any suggestions on how to make the casework more elegant 
I'm all ears but this is the solution I found to work with every sample 
I tested.

- Leo Izen (Traneptora/thebombzen)

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v3 1/3] avcodec/mjpegdec: fix non-subsampled RGB JPEGs

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 3689 bytes --]

On Wed, Apr 19, 2023 at 03:23:41PM -0400, Leo Izen wrote:
> On 4/19/23 14:58, Michael Niedermayer wrote:
> > On Wed, Apr 19, 2023 at 02:11:24PM -0400, Leo Izen wrote:
> > > The change introduced in b18a9c29713abc3a1b081de3f320ab53a47120c6
> > > created a regression for non-subsampled progressive RGB jpegs. This
> > > should fix that.
> > > ---
> > >   libavcodec/mjpegdec.c | 3 ++-
> > >   1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c
> > > index 01537d4774..1e3ddb72fb 100644
> > > --- a/libavcodec/mjpegdec.c
> > > +++ b/libavcodec/mjpegdec.c
> > > @@ -1698,7 +1698,8 @@ int ff_mjpeg_decode_sos(MJpegDecodeContext *s, const uint8_t *mb_bitmask,
> > >           s->h_scount[i]  = s->h_count[index];
> > >           s->v_scount[i]  = s->v_count[index];
> > > -        if(nb_components == 3 && s->nb_components == 3 && s->avctx->pix_fmt == AV_PIX_FMT_GBRP)
> > > +        if((nb_components == 3 || nb_components == 1) && s->nb_components == 3
> > > +                && s->avctx->pix_fmt == AV_PIX_FMT_GBRP && !s->progressive)
> > >               index = (index+2)%3;
> > 
> > Why is progressive/!progressive special cased in all the new RGB code ?
> > 
> 
> With progressive, I decode RGB in RGB-order, and then pivot it into
> GBR-order, whereas baseline is just decoded directly into GBR-order. If you
> decode progressive directly in GBR-order the buffers will be the wrong size
> and it will overrun the subsampled buffer when filling it with a
> non-subsampled one. See the allocation block on line 766 of mjpegdec.c. This
> depends on h_count and v_count, which cannot be changed or pivoted as if you
> do so, progressive JPEGs will fail to decode at all (invalid VLC entries,
> etc.)
> 
> Ideally, you'd just alloc them the right size, but s->component_index[i]
> won't refer to the right index for many progressive files, depending on
> whether the SOS marker has 1 or 3 components. If you have SOS markers with
> one component it will not properly pivot the colors.
> 
> Initially, I didn't have the checks and just always decoded in RGB order and
> then pivoted, but that broke some baseline files like the ones in Trac
> #4045. I used some casework so I could handle all files I tested with this.
> 
> If anyone has any suggestions on how to make the casework more elegant I'm
> all ears but this is the solution I found to work with every sample I
> tested.

First i would document which array is in PIXFMT vs. JPEG order
when anything is in 2 different orders at different points or for
different cases thats probably not a good idea.
But even if such bad cases exist, it should be documented

progressive is complicated because one could argue that it needs
to be possible to both add pieces into the image and also to
make these pieces immedeatly available to the user so some
application could present to the user the image as it is 
"progressing". Ok we maybe dont care for that feature but its
still not a bad way to look at the problem.
I presume all jpeg streams can be decoded without too much problems
if everything is in jpeg order.
at the same time to present it we need planes to be scaled and
ordered into a standard RGB/GBR/YUV form.
I think these 2 worlds JPEG vs presentation should be more clearly
seperated,

am i seeing the issue correctly or am i missing the problems here ?

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Freedom in capitalist society always remains about the same as it was in
ancient Greek republics: Freedom for slave owners. -- Vladimir Lenin

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v3 1/3] avcodec/mjpegdec: fix non-subsampled RGB JPEGs

[Leo Izen]

On 4/19/23 16:37, Michael Niedermayer wrote:
> On Wed, Apr 19, 2023 at 03:23:41PM -0400, Leo Izen wrote:
>> On 4/19/23 14:58, Michael Niedermayer wrote:
>>> On Wed, Apr 19, 2023 at 02:11:24PM -0400, Leo Izen wrote:
>>>> The change introduced in b18a9c29713abc3a1b081de3f320ab53a47120c6
>>>> created a regression for non-subsampled progressive RGB jpegs. This
>>>> should fix that.
>>>> ---
>>>>    libavcodec/mjpegdec.c | 3 ++-
>>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c
>>>> index 01537d4774..1e3ddb72fb 100644
>>>> --- a/libavcodec/mjpegdec.c
>>>> +++ b/libavcodec/mjpegdec.c
>>>> @@ -1698,7 +1698,8 @@ int ff_mjpeg_decode_sos(MJpegDecodeContext *s, const uint8_t *mb_bitmask,
>>>>            s->h_scount[i]  = s->h_count[index];
>>>>            s->v_scount[i]  = s->v_count[index];
>>>> -        if(nb_components == 3 && s->nb_components == 3 && s->avctx->pix_fmt == AV_PIX_FMT_GBRP)
>>>> +        if((nb_components == 3 || nb_components == 1) && s->nb_components == 3
>>>> +                && s->avctx->pix_fmt == AV_PIX_FMT_GBRP && !s->progressive)
>>>>                index = (index+2)%3;
>>>
>>> Why is progressive/!progressive special cased in all the new RGB code ?
>>>
>>
>> With progressive, I decode RGB in RGB-order, and then pivot it into
>> GBR-order, whereas baseline is just decoded directly into GBR-order. If you
>> decode progressive directly in GBR-order the buffers will be the wrong size
>> and it will overrun the subsampled buffer when filling it with a
>> non-subsampled one. See the allocation block on line 766 of mjpegdec.c. This
>> depends on h_count and v_count, which cannot be changed or pivoted as if you
>> do so, progressive JPEGs will fail to decode at all (invalid VLC entries,
>> etc.)
>>
>> Ideally, you'd just alloc them the right size, but s->component_index[i]
>> won't refer to the right index for many progressive files, depending on
>> whether the SOS marker has 1 or 3 components. If you have SOS markers with
>> one component it will not properly pivot the colors.
>>
>> Initially, I didn't have the checks and just always decoded in RGB order and
>> then pivoted, but that broke some baseline files like the ones in Trac
>> #4045. I used some casework so I could handle all files I tested with this.
>>
>> If anyone has any suggestions on how to make the casework more elegant I'm
>> all ears but this is the solution I found to work with every sample I
>> tested.
> 
> First i would document which array is in PIXFMT vs. JPEG order
> when anything is in 2 different orders at different points or for
> different cases thats probably not a good idea.
> But even if such bad cases exist, it should be documented
> 
> progressive is complicated because one could argue that it needs
> to be possible to both add pieces into the image and also to
> make these pieces immedeatly available to the user so some
> application could present to the user the image as it is
> "progressing". Ok we maybe dont care for that feature but its
> still not a bad way to look at the problem.
> I presume all jpeg streams can be decoded without too much problems
> if everything is in jpeg order.
> at the same time to present it we need planes to be scaled and
> ordered into a standard RGB/GBR/YUV form.
> I think these 2 worlds JPEG vs presentation should be more clearly
> seperated,
> 
> am i seeing the issue correctly or am i missing the problems here ?
> 
> thx

I could try to see if I can decode *every* image in JPEG order and then 
pivot the planes from RGB to GBR order at the end, but it might take me 
a bit more time to figure it out. I'll take a look at it this week. It 
would be a more elegant solution and wouldn't require us to document 
which planes go where in which places of the code.

- Leo Izen

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v3 1/3] avcodec/mjpegdec: fix non-subsampled RGB JPEGs

[Caleb Etemesi]

if  the problem you are trying to solve is upsampling, the agreed samples
are usually 422,444 and 420 (Y,Cb,Cr order) but there can be files with
sampling factors like 242, since the jfif spec isn't to restrictive on what
sampling factors are allowed,libjpeg(and it's cousin libjpeg-turbo) support
such factors, so ideally the way to treat such things is on a channel to
channel basis wnd figure out which ones need to be upsampled(and check if
there is code relying on the Y channel not being upsampled)

Note that I haven't looked into the code, just met with a similar problem
sometime ago hence may be wrong.

On Thu, 20 Apr 2023, 00:15 Leo Izen, <leo.izen@gmail.com> wrote:

> On 4/19/23 16:37, Michael Niedermayer wrote:
> > On Wed, Apr 19, 2023 at 03:23:41PM -0400, Leo Izen wrote:
> >> On 4/19/23 14:58, Michael Niedermayer wrote:
> >>> On Wed, Apr 19, 2023 at 02:11:24PM -0400, Leo Izen wrote:
> >>>> The change introduced in b18a9c29713abc3a1b081de3f320ab53a47120c6
> >>>> created a regression for non-subsampled progressive RGB jpegs. This
> >>>> should fix that.
> >>>> ---
> >>>>    libavcodec/mjpegdec.c | 3 ++-
> >>>>    1 file changed, 2 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c
> >>>> index 01537d4774..1e3ddb72fb 100644
> >>>> --- a/libavcodec/mjpegdec.c
> >>>> +++ b/libavcodec/mjpegdec.c
> >>>> @@ -1698,7 +1698,8 @@ int ff_mjpeg_decode_sos(MJpegDecodeContext *s,
> const uint8_t *mb_bitmask,
> >>>>            s->h_scount[i]  = s->h_count[index];
> >>>>            s->v_scount[i]  = s->v_count[index];
> >>>> -        if(nb_components == 3 && s->nb_components == 3 &&
> s->avctx->pix_fmt == AV_PIX_FMT_GBRP)
> >>>> +        if((nb_components == 3 || nb_components == 1) &&
> s->nb_components == 3
> >>>> +                && s->avctx->pix_fmt == AV_PIX_FMT_GBRP &&
> !s->progressive)
> >>>>                index = (index+2)%3;
> >>>
> >>> Why is progressive/!progressive special cased in all the new RGB code ?
> >>>
> >>
> >> With progressive, I decode RGB in RGB-order, and then pivot it into
> >> GBR-order, whereas baseline is just decoded directly into GBR-order. If
> you
> >> decode progressive directly in GBR-order the buffers will be the wrong
> size
> >> and it will overrun the subsampled buffer when filling it with a
> >> non-subsampled one. See the allocation block on line 766 of mjpegdec.c.
> This
> >> depends on h_count and v_count, which cannot be changed or pivoted as
> if you
> >> do so, progressive JPEGs will fail to decode at all (invalid VLC
> entries,
> >> etc.)
> >>
> >> Ideally, you'd just alloc them the right size, but s->component_index[i]
> >> won't refer to the right index for many progressive files, depending on
> >> whether the SOS marker has 1 or 3 components. If you have SOS markers
> with
> >> one component it will not properly pivot the colors.
> >>
> >> Initially, I didn't have the checks and just always decoded in RGB
> order and
> >> then pivoted, but that broke some baseline files like the ones in Trac
> >> #4045. I used some casework so I could handle all files I tested with
> this.
> >>
> >> If anyone has any suggestions on how to make the casework more elegant
> I'm
> >> all ears but this is the solution I found to work with every sample I
> >> tested.
> >
> > First i would document which array is in PIXFMT vs. JPEG order
> > when anything is in 2 different orders at different points or for
> > different cases thats probably not a good idea.
> > But even if such bad cases exist, it should be documented
> >
> > progressive is complicated because one could argue that it needs
> > to be possible to both add pieces into the image and also to
> > make these pieces immedeatly available to the user so some
> > application could present to the user the image as it is
> > "progressing". Ok we maybe dont care for that feature but its
> > still not a bad way to look at the problem.
> > I presume all jpeg streams can be decoded without too much problems
> > if everything is in jpeg order.
> > at the same time to present it we need planes to be scaled and
> > ordered into a standard RGB/GBR/YUV form.
> > I think these 2 worlds JPEG vs presentation should be more clearly
> > seperated,
> >
> > am i seeing the issue correctly or am i missing the problems here ?
> >
> > thx
>
> I could try to see if I can decode *every* image in JPEG order and then
> pivot the planes from RGB to GBR order at the end, but it might take me
> a bit more time to figure it out. I'll take a look at it this week. It
> would be a more elegant solution and wouldn't require us to document
> which planes go where in which places of the code.
>
> - Leo Izen
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v3 1/3] avcodec/mjpegdec: fix non-subsampled RGB JPEGs

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 4591 bytes --]

On Wed, Apr 19, 2023 at 05:15:00PM -0400, Leo Izen wrote:
> On 4/19/23 16:37, Michael Niedermayer wrote:
> > On Wed, Apr 19, 2023 at 03:23:41PM -0400, Leo Izen wrote:
> > > On 4/19/23 14:58, Michael Niedermayer wrote:
> > > > On Wed, Apr 19, 2023 at 02:11:24PM -0400, Leo Izen wrote:
> > > > > The change introduced in b18a9c29713abc3a1b081de3f320ab53a47120c6
> > > > > created a regression for non-subsampled progressive RGB jpegs. This
> > > > > should fix that.
> > > > > ---
> > > > >    libavcodec/mjpegdec.c | 3 ++-
> > > > >    1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c
> > > > > index 01537d4774..1e3ddb72fb 100644
> > > > > --- a/libavcodec/mjpegdec.c
> > > > > +++ b/libavcodec/mjpegdec.c
> > > > > @@ -1698,7 +1698,8 @@ int ff_mjpeg_decode_sos(MJpegDecodeContext *s, const uint8_t *mb_bitmask,
> > > > >            s->h_scount[i]  = s->h_count[index];
> > > > >            s->v_scount[i]  = s->v_count[index];
> > > > > -        if(nb_components == 3 && s->nb_components == 3 && s->avctx->pix_fmt == AV_PIX_FMT_GBRP)
> > > > > +        if((nb_components == 3 || nb_components == 1) && s->nb_components == 3
> > > > > +                && s->avctx->pix_fmt == AV_PIX_FMT_GBRP && !s->progressive)
> > > > >                index = (index+2)%3;
> > > > 
> > > > Why is progressive/!progressive special cased in all the new RGB code ?
> > > > 
> > > 
> > > With progressive, I decode RGB in RGB-order, and then pivot it into
> > > GBR-order, whereas baseline is just decoded directly into GBR-order. If you
> > > decode progressive directly in GBR-order the buffers will be the wrong size
> > > and it will overrun the subsampled buffer when filling it with a
> > > non-subsampled one. See the allocation block on line 766 of mjpegdec.c. This
> > > depends on h_count and v_count, which cannot be changed or pivoted as if you
> > > do so, progressive JPEGs will fail to decode at all (invalid VLC entries,
> > > etc.)
> > > 
> > > Ideally, you'd just alloc them the right size, but s->component_index[i]
> > > won't refer to the right index for many progressive files, depending on
> > > whether the SOS marker has 1 or 3 components. If you have SOS markers with
> > > one component it will not properly pivot the colors.
> > > 
> > > Initially, I didn't have the checks and just always decoded in RGB order and
> > > then pivoted, but that broke some baseline files like the ones in Trac
> > > #4045. I used some casework so I could handle all files I tested with this.
> > > 
> > > If anyone has any suggestions on how to make the casework more elegant I'm
> > > all ears but this is the solution I found to work with every sample I
> > > tested.
> > 
> > First i would document which array is in PIXFMT vs. JPEG order
> > when anything is in 2 different orders at different points or for
> > different cases thats probably not a good idea.
> > But even if such bad cases exist, it should be documented
> > 
> > progressive is complicated because one could argue that it needs
> > to be possible to both add pieces into the image and also to
> > make these pieces immedeatly available to the user so some
> > application could present to the user the image as it is
> > "progressing". Ok we maybe dont care for that feature but its
> > still not a bad way to look at the problem.
> > I presume all jpeg streams can be decoded without too much problems
> > if everything is in jpeg order.
> > at the same time to present it we need planes to be scaled and
> > ordered into a standard RGB/GBR/YUV form.
> > I think these 2 worlds JPEG vs presentation should be more clearly
> > seperated,
> > 
> > am i seeing the issue correctly or am i missing the problems here ?
> > 
> > thx
> 
> I could try to see if I can decode *every* image in JPEG order and then
> pivot the planes from RGB to GBR order at the end, but it might take me a
> bit more time to figure it out. I'll take a look at it this week. It would
> be a more elegant solution and wouldn't require us to document which planes
> go where in which places of the code.

You might run into problems with user provided buffers if teh ordering is
entirely done at the end. because when decoding lets say red it should be in
the red plane. The user app might not expect its planes swapped

[...]

thx

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I have often repented speaking, but never of holding my tongue.
-- Xenocrates

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v3 1/3] avcodec/mjpegdec: fix non-subsampled RGB JPEGs

[Leo Izen]

On 4/20/23 05:50, Michael Niedermayer wrote:
> On Wed, Apr 19, 2023 at 05:15:00PM -0400, Leo Izen wrote:
>> On 4/19/23 16:37, Michael Niedermayer wrote:
>>> On Wed, Apr 19, 2023 at 03:23:41PM -0400, Leo Izen wrote:
>>>> On 4/19/23 14:58, Michael Niedermayer wrote:
>>>>> On Wed, Apr 19, 2023 at 02:11:24PM -0400, Leo Izen wrote:
>>>>>> The change introduced in b18a9c29713abc3a1b081de3f320ab53a47120c6
>>>>>> created a regression for non-subsampled progressive RGB jpegs. This
>>>>>> should fix that.
>>>>>> ---
>>>>>>     libavcodec/mjpegdec.c | 3 ++-
>>>>>>     1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c
>>>>>> index 01537d4774..1e3ddb72fb 100644
>>>>>> --- a/libavcodec/mjpegdec.c
>>>>>> +++ b/libavcodec/mjpegdec.c
>>>>>> @@ -1698,7 +1698,8 @@ int ff_mjpeg_decode_sos(MJpegDecodeContext *s, const uint8_t *mb_bitmask,
>>>>>>             s->h_scount[i]  = s->h_count[index];
>>>>>>             s->v_scount[i]  = s->v_count[index];
>>>>>> -        if(nb_components == 3 && s->nb_components == 3 && s->avctx->pix_fmt == AV_PIX_FMT_GBRP)
>>>>>> +        if((nb_components == 3 || nb_components == 1) && s->nb_components == 3
>>>>>> +                && s->avctx->pix_fmt == AV_PIX_FMT_GBRP && !s->progressive)
>>>>>>                 index = (index+2)%3;
>>>>>
>>>>> Why is progressive/!progressive special cased in all the new RGB code ?
>>>>>
>>>>
>>>> With progressive, I decode RGB in RGB-order, and then pivot it into
>>>> GBR-order, whereas baseline is just decoded directly into GBR-order. If you
>>>> decode progressive directly in GBR-order the buffers will be the wrong size
>>>> and it will overrun the subsampled buffer when filling it with a
>>>> non-subsampled one. See the allocation block on line 766 of mjpegdec.c. This
>>>> depends on h_count and v_count, which cannot be changed or pivoted as if you
>>>> do so, progressive JPEGs will fail to decode at all (invalid VLC entries,
>>>> etc.)
>>>>
>>>> Ideally, you'd just alloc them the right size, but s->component_index[i]
>>>> won't refer to the right index for many progressive files, depending on
>>>> whether the SOS marker has 1 or 3 components. If you have SOS markers with
>>>> one component it will not properly pivot the colors.
>>>>
>>>> Initially, I didn't have the checks and just always decoded in RGB order and
>>>> then pivoted, but that broke some baseline files like the ones in Trac
>>>> #4045. I used some casework so I could handle all files I tested with this.
>>>>
>>>> If anyone has any suggestions on how to make the casework more elegant I'm
>>>> all ears but this is the solution I found to work with every sample I
>>>> tested.
>>>
>>> First i would document which array is in PIXFMT vs. JPEG order
>>> when anything is in 2 different orders at different points or for
>>> different cases thats probably not a good idea.
>>> But even if such bad cases exist, it should be documented
>>>
>>> progressive is complicated because one could argue that it needs
>>> to be possible to both add pieces into the image and also to
>>> make these pieces immedeatly available to the user so some
>>> application could present to the user the image as it is
>>> "progressing". Ok we maybe dont care for that feature but its
>>> still not a bad way to look at the problem.
>>> I presume all jpeg streams can be decoded without too much problems
>>> if everything is in jpeg order.
>>> at the same time to present it we need planes to be scaled and
>>> ordered into a standard RGB/GBR/YUV form.
>>> I think these 2 worlds JPEG vs presentation should be more clearly
>>> seperated,
>>>
>>> am i seeing the issue correctly or am i missing the problems here ?
>>>
>>> thx
>>
>> I could try to see if I can decode *every* image in JPEG order and then
>> pivot the planes from RGB to GBR order at the end, but it might take me a
>> bit more time to figure it out. I'll take a look at it this week. It would
>> be a more elegant solution and wouldn't require us to document which planes
>> go where in which places of the code.
> 
> You might run into problems with user provided buffers if teh ordering is
> entirely done at the end. because when decoding lets say red it should be in
> the red plane. The user app might not expect its planes swapped
> 

As far as I'm aware, I should be able to just rotate the AVFrame->data 
pointers and AVFrame->linesize values, as we don't promise these are 
ordered in the same order as the AVBufferRefs.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".