Re: [FFmpeg-devel] [PATCH] pgssubdec: fix subpicture output colorspace and range

[Hendrik Leppkes]

On Sat, Apr 23, 2016 at 2:33 PM, Jan Ekstrom  wrote:
> On Sat, Apr 23, 2016 at 3:21 PM, wm4  wrote:
>> In that case the hdtv field should be completely removed, and the test
>> put in the palette conversion code.
>>
>
> If avctx->height is by default 0, then it would have to be something a la:
>
> if (!avctx->height || avctx->height > 576) {
> YUV_TO_RGB1_CCIR_BT709(cb, cr);
> } else {
> YUV_TO_RGB1_CCIR(cb, cr);
> }
>
> I guess? If the default is -1 then it'd be a check for <0 or >576.
> This way even if in a cut file a palette comes up before a
> presentation segment it should default to BT.709.
>

That looks fine.

- Hendrik
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] pgssubdec: fix subpicture output colorspace and range

[Jan Ekstrom]

On Sat, Apr 23, 2016 at 3:21 PM, wm4  wrote:
> In that case the hdtv field should be completely removed, and the test
> put in the palette conversion code.
>

If avctx->height is by default 0, then it would have to be something a la:

if (!avctx->height || avctx->height > 576) {
YUV_TO_RGB1_CCIR_BT709(cb, cr);
} else {
YUV_TO_RGB1_CCIR(cb, cr);
}

I guess? If the default is -1 then it'd be a check for <0 or >576.
This way even if in a cut file a palette comes up before a
presentation segment it should default to BT.709.

Jan
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] pgssubdec: fix subpicture output colorspace and range

[Jan Ekstrom]

On Sat, Apr 23, 2016 at 11:09 AM, Carl Eugen Hoyos  wrote:
> Please mention the relevant ticket in the commit message.

OK, will do.

>
>> +int hdtv;
>
> Please rename to sdtv so you can remove the assignment from
> init_decoder().
>

What would that make of the default value? The current design lets me
test for C true/false since the default is overridden at init() to -1
and the HD content would have 1 set. I seem to remember that in stack
various values are set to zero, but is the context always on stack?

>
> ctx->sdtv = h <=576 (or add braces).
>

OK, will set the value according to the response to my previous question.

>> +#define YUV_TO_RGB1_CCIR_BT709(cb1, cr1)\
>
> Just being curious: Did you test how much the values were
> off without this hunk?
>

The usual amount of BT.601 vs BT.709. Granted, many subpictures don't
contain much if any color, but since the values are not really limited
I'd say it's only fair to compare it to a standard BT.709/BT.601
rendering difference. If you haven't experienced such difference yet,
you can test it with these following files that contain the exact same
YCbCr values, but different colorimetry metadata in the bit stream
(601 and 709 accordingly):
http://cccp-project.net/beta/test_files/renderer_test_with_bt601.mkv
http://cccp-project.net/beta/test_files/renderer_test_with_bt709.mkv

Jan
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] pgssubdec: fix subpicture output colorspace and range

[Jan Ekstrom]

On Sat, Apr 23, 2016 at 2:53 PM, Hendrik Leppkes  wrote:
> Otherwise, I think Carl's suggestion might help, PGS subtitles are
> generally from Blu-rays, which means most of it is HD, so swapping the
> flag to detect SD conditions might make it act more "appropriate" in
> absence of w/h.

The default is -1 and I have switched the check for BT.709 to `if
(ctx->hdtv)` so it should only use BT.601 if and only if it gets a
presentation segment with a height that is <= 576.

Jan
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] pgssubdec: fix subpicture output colorspace and range

[Hendrik Leppkes]

