diff mbox series

[FFmpeg-devel,RFC] libavcodec/libopenjpeg: pix fmt selection change

Message ID 20200609120653.20078-1-gautamramk@gmail.com
State New
Headers show
Series [FFmpeg-devel,RFC] libavcodec/libopenjpeg: pix fmt selection change
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 June 9, 2020, 12:06 p.m. UTC
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(-)

Comments

Michael Bradshaw June 9, 2020, 1:23 p.m. UTC | #1
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?
Carl Eugen Hoyos June 9, 2020, 4:53 p.m. UTC | #2
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
Gautam Ramakrishnan June 9, 2020, 5:12 p.m. UTC | #3
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".
Carl Eugen Hoyos June 9, 2020, 6:25 p.m. UTC | #4
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
Gautam Ramakrishnan June 11, 2020, 2:25 a.m. UTC | #5
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".
Michael Bradshaw June 11, 2020, 5:07 a.m. UTC | #6
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).
Gautam Ramakrishnan June 11, 2020, 3:42 p.m. UTC | #7
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".
Michael Bradshaw June 11, 2020, 4:11 p.m. UTC | #8
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.
Gautam Ramakrishnan June 12, 2020, 3:50 p.m. UTC | #9
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.
Carl Eugen Hoyos July 18, 2020, 3:41 p.m. UTC | #10
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
Gautam Ramakrishnan July 19, 2020, 11:26 a.m. UTC | #11
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 mbox series

Patch

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;