| Message ID | 20200701183423.21335-1-gautamramk@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [FFmpeg-devel,1/2] libavcodec/jpeg2000dec: Enhance pix fmt selection | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
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
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.
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 [...]
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?
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 [...]
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
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 [...]
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 --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; } }
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(+)