diff mbox

[FFmpeg-devel] avcodec/mjpeg: don't override color information from demuxer

Message ID E1gUKEE-001Z0L-2T@pannekake.samfundet.no
State New
Headers show

Commit Message

Steinar H. Gunderson Dec. 4, 2018, 11:31 p.m. UTC 
Some demuxers, like Matroska, allow for sending colorspace information
that override MJPEG defaults when it comes to Y'CbCr coefficients or
chroma location. Don't override such data if the demuxer already has
set it; this allows decoding such MJPEG streams correctly.

Also document in avcodec.h that these fields are actually set first by
libavformat, even if libavcodec is usually the one to set them. This is
not new behavior; e.g., dnxhd already works this way.
---
 libavcodec/avcodec.h                      | 10 +++++-----
 libavcodec/mjpegdec.c                     |  8 ++++++--
 tests/fate/matroska.mak                   |  3 +++
 tests/ref/fate/matroska-mjpeg-color-space | 22 ++++++++++++++++++++++
 4 files changed, 36 insertions(+), 7 deletions(-)
 create mode 100644 tests/ref/fate/matroska-mjpeg-color-space

Comments

Steinar H. Gunderson Dec. 4, 2018, 11:36 p.m. UTC | #1 
On Wed, Dec 05, 2018 at 12:31:10AM +0100, Steinar H. Gunderson wrote:
> Some demuxers, like Matroska, allow for sending colorspace information
> that override MJPEG defaults when it comes to Y'CbCr coefficients or
> chroma location. Don't override such data if the demuxer already has
> set it; this allows decoding such MJPEG streams correctly.
> 
> Also document in avcodec.h that these fields are actually set first by
> libavformat, even if libavcodec is usually the one to set them. This is
> not new behavior; e.g., dnxhd already works this way.

Attaching the FATE test file used (created by myself; it's just a black
MJPEG frame, although with the special CS=ITU601 tag set to get TV range).

