| Message ID | 20200609120653.20078-1-gautamramk@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,RFC] libavcodec/libopenjpeg: pix fmt selection change | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
On Tue, Jun 9, 2020 at 6:07 AM <gautamramk@gmail.com> wrote: > From: Gautam Ramakrishnan <gautamramk@gmail.com> > > This patch makes selection of pix_fmt similar to > that in the native decoder. This makes samples such > as p0_05.j2k and p1_03.j2k decodable by libopenjpeg. > --- > libavcodec/libopenjpegdec.c | 21 +++++++++++++++++++-- > 1 file changed, 19 insertions(+), 2 deletions(-) > > diff --git a/libavcodec/libopenjpegdec.c b/libavcodec/libopenjpegdec.c > index 344c5ba5a3..f5f208784e 100644 > --- a/libavcodec/libopenjpegdec.c > +++ b/libavcodec/libopenjpegdec.c > @@ -272,11 +272,11 @@ static inline void libopenjpeg_copyto8(AVFrame > *picture, opj_image_t *image) { > int *comp_data; > uint8_t *img_ptr; > int index, x, y; > - > for (index = 0; index < image->numcomps; index++) { > + int plane = index?index-1:image->numcomps-1; > Won't this break things for other pictures (e.g., RGB, YUV, etc.)? comp_data = image->comps[index].data; > for (y = 0; y < image->comps[index].h; y++) { > - img_ptr = picture->data[index] + y * picture->linesize[index]; > + img_ptr = picture->data[plane] + y * picture->linesize[plane]; > for (x = 0; x < image->comps[index].w; x++) { > *img_ptr = 0x80 * image->comps[index].sgnd + *comp_data; > img_ptr++; > @@ -408,6 +408,23 @@ static int libopenjpeg_decode_frame(AVCodecContext > *avctx, > if (avctx->pix_fmt == AV_PIX_FMT_NONE) > avctx->pix_fmt = libopenjpeg_guess_pix_fmt(image); > > + if (avctx->pix_fmt == AV_PIX_FMT_NONE) { > + if (image->numcomps == 4 && > + image->comps[0].dx == 1 && image->comps[0].dy == 1 && > + image->comps[1].dx == 1 && image->comps[1].dy == 1 && > + image->comps[2].dx == image->comps[3].dx && > + image->comps[2].dy == image->comps[3].dy) { > + int maxprec = 0; > + for (int i = 0; i < 4; i++) > + maxprec = FFMAX(maxprec, image->comps[i].prec); > + if (maxprec == 8 && > + image->comps[2].dx == 2 && > + image->comps[2].dy == 2) { > + avctx->pix_fmt = AV_PIX_FMT_YUVA420P; > + } > + } > + } > + > Please move this up to libopenjpeg_guess_pix_fmt. Also, are the planes stored in this image stored in AYUV order?
Am Di., 9. Juni 2020 um 14:07 Uhr schrieb <gautamramk@gmail.com>: > > From: Gautam Ramakrishnan <gautamramk@gmail.com> > > This patch makes selection of pix_fmt similar to > that in the native decoder. This makes samples such > as p0_05.j2k and p1_03.j2k decodable by libopenjpeg. Since both files are not YUVA420P, I am not sure if this patch is a good idea, in any case, the commit message mentioning the two files as reasons for this patch is wrong. Carl Eugen
On Tue, Jun 9, 2020 at 10:24 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > > Am Di., 9. Juni 2020 um 14:07 Uhr schrieb <gautamramk@gmail.com>: > > > > From: Gautam Ramakrishnan <gautamramk@gmail.com> > > > > This patch makes selection of pix_fmt similar to > > that in the native decoder. This makes samples such > > as p0_05.j2k and p1_03.j2k decodable by libopenjpeg. > > Since both files are not YUVA420P, I am not sure if this > patch is a good idea, in any case, the commit message > mentioning the two files as reasons for this patch is wrong. > I am not sure what file format this is then. In the native decoder, it was getting recognized as YUVA420P. The native decoder will also have to be modified to account for that then. > Carl Eugen > _______________________________________________ > 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".
Am Di., 9. Juni 2020 um 19:12 Uhr schrieb Gautam Ramakrishnan <gautamramk@gmail.com>: > > On Tue, Jun 9, 2020 at 10:24 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > > > > Am Di., 9. Juni 2020 um 14:07 Uhr schrieb <gautamramk@gmail.com>: > > > > > > From: Gautam Ramakrishnan <gautamramk@gmail.com> > > > > > > This patch makes selection of pix_fmt similar to > > > that in the native decoder. This makes samples such > > > as p0_05.j2k and p1_03.j2k decodable by libopenjpeg. > > > > Since both files are not YUVA420P, I am not sure if this > > patch is a good idea, in any case, the commit message > > mentioning the two files as reasons for this patch is wrong. > > > I am not sure what file format this is then. I failed to find out so far, possibly Bayer, CMYK seems less likely to me. Carl Eugen
On Tue, Jun 9, 2020 at 11:55 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > > Am Di., 9. Juni 2020 um 19:12 Uhr schrieb Gautam Ramakrishnan > <gautamramk@gmail.com>: > > > > On Tue, Jun 9, 2020 at 10:24 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > > > > > > Am Di., 9. Juni 2020 um 14:07 Uhr schrieb <gautamramk@gmail.com>: > > > > > > > > From: Gautam Ramakrishnan <gautamramk@gmail.com> > > > > > > > > This patch makes selection of pix_fmt similar to > > > > that in the native decoder. This makes samples such > > > > as p0_05.j2k and p1_03.j2k decodable by libopenjpeg. > > > > > > Since both files are not YUVA420P, I am not sure if this > > > patch is a good idea, in any case, the commit message > > > mentioning the two files as reasons for this patch is wrong. > > > > > I am not sure what file format this is then. > > I failed to find out so far, possibly Bayer, CMYK seems less > likely to me. The reference file has 4 components, Whereas all the Bayer formats have 3 components. Are we missing any Bayer pixel format in ffmpeg? Also, any other ideas on what has to be done for the 2 reference files mentioned? If this seems like a good idea, I could go through opj_decompress and try to replicate what it does. > > Carl Eugen > _______________________________________________ > 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".
On Wed, Jun 10, 2020 at 9:35 PM Gautam Ramakrishnan <gautamramk@gmail.com> wrote: > The reference file has 4 components, Whereas all the Bayer formats > have 3 components. Are we missing any Bayer pixel format in ffmpeg? > Also, any other ideas on what has to be done for the 2 reference files > mentioned? If this seems like a good idea, I could go through > opj_decompress > and try to replicate what it does. I don't think this is a real image. I think it's probably just a synthetic image for the purposes of conformance testing. p1_03.j2k and p0_05.j2k (and others) are too weird. I decoded p1_03.j2k's components into different files (opj_decompress -i p1_03.j2k -split-pnm -o /tmp/p1_03.pgm) and inspected each one. Additionally, I cat'd the raw gray8 pixel values to construct raw yuv420p frames. I created 4 raw yuv420p frames using every valid combination of p1_03.j2k's four planes. All of them looked like garbage. That's not too surprising. Each plane has dark near-zero pixel values in the sky region. A bright region like the sky should have high pixel values for the Y-plane. I highly suspect this j2k file is synthetic and just tests whether or not a decoder can decode the planes, but that doesn't test whether or not they can be sensibly displayed (and I'd argue they can't be sensibly displayed).
On Thu, Jun 11, 2020 at 11:42 AM Michael Bradshaw <mjbshaw@gmail.com> wrote: > > On Wed, Jun 10, 2020 at 9:35 PM Gautam Ramakrishnan <gautamramk@gmail.com> > wrote: > > > The reference file has 4 components, Whereas all the Bayer formats > > have 3 components. Are we missing any Bayer pixel format in ffmpeg? > > Also, any other ideas on what has to be done for the 2 reference files > > mentioned? If this seems like a good idea, I could go through > > opj_decompress > > and try to replicate what it does. > > > I don't think this is a real image. I think it's probably just a synthetic > image for the purposes of conformance testing. p1_03.j2k and p0_05.j2k (and > others) are too weird. > > I decoded p1_03.j2k's components into different files (opj_decompress -i > p1_03.j2k -split-pnm -o /tmp/p1_03.pgm) and inspected each one. > Additionally, I cat'd the raw gray8 pixel values to construct raw yuv420p > frames. I created 4 raw yuv420p frames using every valid combination > of p1_03.j2k's four planes. All of them looked like garbage. > > That's not too surprising. Each plane has dark near-zero pixel values in > the sky region. A bright region like the sky should have high pixel values > for the Y-plane. > > I highly suspect this j2k file is synthetic and just tests whether or not a > decoder can decode the planes, but that doesn't test whether or not they > can be sensibly displayed (and I'd argue they can't be sensibly displayed). Got it. In that case we can safely ignore the patch to fix libopenjpeg. However, p1_03.j2k is one of the 2 files to have ppm marker. How could I validate a patch to add ppm marker? I need something to cross validate. Any suggestions for that? > _______________________________________________ > 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".
On Thu, Jun 11, 2020 at 9:42 AM Gautam Ramakrishnan <gautamramk@gmail.com> wrote: > Got it. In that case we can safely ignore the patch to fix libopenjpeg. > However, p1_03.j2k is one of the 2 files to have ppm marker. How could I > validate a patch to add ppm marker? I need something to cross validate. > Any suggestions for that? Does the other file with a ppm marker have a sane pixel format? If not the only the only way I can think of to test this is to hack ffmpeg to remap the planes in libopenjpegdec.c (e.g., remap them to yuva format). You can use that for initial validation but it'll have to be reverted when committing/pushing. Long-term I'm not sure how one would regression test this without having a different file with a sane pixel format. I'm not sure how feasible it is to hack p1_03.j2k to remove a plane while retaining the ppm marker or perhaps hacking opj_compress to add a ppm marker to a new test file.
On Thu, Jun 11, 2020 at 9:41 PM Michael Bradshaw <mjbshaw-at-google.com@ffmpeg.org> wrote: > > On Thu, Jun 11, 2020 at 9:42 AM Gautam Ramakrishnan <gautamramk@gmail.com> > wrote: > > > Got it. In that case we can safely ignore the patch to fix libopenjpeg. > > However, p1_03.j2k is one of the 2 files to have ppm marker. How could I > > validate a patch to add ppm marker? I need something to cross validate. > > Any suggestions for that? > > > Does the other file with a ppm marker have a sane pixel format? If not the > only the only way I can think of to test this is to hack ffmpeg to remap > the planes in libopenjpegdec.c (e.g., remap them to yuva format). You can > use that for initial validation but it'll have to be reverted when > committing/pushing. > > Long-term I'm not sure how one would regression test this without having a > different file with a sane pixel format. I'm not sure how feasible it is to > hack p1_03.j2k to remove a plane while retaining the ppm marker or perhaps > hacking opj_compress to add a ppm marker to a new test file. > _______________________________________________ > 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". The other image with a PPM marker has only 3 components. I guess it will be a recognized format. However, it has a marker which is not yet implemented in the native decoder. I guess I'll just implement the other feature and we will be able to test this as well.
Am Di., 9. Juni 2020 um 20:25 Uhr schrieb Carl Eugen Hoyos <ceffmpeg@gmail.com>: > > Am Di., 9. Juni 2020 um 19:12 Uhr schrieb Gautam Ramakrishnan > <gautamramk@gmail.com>: > > > > On Tue, Jun 9, 2020 at 10:24 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > > > > > > Am Di., 9. Juni 2020 um 14:07 Uhr schrieb <gautamramk@gmail.com>: > > > > > > > > From: Gautam Ramakrishnan <gautamramk@gmail.com> > > > > > > > > This patch makes selection of pix_fmt similar to > > > > that in the native decoder. This makes samples such > > > > as p0_05.j2k and p1_03.j2k decodable by libopenjpeg. > > > > > > Since both files are not YUVA420P, I am not sure if this > > > patch is a good idea, in any case, the commit message > > > mentioning the two files as reasons for this patch is wrong. > > > > > I am not sure what file format this is then. > > I failed to find out so far, possibly Bayer, CMYK seems less > likely to me. I believe it is YCCK, two components (CC, if I am right) are subsampled. Carl Eugen
On Sun, Jul 19, 2020 at 12:09 AM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > > Am Di., 9. Juni 2020 um 20:25 Uhr schrieb Carl Eugen Hoyos <ceffmpeg@gmail.com>: > > > > Am Di., 9. Juni 2020 um 19:12 Uhr schrieb Gautam Ramakrishnan > > <gautamramk@gmail.com>: > > > > > > On Tue, Jun 9, 2020 at 10:24 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > > > > > > > > Am Di., 9. Juni 2020 um 14:07 Uhr schrieb <gautamramk@gmail.com>: > > > > > > > > > > From: Gautam Ramakrishnan <gautamramk@gmail.com> > > > > > > > > > > This patch makes selection of pix_fmt similar to > > > > > that in the native decoder. This makes samples such > > > > > as p0_05.j2k and p1_03.j2k decodable by libopenjpeg. > > > > > > > > Since both files are not YUVA420P, I am not sure if this > > > > patch is a good idea, in any case, the commit message > > > > mentioning the two files as reasons for this patch is wrong. > > > > > > > I am not sure what file format this is then. > > > > I failed to find out so far, possibly Bayer, CMYK seems less > > likely to me. > > I believe it is YCCK, two components (CC, if I am right) are > subsampled. In that case, does this patch make sense. I kinda just tried to replicate the native decoder
diff --git a/libavcodec/libopenjpegdec.c b/libavcodec/libopenjpegdec.c index 344c5ba5a3..f5f208784e 100644 --- a/libavcodec/libopenjpegdec.c +++ b/libavcodec/libopenjpegdec.c @@ -272,11 +272,11 @@ static inline void libopenjpeg_copyto8(AVFrame *picture, opj_image_t *image) { int *comp_data; uint8_t *img_ptr; int index, x, y; - for (index = 0; index < image->numcomps; index++) { + int plane = index?index-1:image->numcomps-1; comp_data = image->comps[index].data; for (y = 0; y < image->comps[index].h; y++) { - img_ptr = picture->data[index] + y * picture->linesize[index]; + img_ptr = picture->data[plane] + y * picture->linesize[plane]; for (x = 0; x < image->comps[index].w; x++) { *img_ptr = 0x80 * image->comps[index].sgnd + *comp_data; img_ptr++; @@ -408,6 +408,23 @@ static int libopenjpeg_decode_frame(AVCodecContext *avctx, if (avctx->pix_fmt == AV_PIX_FMT_NONE) avctx->pix_fmt = libopenjpeg_guess_pix_fmt(image); + if (avctx->pix_fmt == AV_PIX_FMT_NONE) { + if (image->numcomps == 4 && + image->comps[0].dx == 1 && image->comps[0].dy == 1 && + image->comps[1].dx == 1 && image->comps[1].dy == 1 && + image->comps[2].dx == image->comps[3].dx && + image->comps[2].dy == image->comps[3].dy) { + int maxprec = 0; + for (int i = 0; i < 4; i++) + maxprec = FFMAX(maxprec, image->comps[i].prec); + if (maxprec == 8 && + image->comps[2].dx == 2 && + image->comps[2].dy == 2) { + avctx->pix_fmt = AV_PIX_FMT_YUVA420P; + } + } + } + if (avctx->pix_fmt == AV_PIX_FMT_NONE) { av_log(avctx, AV_LOG_ERROR, "Unable to determine pixel format.\n"); ret = AVERROR_UNKNOWN;
From: Gautam Ramakrishnan <gautamramk@gmail.com> This patch makes selection of pix_fmt similar to that in the native decoder. This makes samples such as p0_05.j2k and p1_03.j2k decodable by libopenjpeg. --- libavcodec/libopenjpegdec.c | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-)