diff mbox series

[FFmpeg-devel,1/2] libavcodec/jpeg2000dec: Enhance pix fmt selection

Message ID 20200701183423.21335-1-gautamramk@gmail.com
State Superseded
Headers show
Series [FFmpeg-devel,1/2] libavcodec/jpeg2000dec: Enhance pix fmt selection
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Gautam Ramakrishnan July 1, 2020, 6:34 p.m. UTC
From: Gautam Ramakrishnan <gautamramk@gmail.com>

This patch assigns default pix format values when
a match does not take place.
---
 libavcodec/jpeg2000dec.c | 9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Carl Eugen Hoyos July 1, 2020, 9:42 p.m. UTC | #1
Am Mi., 1. Juli 2020 um 20:34 Uhr schrieb <gautamramk@gmail.com>:
>
> From: Gautam Ramakrishnan <gautamramk@gmail.com>
>
> This patch assigns default pix format values when
> a match does not take place.
> ---
>  libavcodec/jpeg2000dec.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/libavcodec/jpeg2000dec.c b/libavcodec/jpeg2000dec.c
> index 3f4a9ef96c..86f9170723 100644
> --- a/libavcodec/jpeg2000dec.c
> +++ b/libavcodec/jpeg2000dec.c
> @@ -436,6 +436,15 @@ static int get_siz(Jpeg2000DecoderContext *s)
>                  s->cdef[3] = 3;
>                  i = 0;
>              }
> +        } else if (ncomponents == 3 && s->precision == 8) {
> +            s->avctx->pix_fmt = AV_PIX_FMT_RGB24;
> +            i = 0;
> +        } else if (ncomponents == 2 && s->precision == 8) {
> +            s->avctx->pix_fmt = AV_PIX_FMT_YA8;
> +            i = 0;

Which samples does this fix / why is this a good idea?

Carl Eugen
Gautam Ramakrishnan July 2, 2020, 3:10 a.m. UTC | #2
On Thu, Jul 2, 2020 at 3:42 AM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>
> Am Mi., 1. Juli 2020 um 20:34 Uhr schrieb <gautamramk@gmail.com>:
> >
> > From: Gautam Ramakrishnan <gautamramk@gmail.com>
> >
> > This patch assigns default pix format values when
> > a match does not take place.
> > ---
> >  libavcodec/jpeg2000dec.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/libavcodec/jpeg2000dec.c b/libavcodec/jpeg2000dec.c
> > index 3f4a9ef96c..86f9170723 100644
> > --- a/libavcodec/jpeg2000dec.c
> > +++ b/libavcodec/jpeg2000dec.c
> > @@ -436,6 +436,15 @@ static int get_siz(Jpeg2000DecoderContext *s)
> >                  s->cdef[3] = 3;
> >                  i = 0;
> >              }
> > +        } else if (ncomponents == 3 && s->precision == 8) {
> > +            s->avctx->pix_fmt = AV_PIX_FMT_RGB24;
> > +            i = 0;
> > +        } else if (ncomponents == 2 && s->precision == 8) {
> > +            s->avctx->pix_fmt = AV_PIX_FMT_YA8;
> > +            i = 0;
>
> Which samples does this fix / why is this a good idea?
Sorry I missed out on this. This is for samples p0_10.j2k,
p1_01.j2k and p1_07.j2k. I finally fully understood what
pix_fmt_match() was doing and realized a previous patch,
which removes a check based on log2_chroma_wh was a bad
idea.I felt that pix_fmt_match was rejecting a few samples just
because the subsampling rate of a few images was not 1, as the
pix_fmt_match() function rejects rgb24, gray8 and ya8 if the sampling
was not 1. This patch adds a default pixel format in these cases.
Michael Niedermayer July 2, 2020, 3 p.m. UTC | #3
On Thu, Jul 02, 2020 at 12:04:22AM +0530, gautamramk@gmail.com wrote:
> From: Gautam Ramakrishnan <gautamramk@gmail.com>
> 
> This patch assigns default pix format values when
> a match does not take place.
> ---
>  libavcodec/jpeg2000dec.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/libavcodec/jpeg2000dec.c b/libavcodec/jpeg2000dec.c
> index 3f4a9ef96c..86f9170723 100644
> --- a/libavcodec/jpeg2000dec.c
> +++ b/libavcodec/jpeg2000dec.c
> @@ -436,6 +436,15 @@ static int get_siz(Jpeg2000DecoderContext *s)
>                  s->cdef[3] = 3;
>                  i = 0;
>              }
> +        } else if (ncomponents == 3 && s->precision == 8) {
> +            s->avctx->pix_fmt = AV_PIX_FMT_RGB24;
> +            i = 0;
> +        } else if (ncomponents == 2 && s->precision == 8) {
> +            s->avctx->pix_fmt = AV_PIX_FMT_YA8;
> +            i = 0;

I dont think 2 and 3 component formats can ignore the subsampling
of the planes.
They have to match even if they are not 1
if they mismatch the differing plane would need to be rescaled

Thats unless iam missing something, which is possible

thx

[...]
Gautam Ramakrishnan July 2, 2020, 7:35 p.m. UTC | #4
On Thu, Jul 2, 2020 at 8:30 PM Michael Niedermayer
<michael@niedermayer.cc> wrote:
>
> On Thu, Jul 02, 2020 at 12:04:22AM +0530, gautamramk@gmail.com wrote:
> > From: Gautam Ramakrishnan <gautamramk@gmail.com>
> >
> > This patch assigns default pix format values when
> > a match does not take place.
> > ---
> >  libavcodec/jpeg2000dec.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/libavcodec/jpeg2000dec.c b/libavcodec/jpeg2000dec.c
> > index 3f4a9ef96c..86f9170723 100644
> > --- a/libavcodec/jpeg2000dec.c
> > +++ b/libavcodec/jpeg2000dec.c
> > @@ -436,6 +436,15 @@ static int get_siz(Jpeg2000DecoderContext *s)
> >                  s->cdef[3] = 3;
> >                  i = 0;
> >              }
> > +        } else if (ncomponents == 3 && s->precision == 8) {
> > +            s->avctx->pix_fmt = AV_PIX_FMT_RGB24;
> > +            i = 0;
> > +        } else if (ncomponents == 2 && s->precision == 8) {
> > +            s->avctx->pix_fmt = AV_PIX_FMT_YA8;
> > +            i = 0;
>
> I dont think 2 and 3 component formats can ignore the subsampling
> of the planes.
> They have to match even if they are not 1
> if they mismatch the differing plane would need to be rescaled
>
> Thats unless iam missing something, which is possible
I do not have a very good understanding of colour formats. But,
I checked out p1_07.j2k, which is a 2 component image. The subsampling
of the 2 layers are different. As a result, both components have different
dimensions. As a reference, I decoded the pgx files and saw the output.
It does match with what would happen with this patch applied. However,
that is probably because the samples are designed just to test the working
of the decoder and not to actually account for intricacies of the pixel formats.
As a compromise, do you think if would make sense if this patch can be applied
if the subsampling of all the planes are the same, but not the same as
that of rgb24
or ya8. Atleast, in that case, we will not have differing planes.
In case of YCC formats, it makes sense that every subsampling rate must match.
However, for rgb, from my understanding, it could also mean that the
final image is
just scaled according to the sample separation rate?
Michael Niedermayer July 3, 2020, 6:32 p.m. UTC | #5
On Fri, Jul 03, 2020 at 01:05:12AM +0530, Gautam Ramakrishnan wrote:
> On Thu, Jul 2, 2020 at 8:30 PM Michael Niedermayer
> <michael@niedermayer.cc> wrote:
> >
> > On Thu, Jul 02, 2020 at 12:04:22AM +0530, gautamramk@gmail.com wrote:
> > > From: Gautam Ramakrishnan <gautamramk@gmail.com>
> > >
> > > This patch assigns default pix format values when
> > > a match does not take place.
> > > ---
> > >  libavcodec/jpeg2000dec.c | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > >
> > > diff --git a/libavcodec/jpeg2000dec.c b/libavcodec/jpeg2000dec.c
> > > index 3f4a9ef96c..86f9170723 100644
> > > --- a/libavcodec/jpeg2000dec.c
> > > +++ b/libavcodec/jpeg2000dec.c
> > > @@ -436,6 +436,15 @@ static int get_siz(Jpeg2000DecoderContext *s)
> > >                  s->cdef[3] = 3;
> > >                  i = 0;
> > >              }
> > > +        } else if (ncomponents == 3 && s->precision == 8) {
> > > +            s->avctx->pix_fmt = AV_PIX_FMT_RGB24;
> > > +            i = 0;
> > > +        } else if (ncomponents == 2 && s->precision == 8) {
> > > +            s->avctx->pix_fmt = AV_PIX_FMT_YA8;
> > > +            i = 0;
> >
> > I dont think 2 and 3 component formats can ignore the subsampling
> > of the planes.
> > They have to match even if they are not 1
> > if they mismatch the differing plane would need to be rescaled
> >
> > Thats unless iam missing something, which is possible
> I do not have a very good understanding of colour formats. But,
> I checked out p1_07.j2k, which is a 2 component image. The subsampling
> of the 2 layers are different. As a result, both components have different
> dimensions. As a reference, I decoded the pgx files and saw the output.
> It does match with what would happen with this patch applied. However,
> that is probably because the samples are designed just to test the working
> of the decoder and not to actually account for intricacies of the pixel formats.
> As a compromise, do you think if would make sense if this patch can be applied
> if the subsampling of all the planes are the same, but not the same as
> that of rgb24
> or ya8. Atleast, in that case, we will not have differing planes.
> In case of YCC formats, it makes sense that every subsampling rate must match.
> However, for rgb, from my understanding, it could also mean that the
> final image is
> just scaled according to the sample separation rate?

for RGB the image leaving the decoder each of the R, G and B planes must
have the same size.
for YA the luma and alpha planes similarly must have the same size.

if all planes are considered not subsampled or they are all considered
subsampled by the same amount is the same from the point of view of
the pixel format.
Also the decoder could rescale a single subsampled plane if its needed.
Our jpeg decoder does that for some of these bizare cases
i hope that clarifies it

thx

[...]
Gautam Ramakrishnan July 4, 2020, 5:31 p.m. UTC | #6
On Sat, Jul 4, 2020 at 12:02 AM Michael Niedermayer
<michael@niedermayer.cc> wrote:
>
> On Fri, Jul 03, 2020 at 01:05:12AM +0530, Gautam Ramakrishnan wrote:
> > On Thu, Jul 2, 2020 at 8:30 PM Michael Niedermayer
> > <michael@niedermayer.cc> wrote:
> > >
> > > On Thu, Jul 02, 2020 at 12:04:22AM +0530, gautamramk@gmail.com wrote:
> > > > From: Gautam Ramakrishnan <gautamramk@gmail.com>
> > > >
> > > > This patch assigns default pix format values when
> > > > a match does not take place.
> > > > ---
> > > >  libavcodec/jpeg2000dec.c | 9 +++++++++
> > > >  1 file changed, 9 insertions(+)
> > > >
> > > > diff --git a/libavcodec/jpeg2000dec.c b/libavcodec/jpeg2000dec.c
> > > > index 3f4a9ef96c..86f9170723 100644
> > > > --- a/libavcodec/jpeg2000dec.c
> > > > +++ b/libavcodec/jpeg2000dec.c
> > > > @@ -436,6 +436,15 @@ static int get_siz(Jpeg2000DecoderContext *s)
> > > >                  s->cdef[3] = 3;
> > > >                  i = 0;
> > > >              }
> > > > +        } else if (ncomponents == 3 && s->precision == 8) {
> > > > +            s->avctx->pix_fmt = AV_PIX_FMT_RGB24;
> > > > +            i = 0;
> > > > +        } else if (ncomponents == 2 && s->precision == 8) {
> > > > +            s->avctx->pix_fmt = AV_PIX_FMT_YA8;
> > > > +            i = 0;
> > >
> > > I dont think 2 and 3 component formats can ignore the subsampling
> > > of the planes.
> > > They have to match even if they are not 1
> > > if they mismatch the differing plane would need to be rescaled
> > >
> > > Thats unless iam missing something, which is possible
> > I do not have a very good understanding of colour formats. But,
> > I checked out p1_07.j2k, which is a 2 component image. The subsampling
> > of the 2 layers are different. As a result, both components have different
> > dimensions. As a reference, I decoded the pgx files and saw the output.
> > It does match with what would happen with this patch applied. However,
> > that is probably because the samples are designed just to test the working
> > of the decoder and not to actually account for intricacies of the pixel formats.
> > As a compromise, do you think if would make sense if this patch can be applied
> > if the subsampling of all the planes are the same, but not the same as
> > that of rgb24
> > or ya8. Atleast, in that case, we will not have differing planes.
> > In case of YCC formats, it makes sense that every subsampling rate must match.
> > However, for rgb, from my understanding, it could also mean that the
> > final image is
> > just scaled according to the sample separation rate?
>
> for RGB the image leaving the decoder each of the R, G and B planes must
> have the same size.
> for YA the luma and alpha planes similarly must have the same size.
>
> if all planes are considered not subsampled or they are all considered
> subsampled by the same amount is the same from the point of view of
> the pixel format.
It is clear. I shall add a check that ensures that the subsampling of all the
planes are the same.
> Also the decoder could rescale a single subsampled plane if its needed.
> Our jpeg decoder does that for some of these bizare cases
I did not understand which cases are referred here. This is important because
this is very relevant to the p1_07.j2k reference image.
> i hope that clarifies it
>
> thx
Michael Niedermayer July 4, 2020, 6:30 p.m. UTC | #7
On Sat, Jul 04, 2020 at 11:01:14PM +0530, Gautam Ramakrishnan wrote:
> On Sat, Jul 4, 2020 at 12:02 AM Michael Niedermayer
> <michael@niedermayer.cc> wrote:
> >
> > On Fri, Jul 03, 2020 at 01:05:12AM +0530, Gautam Ramakrishnan wrote:
> > > On Thu, Jul 2, 2020 at 8:30 PM Michael Niedermayer
> > > <michael@niedermayer.cc> wrote:
> > > >
> > > > On Thu, Jul 02, 2020 at 12:04:22AM +0530, gautamramk@gmail.com wrote:
> > > > > From: Gautam Ramakrishnan <gautamramk@gmail.com>
> > > > >
> > > > > This patch assigns default pix format values when
> > > > > a match does not take place.
> > > > > ---
> > > > >  libavcodec/jpeg2000dec.c | 9 +++++++++
> > > > >  1 file changed, 9 insertions(+)
> > > > >
> > > > > diff --git a/libavcodec/jpeg2000dec.c b/libavcodec/jpeg2000dec.c
> > > > > index 3f4a9ef96c..86f9170723 100644
> > > > > --- a/libavcodec/jpeg2000dec.c
> > > > > +++ b/libavcodec/jpeg2000dec.c
> > > > > @@ -436,6 +436,15 @@ static int get_siz(Jpeg2000DecoderContext *s)
> > > > >                  s->cdef[3] = 3;
> > > > >                  i = 0;
> > > > >              }
> > > > > +        } else if (ncomponents == 3 && s->precision == 8) {
> > > > > +            s->avctx->pix_fmt = AV_PIX_FMT_RGB24;
> > > > > +            i = 0;
> > > > > +        } else if (ncomponents == 2 && s->precision == 8) {
> > > > > +            s->avctx->pix_fmt = AV_PIX_FMT_YA8;
> > > > > +            i = 0;
> > > >
> > > > I dont think 2 and 3 component formats can ignore the subsampling
> > > > of the planes.
> > > > They have to match even if they are not 1
> > > > if they mismatch the differing plane would need to be rescaled
> > > >
> > > > Thats unless iam missing something, which is possible
> > > I do not have a very good understanding of colour formats. But,
> > > I checked out p1_07.j2k, which is a 2 component image. The subsampling
> > > of the 2 layers are different. As a result, both components have different
> > > dimensions. As a reference, I decoded the pgx files and saw the output.
> > > It does match with what would happen with this patch applied. However,
> > > that is probably because the samples are designed just to test the working
> > > of the decoder and not to actually account for intricacies of the pixel formats.
> > > As a compromise, do you think if would make sense if this patch can be applied
> > > if the subsampling of all the planes are the same, but not the same as
> > > that of rgb24
> > > or ya8. Atleast, in that case, we will not have differing planes.
> > > In case of YCC formats, it makes sense that every subsampling rate must match.
> > > However, for rgb, from my understanding, it could also mean that the
> > > final image is
> > > just scaled according to the sample separation rate?
> >
> > for RGB the image leaving the decoder each of the R, G and B planes must
> > have the same size.
> > for YA the luma and alpha planes similarly must have the same size.
> >
> > if all planes are considered not subsampled or they are all considered
> > subsampled by the same amount is the same from the point of view of
> > the pixel format.
> It is clear. I shall add a check that ensures that the subsampling of all the
> planes are the same.

> > Also the decoder could rescale a single subsampled plane if its needed.
> > Our jpeg decoder does that for some of these bizare cases
> I did not understand which cases are referred here. This is important because
> this is very relevant to the p1_07.j2k reference image.

I refered to something ive seen in plain jpeg
files which contain some YCbCr format but the Cb and Cr planes use each
different subsampling, to support this one plane is rescaled in the decoder.

Iam suspecting something similar could be occuring in jpeg2000 but i did not
check

[...]
Gautam Ramakrishnan July 4, 2020, 6:59 p.m. UTC | #8
On Sun, Jul 5, 2020 at 12:00 AM Michael Niedermayer
<michael@niedermayer.cc> wrote:
>
> On Sat, Jul 04, 2020 at 11:01:14PM +0530, Gautam Ramakrishnan wrote:
> > On Sat, Jul 4, 2020 at 12:02 AM Michael Niedermayer
> > <michael@niedermayer.cc> wrote:
> > >
> > > On Fri, Jul 03, 2020 at 01:05:12AM +0530, Gautam Ramakrishnan wrote:
> > > > On Thu, Jul 2, 2020 at 8:30 PM Michael Niedermayer
> > > > <michael@niedermayer.cc> wrote:
> > > > >
> > > > > On Thu, Jul 02, 2020 at 12:04:22AM +0530, gautamramk@gmail.com wrote:
> > > > > > From: Gautam Ramakrishnan <gautamramk@gmail.com>
> > > > > >
> > > > > > This patch assigns default pix format values when
> > > > > > a match does not take place.
> > > > > > ---
> > > > > >  libavcodec/jpeg2000dec.c | 9 +++++++++
> > > > > >  1 file changed, 9 insertions(+)
> > > > > >
> > > > > > diff --git a/libavcodec/jpeg2000dec.c b/libavcodec/jpeg2000dec.c
> > > > > > index 3f4a9ef96c..86f9170723 100644
> > > > > > --- a/libavcodec/jpeg2000dec.c
> > > > > > +++ b/libavcodec/jpeg2000dec.c
> > > > > > @@ -436,6 +436,15 @@ static int get_siz(Jpeg2000DecoderContext *s)
> > > > > >                  s->cdef[3] = 3;
> > > > > >                  i = 0;
> > > > > >              }
> > > > > > +        } else if (ncomponents == 3 && s->precision == 8) {
> > > > > > +            s->avctx->pix_fmt = AV_PIX_FMT_RGB24;
> > > > > > +            i = 0;
> > > > > > +        } else if (ncomponents == 2 && s->precision == 8) {
> > > > > > +            s->avctx->pix_fmt = AV_PIX_FMT_YA8;
> > > > > > +            i = 0;
> > > > >
> > > > > I dont think 2 and 3 component formats can ignore the subsampling
> > > > > of the planes.
> > > > > They have to match even if they are not 1
> > > > > if they mismatch the differing plane would need to be rescaled
> > > > >
> > > > > Thats unless iam missing something, which is possible
> > > > I do not have a very good understanding of colour formats. But,
> > > > I checked out p1_07.j2k, which is a 2 component image. The subsampling
> > > > of the 2 layers are different. As a result, both components have different
> > > > dimensions. As a reference, I decoded the pgx files and saw the output.
> > > > It does match with what would happen with this patch applied. However,
> > > > that is probably because the samples are designed just to test the working
> > > > of the decoder and not to actually account for intricacies of the pixel formats.
> > > > As a compromise, do you think if would make sense if this patch can be applied
> > > > if the subsampling of all the planes are the same, but not the same as
> > > > that of rgb24
> > > > or ya8. Atleast, in that case, we will not have differing planes.
> > > > In case of YCC formats, it makes sense that every subsampling rate must match.
> > > > However, for rgb, from my understanding, it could also mean that the
> > > > final image is
> > > > just scaled according to the sample separation rate?
> > >
> > > for RGB the image leaving the decoder each of the R, G and B planes must
> > > have the same size.
> > > for YA the luma and alpha planes similarly must have the same size.
> > >
> > > if all planes are considered not subsampled or they are all considered
> > > subsampled by the same amount is the same from the point of view of
> > > the pixel format.
> > It is clear. I shall add a check that ensures that the subsampling of all the
> > planes are the same.
>
> > > Also the decoder could rescale a single subsampled plane if its needed.
> > > Our jpeg decoder does that for some of these bizare cases
> > I did not understand which cases are referred here. This is important because
> > this is very relevant to the p1_07.j2k reference image.
>
> I refered to something ive seen in plain jpeg
> files which contain some YCbCr format but the Cb and Cr planes use each
> different subsampling, to support this one plane is rescaled in the decoder.
>
In that case, I'll refer to the jpeg decoder and see if a similar
exception could be made
for the jpeg2000 decoder to support p1_07.j2k.
> Iam suspecting something similar could be occuring in jpeg2000 but i did not
> check
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Let us carefully observe those good qualities wherein our enemies excel us
> and endeavor to excel them, by avoiding what is faulty, and imitating what
> is excellent in them. -- Plutarch
> _______________________________________________
> 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".
diff mbox series

Patch

diff --git a/libavcodec/jpeg2000dec.c b/libavcodec/jpeg2000dec.c
index 3f4a9ef96c..86f9170723 100644
--- a/libavcodec/jpeg2000dec.c
+++ b/libavcodec/jpeg2000dec.c
@@ -436,6 +436,15 @@  static int get_siz(Jpeg2000DecoderContext *s)
                 s->cdef[3] = 3;
                 i = 0;
             }
+        } else if (ncomponents == 3 && s->precision == 8) {
+            s->avctx->pix_fmt = AV_PIX_FMT_RGB24;
+            i = 0;
+        } else if (ncomponents == 2 && s->precision == 8) {
+            s->avctx->pix_fmt = AV_PIX_FMT_YA8;
+            i = 0;
+        } else if (ncomponents == 1 && s->precision == 8) {
+            s->avctx->pix_fmt = AV_PIX_FMT_GRAY8;
+            i = 0;
         }
     }