/* Steinar */
Hendrik Leppkes Dec. 4, 2018, 11:57 p.m. UTC | #2 
On Wed, Dec 5, 2018 at 12:35 AM Steinar H. Gunderson
<steinar+ffmpeg@gunderson.no> wrote:
>
> Some demuxers, like Matroska, allow for sending colorspace information
> that override MJPEG defaults when it comes to Y'CbCr coefficients or
> chroma location. Don't override such data if the demuxer already has
> set it; this allows decoding such MJPEG streams correctly.
>
> Also document in avcodec.h that these fields are actually set first by
> libavformat, even if libavcodec is usually the one to set them. This is
> not new behavior; e.g., dnxhd already works this way.
> ---
>  libavcodec/avcodec.h                      | 10 +++++-----
>  libavcodec/mjpegdec.c                     |  8 ++++++--
>  tests/fate/matroska.mak                   |  3 +++
>  tests/ref/fate/matroska-mjpeg-color-space | 22 ++++++++++++++++++++++
>  4 files changed, 36 insertions(+), 7 deletions(-)
>  create mode 100644 tests/ref/fate/matroska-mjpeg-color-space
>
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index 3922e89331..b7ea99d8ab 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -2145,35 +2145,35 @@ typedef struct AVCodecContext {
>      /**
>       * Chromaticity coordinates of the source primaries.
>       * - encoding: Set by user
> -     * - decoding: Set by libavcodec
> +     * - decoding: Set by libavformat and/or libavcodec
>       */
>      enum AVColorPrimaries color_primaries;
>
>      /**
>       * Color Transfer Characteristic.
>       * - encoding: Set by user
> -     * - decoding: Set by libavcodec
> +     * - decoding: Set by libavformat and/or libavcodec
>       */
>      enum AVColorTransferCharacteristic color_trc;
>
>      /**
>       * YUV colorspace type.
>       * - encoding: Set by user
> -     * - decoding: Set by libavcodec
> +     * - decoding: Set by libavformat and/or libavcodec
>       */
>      enum AVColorSpace colorspace;
>
>      /**
>       * MPEG vs JPEG YUV range.
>       * - encoding: Set by user
> -     * - decoding: Set by libavcodec
> +     * - decoding: Set by libavformat and/or libavcodec
>       */
>      enum AVColorRange color_range;
>
>      /**
>       * This defines the location of chroma samples.
>       * - encoding: Set by user
> -     * - decoding: Set by libavcodec
> +     * - decoding: Set by libavformat and/or libavcodec
>       */
>      enum AVChromaLocation chroma_sample_location;
>

These comments are not accurate. avformat does not fill these values,
because outside of deprecated code it does not expose such a struct to
the user.

- Hendrik
Steinar H. Gunderson Dec. 5, 2018, 12:14 a.m. UTC | #3 
On Wed, Dec 05, 2018 at 12:57:43AM +0100, Hendrik Leppkes wrote:
> These comments are not accurate. avformat does not fill these values,
> because outside of deprecated code it does not expose such a struct to
> the user.

Hm, I was asked on #ffmpeg-devel to update it :-) But I suppose maybe it sets
them in AVCodecParameters instead?

/* Steinar */
Hendrik Leppkes Dec. 5, 2018, 12:33 a.m. UTC | #4 
On Wed, Dec 5, 2018 at 1:14 AM Steinar H. Gunderson
<steinar+ffmpeg@gunderson.no> wrote:
>
> On Wed, Dec 05, 2018 at 12:57:43AM +0100, Hendrik Leppkes wrote:
> > These comments are not accurate. avformat does not fill these values,
> > because outside of deprecated code it does not expose such a struct to
> > the user.
>
> Hm, I was asked on #ffmpeg-devel to update it :-) But I suppose maybe it sets
> them in AVCodecParameters instead?
>

It would. And that struct is only ever filled by avformat (or a user),
and not avcodec, so there is no overlap there either.

The real problem is that its impossible to know which component to
really trust. Why does a demuxer have more authority on the color
format then a decoder? Or vice-versa?
We don't have a solution for that, and its really something the user
application currently has to solve.

- Hendrik
Steinar H. Gunderson Dec. 5, 2018, 8:31 a.m. UTC | #5 
On Wed, Dec 05, 2018 at 01:33:12AM +0100, Hendrik Leppkes wrote:
> The real problem is that its impossible to know which component to
> really trust. Why does a demuxer have more authority on the color
> format then a decoder? Or vice-versa?

You can make this arbitrarily complicated, of course, but if the codec
has no info apart from defaults (as in this case), I'd say the decision is
fairly simple. The one with the more specific information wins, which is the
demuxer.

There's a corner case where you can't specify _explicitly_ in the mux
“I don't know, but it's probably not the defaults” (since there's no
distinction between “unspecified because the format doesn't say so”
and “unspecified because the user explicitly told us to”), but I can't see
what an application would do in that case except use the codec defaults
anyway.

> We don't have a solution for that, and its really something the user
> application currently has to solve.

The user application doesn't feel like the right place for this. How would it
be in a better position than FFmpeg to know? And if it really is in the
position to set a policy here, it can read out the information of the codec
parameter struct and override it, no matter what libavcodec decided to do.

/* Steinar */
Steinar H. Gunderson Dec. 26, 2018, 1:21 p.m. UTC | #6 
On Wed, Dec 05, 2018 at 09:31:48AM +0100, Steinar H. Gunderson wrote:
>> We don't have a solution for that, and its really something the user
>> application currently has to solve.
> The user application doesn't feel like the right place for this. How would it
> be in a better position than FFmpeg to know? And if it really is in the
> position to set a policy here, it can read out the information of the codec
> parameter struct and override it, no matter what libavcodec decided to do.

What's the status of this patch? Is it rejected on the basis of “this is the
wrong thing to do” and nedes a larger rework of the rest of FFmpeg, or could it
be merged with some changes?

/* Steinar */
diff mbox

Patch

diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
index 3922e89331..b7ea99d8ab 100644
--- a/libavcodec/avcodec.h
+++ b/libavcodec/avcodec.h
@@ -2145,35 +2145,35 @@  typedef struct AVCodecContext {
     /**
      * Chromaticity coordinates of the source primaries.
      * - encoding: Set by user
-     * - decoding: Set by libavcodec
+     * - decoding: Set by libavformat and/or libavcodec
      */
     enum AVColorPrimaries color_primaries;
 
     /**
      * Color Transfer Characteristic.
      * - encoding: Set by user
-     * - decoding: Set by libavcodec
+     * - decoding: Set by libavformat and/or libavcodec
      */
     enum AVColorTransferCharacteristic color_trc;
 
     /**
      * YUV colorspace type.
      * - encoding: Set by user
-     * - decoding: Set by libavcodec
+     * - decoding: Set by libavformat and/or libavcodec
      */
     enum AVColorSpace colorspace;
 
     /**
      * MPEG vs JPEG YUV range.
      * - encoding: Set by user
-     * - decoding: Set by libavcodec
+     * - decoding: Set by libavformat and/or libavcodec
      */
     enum AVColorRange color_range;
 
     /**
      * This defines the location of chroma samples.
      * - encoding: Set by user
-     * - decoding: Set by libavcodec
+     * - decoding: Set by libavformat and/or libavcodec
      */
     enum AVChromaLocation chroma_sample_location;
 
diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c
index 2f1635838a..9ee2daadf8 100644
--- a/libavcodec/mjpegdec.c
+++ b/libavcodec/mjpegdec.c
@@ -158,8 +158,12 @@  av_cold int ff_mjpeg_decode_init(AVCodecContext *avctx)
     s->first_picture = 1;
     s->got_picture   = 0;
     s->org_height    = avctx->coded_height;
-    avctx->chroma_sample_location = AVCHROMA_LOC_CENTER;
-    avctx->colorspace = AVCOL_SPC_BT470BG;
+    if (avctx->chroma_sample_location == AVCHROMA_LOC_UNSPECIFIED) {
+        avctx->chroma_sample_location = AVCHROMA_LOC_CENTER;
+    }
+    if (avctx->colorspace == AVCOL_SPC_UNSPECIFIED) {
+        avctx->colorspace = AVCOL_SPC_BT470BG;
+    }
     s->hwaccel_pix_fmt = s->hwaccel_sw_pix_fmt = AV_PIX_FMT_NONE;
 
     if ((ret = init_default_huffman_tables(s)) < 0)
diff --git a/tests/fate/matroska.mak b/tests/fate/matroska.mak
index 2747496e1e..1768ce6ee2 100644
--- a/tests/fate/matroska.mak
+++ b/tests/fate/matroska.mak
@@ -9,5 +9,8 @@  fate-matroska-remux: REF = 1ed49a4f2b6790357fac268938357353
 FATE_MATROSKA_FFPROBE-$(call ALLYES, MATROSKA_DEMUXER) += fate-matroska-spherical-mono
 fate-matroska-spherical-mono: CMD = run ffprobe$(PROGSSUF)$(EXESUF) -show_entries stream_side_data_list -select_streams v -v 0 $(TARGET_SAMPLES)/mkv/spherical.mkv
 
+FATE_MATROSKA_FFPROBE-$(call ALLYES, MATROSKA_DEMUXER) += fate-matroska-mjpeg-color-space
+fate-matroska-mjpeg-color-space: CMD = run ffprobe$(PROGSSUF)$(EXESUF) -show_entries frame=color_range,color_space,color_primaries,color_transfer,chroma_location $(TARGET_SAMPLES)/mkv/mjpeg-color-space.mkv
+
 FATE_SAMPLES_AVCONV += $(FATE_MATROSKA-yes)
 FATE_SAMPLES_FFPROBE += $(FATE_MATROSKA_FFPROBE-yes)
diff --git a/tests/ref/fate/matroska-mjpeg-color-space b/tests/ref/fate/matroska-mjpeg-color-space
new file mode 100644
index 0000000000..d3cd39f967
--- /dev/null
+++ b/tests/ref/fate/matroska-mjpeg-color-space
@@ -0,0 +1,22 @@ 
+[FRAME]
+color_range=tv
+color_space=bt709
+color_primaries=bt709
+color_transfer=iec61966-2-1
+chroma_location=left
+[SIDE_DATA]
+[/SIDE_DATA]
+[SIDE_DATA]
+[/SIDE_DATA]
+[/FRAME]
+[FRAME]
+color_range=tv
+color_space=bt709
+color_primaries=bt709
+color_transfer=iec61966-2-1
+chroma_location=left
+[SIDE_DATA]
+[/SIDE_DATA]
+[SIDE_DATA]
+[/SIDE_DATA]
+[/FRAME]