On Sat, Apr 23, 2016 at 1:18 PM, wm4  wrote:
> On Fri, 22 Apr 2016 23:06:37 +0300
> Jan Ekström  wrote:
>
>> Functionality used before didn't widen the values from limited to
>> full range. Additionally, now the decoder uses BT.709 where it
>> should be used according to the video resolution.
>>
>> Default for not yet set colorimetry is BT.709 due to most observed
>> HDMV content being HD.
>>
>> BT.709 coefficients were gathered from the first two parts of BT.709
>> to BT.2020 conversion guide in ARIB STD-B62 (Pt. 1, Chapter 6.2.2).
>>
>> Based on a patch by Carl Eugen Hoyos.
>> ---
>>  libavcodec/pgssubdec.c | 20 ++--
>>  libavutil/colorspace.h | 10 ++
>>  2 files changed, 28 insertions(+), 2 deletions(-)
>>
>> diff --git a/libavcodec/pgssubdec.c b/libavcodec/pgssubdec.c
>> index 07a2a78..4145f1c 100644
>> --- a/libavcodec/pgssubdec.c
>> +++ b/libavcodec/pgssubdec.c
>> @@ -96,6 +96,7 @@ typedef struct PGSSubContext {
>>  PGSSubPalettes palettes;
>>  PGSSubObjects  objects;
>>  int forced_subs_only;
>> +int hdtv;
>>  } PGSSubContext;
>>
>>  static void flush_cache(AVCodecContext *avctx)
>> @@ -136,6 +137,9 @@ static PGSSubPalette * find_palette(int id, 
>> PGSSubPalettes *palettes)
>>
>>  static av_cold int init_decoder(AVCodecContext *avctx)
>>  {
>> +PGSSubContext *ctx = avctx->priv_data;
>> +ctx->hdtv  = -1;
>> +
>>  avctx->pix_fmt = AV_PIX_FMT_PAL8;
>>
>>  return 0;
>> @@ -354,8 +358,14 @@ static int parse_palette_segment(AVCodecContext *avctx,
>>  cb= bytestream_get_byte();
>>  alpha = bytestream_get_byte();
>>
>> -YUV_TO_RGB1(cb, cr);
>> -YUV_TO_RGB2(r, g, b, y);
>> +/* Default to HDTV (-1 or 1) */
>> +if (ctx->hdtv) {
>> +YUV_TO_RGB1_CCIR_BT709(cb, cr);
>> +} else {
>> +YUV_TO_RGB1_CCIR(cb, cr);
>> +}
>> +
>> +YUV_TO_RGB2_CCIR(r, g, b, y);
>>
>>  ff_dlog(avctx, "Color %d := (%d,%d,%d,%d)\n", color_id, r, g, b, 
>> alpha);
>>
>> @@ -388,6 +398,12 @@ static int parse_presentation_segment(AVCodecContext 
>> *avctx,
>>  int w = bytestream_get_be16();
>>  int h = bytestream_get_be16();
>>
>> +// Set colorimetry according to resolution
>> +if (h > 576)
>> +ctx->hdtv = 1;
>> +else
>> +ctx->hdtv = 0;
>> +
>>  ctx->presentation.pts = pts;
>>
>>  ff_dlog(avctx, "Video Dimensions %dx%d\n",
>> diff --git a/libavutil/colorspace.h b/libavutil/colorspace.h
>> index 826ffd5..7d3f711 100644
>> --- a/libavutil/colorspace.h
>> +++ b/libavutil/colorspace.h
>> @@ -41,6 +41,16 @@
>>  b_add = FIX(1.77200*255.0/224.0) * cb + ONE_HALF;\
>>  }
>>
>> +#define YUV_TO_RGB1_CCIR_BT709(cb1, cr1)\
>> +{\
>> +cb = (cb1) - 128;\
>> +cr = (cr1) - 128;\
>> +r_add = FIX(1.5747*255.0/224.0) * cr + ONE_HALF;\
>> +g_add = - FIX(0.1873*255.0/224.0) * cb - FIX(0.4682*255.0/224.0) * cr + 
>> \
>> +ONE_HALF;\
>> +b_add = FIX(1.8556*255.0/224.0) * cb + ONE_HALF;\
>> +}
>> +
>>  #define YUV_TO_RGB2_CCIR(r, g, b, y1)\
>>  {\
>>  y = ((y1) - 16) * FIX(255.0/219.0);\
>
> This still has the problem that the palette could be read before the
> resolution is known.

The Blu-ray specification controls the order of segments, and this
should not happen on conformant streams.
Otherwise, I think Carl's suggestion might help, PGS subtitles are
generally from Blu-rays, which means most of it is HD, so swapping the
flag to detect SD conditions might make it act more "appropriate" in
absence of w/h.

- Hendrik
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] pgssubdec: fix subpicture output colorspace and range

[wm4]

On Fri, 22 Apr 2016 23:06:37 +0300
Jan Ekström  wrote:

> Functionality used before didn't widen the values from limited to
> full range. Additionally, now the decoder uses BT.709 where it
> should be used according to the video resolution.
> 
> Default for not yet set colorimetry is BT.709 due to most observed
> HDMV content being HD.
> 
> BT.709 coefficients were gathered from the first two parts of BT.709
> to BT.2020 conversion guide in ARIB STD-B62 (Pt. 1, Chapter 6.2.2).
> 
> Based on a patch by Carl Eugen Hoyos.
> ---
>  libavcodec/pgssubdec.c | 20 ++--
>  libavutil/colorspace.h | 10 ++
>  2 files changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/libavcodec/pgssubdec.c b/libavcodec/pgssubdec.c
> index 07a2a78..4145f1c 100644
> --- a/libavcodec/pgssubdec.c
> +++ b/libavcodec/pgssubdec.c
> @@ -96,6 +96,7 @@ typedef struct PGSSubContext {
>  PGSSubPalettes palettes;
>  PGSSubObjects  objects;
>  int forced_subs_only;
> +int hdtv;
>  } PGSSubContext;
>  
>  static void flush_cache(AVCodecContext *avctx)
> @@ -136,6 +137,9 @@ static PGSSubPalette * find_palette(int id, 
> PGSSubPalettes *palettes)
>  
>  static av_cold int init_decoder(AVCodecContext *avctx)
>  {
> +PGSSubContext *ctx = avctx->priv_data;
> +ctx->hdtv  = -1;
> +
>  avctx->pix_fmt = AV_PIX_FMT_PAL8;
>  
>  return 0;
> @@ -354,8 +358,14 @@ static int parse_palette_segment(AVCodecContext *avctx,
>  cb= bytestream_get_byte();
>  alpha = bytestream_get_byte();
>  
> -YUV_TO_RGB1(cb, cr);
> -YUV_TO_RGB2(r, g, b, y);
> +/* Default to HDTV (-1 or 1) */
> +if (ctx->hdtv) {
> +YUV_TO_RGB1_CCIR_BT709(cb, cr);
> +} else {
> +YUV_TO_RGB1_CCIR(cb, cr);
> +}
> +
> +YUV_TO_RGB2_CCIR(r, g, b, y);
>  
>  ff_dlog(avctx, "Color %d := (%d,%d,%d,%d)\n", color_id, r, g, b, 
> alpha);
>  
> @@ -388,6 +398,12 @@ static int parse_presentation_segment(AVCodecContext 
> *avctx,
>  int w = bytestream_get_be16();
>  int h = bytestream_get_be16();
>  
> +// Set colorimetry according to resolution
> +if (h > 576)
> +ctx->hdtv = 1;
> +else
> +ctx->hdtv = 0;
> +
>  ctx->presentation.pts = pts;
>  
>  ff_dlog(avctx, "Video Dimensions %dx%d\n",
> diff --git a/libavutil/colorspace.h b/libavutil/colorspace.h
> index 826ffd5..7d3f711 100644
> --- a/libavutil/colorspace.h
> +++ b/libavutil/colorspace.h
> @@ -41,6 +41,16 @@
>  b_add = FIX(1.77200*255.0/224.0) * cb + ONE_HALF;\
>  }
>  
> +#define YUV_TO_RGB1_CCIR_BT709(cb1, cr1)\
> +{\
> +cb = (cb1) - 128;\
> +cr = (cr1) - 128;\
> +r_add = FIX(1.5747*255.0/224.0) * cr + ONE_HALF;\
> +g_add = - FIX(0.1873*255.0/224.0) * cb - FIX(0.4682*255.0/224.0) * cr + \
> +ONE_HALF;\
> +b_add = FIX(1.8556*255.0/224.0) * cb + ONE_HALF;\
> +}
> +
>  #define YUV_TO_RGB2_CCIR(r, g, b, y1)\
>  {\
>  y = ((y1) - 16) * FIX(255.0/219.0);\

This still has the problem that the palette could be read before the
resolution is known.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] pgssubdec: fix subpicture output colorspace and range

[Carl Eugen Hoyos]

Jan Ekström  gmail.com> writes:

> Functionality used before didn't widen the values from limited to
> full range. Additionally, now the decoder uses BT.709 where it
> should be used according to the video resolution.

Please mention the relevant ticket in the commit message.

> +int hdtv;

Please rename to sdtv so you can remove the assignment from 
init_decoder().

> +// Set colorimetry according to resolution
> +if (h > 576)
> +ctx->hdtv = 1;
> +else
> +ctx->hdtv = 0;

ctx->sdtv = h <=576 (or add braces).

> +#define YUV_TO_RGB1_CCIR_BT709(cb1, cr1)\

Just being curious: Did you test how much the values were 
off without this hunk?

Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel