Re: [FFmpeg-devel] [PATCH]lavc/hevc: Support GBR encoding and decoding

2016-03-13 Thread Carl Eugen Hoyos 
Hendrik Leppkes  gmail.com> writes:

> > Encoder and decoder patches should be separated, they are quite
> > distinct code other then being related by codec, otherwise its
> > probably fine.
> >
> 
> Actually some further comments:

All comments applied and changes pushed.

Thank you for the review, Carl Eugen

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

Re: [FFmpeg-devel] [PATCH]lavc/hevc: Support GBR encoding and decoding

2016-03-12 Thread Hendrik Leppkes 
On Sun, Mar 13, 2016 at 3:14 AM, Hendrik Leppkes  wrote:
> On Sat, Mar 12, 2016 at 1:44 PM, Carl Eugen Hoyos  wrote:
>> Hi!
>>
>> Attached patch allows GBR encoding and decoding for hevc.
>>
>
> Encoder and decoder patches should be separated, they are quite
> distinct code other then being related by codec, otherwise its
> probably fine.
>

Actually some further comments:

> diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c
> index bcb63a3..7237ee6 100644
> --- a/libavcodec/hevc_ps.c
> +++ b/libavcodec/hevc_ps.c
> @@ -561,6 +561,17 @@ static void decode_vui(GetBitContext *gb, AVCodecContext 
> *avctx,
> vui->transfer_characteristic = AVCOL_TRC_UNSPECIFIED;
> if (vui->matrix_coeffs >= AVCOL_SPC_NB)
> vui->matrix_coeffs = AVCOL_SPC_UNSPECIFIED;
> +if (vui->matrix_coeffs == AVCOL_SPC_RGB)
> +switch (sps->pix_fmt) {
> +case AV_PIX_FMT_YUV444P:
> +sps->pix_fmt = AV_PIX_FMT_GBRP;
> +break;
> +case AV_PIX_FMT_YUV444P10:
> +sps->pix_fmt = AV_PIX_FMT_GBRP10;
> +break;
> +case AV_PIX_FMT_YUV444P12:
> +sps->pix_fmt = AV_PIX_FMT_GBRP12;
> +}

Please add a break here for consistency, avoiding issues early, etc. :)

> }
> }
>
> diff --git a/libavcodec/libx265.c b/libavcodec/libx265.c
> index 68c7fba..072bace 100644
> --- a/libavcodec/libx265.c
> +++ b/libavcodec/libx265.c
> @@ -257,6 +263,7 @@ static int libx265_encode_frame(AVCodecContext *avctx, 
> AVPacket *pkt,
>
> x265pic.pts  = pic->pts;
> x265pic.bitDepth = av_pix_fmt_desc_get(avctx->pix_fmt)->comp[0].depth;
> +printf("x265pic.bitDepth: %d \n", x265pic.bitDepth);
>
> x265pic.sliceType = pic->pict_type == AV_PICTURE_TYPE_I ? X265_TYPE_I 
> :
> pic->pict_type == AV_PICTURE_TYPE_P ? X265_TYPE_P 
> :

The printf is hopefully a debugging artifact? :)


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

Re: [FFmpeg-devel] [PATCH]lavc/hevc: Support GBR encoding and decoding

2016-03-12 Thread Hendrik Leppkes 
On Sat, Mar 12, 2016 at 1:44 PM, Carl Eugen Hoyos  wrote:
> Hi!
>
> Attached patch allows GBR encoding and decoding for hevc.
>

Encoder and decoder patches should be separated, they are quite
distinct code other then being related by codec, otherwise its
probably fine.

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

Re: [FFmpeg-devel] [PATCH]lavc/hevc: Support GBR encoding and decoding

2016-03-12 Thread Carl Eugen Hoyos 
Moritz Barsnick  gmx.net> writes:

> > +case AV_PIX_FMT_GBRP:
> > +case AV_PIX_FMT_GBRP10:
> > +case AV_PIX_FMT_GBRP12:
> > +ctx->params->vui.matrixCoeffs = AVCOL_SPC_RGB;
> > +ctx->params->vui.bEnableVideoSignalTypePresentFlag  = 1;
> > +ctx->params->vui.bEnableColorDescriptionPresentFlag = 1;
> >  case AV_PIX_FMT_YUV444P:
> >  case AV_PIX_FMT_YUV444P10:
> >  case AV_PIX_FMT_YUV444P12:
> 
> Fallthrough intended?

Definitely.

> Might be worth documenting then, because reading
> such switch/cases can become confusing.

I believe we don't usually add a comment here, 
several files distinguish in a similar way between 
gbrp and yuv444p.

Carl Eugen

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

Re: [FFmpeg-devel] [PATCH]lavc/hevc: Support GBR encoding and decoding

2016-03-12 Thread Moritz Barsnick 
On Sat, Mar 12, 2016 at 13:44:13 +0100, Carl Eugen Hoyos wrote:
>  break;
> +case AV_PIX_FMT_GBRP:
> +case AV_PIX_FMT_GBRP10:
> +case AV_PIX_FMT_GBRP12:
> +ctx->params->vui.matrixCoeffs = AVCOL_SPC_RGB;
> +ctx->params->vui.bEnableVideoSignalTypePresentFlag  = 1;
> +ctx->params->vui.bEnableColorDescriptionPresentFlag = 1;
>  case AV_PIX_FMT_YUV444P:
>  case AV_PIX_FMT_YUV444P10:
>  case AV_PIX_FMT_YUV444P12:

Fallthrough intended? Might be worth documenting then, because reading
such switch/cases can become confusing.

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

Re: [FFmpeg-devel] [PATCH]lavc/hevc: Support GBR encoding and decoding

2016-03-12 Thread Carl Eugen Hoyos 
Carl Eugen Hoyos  ag.or.at> writes:

>  AV_PIX_FMT_YUV420P,
>  AV_PIX_FMT_YUV422P,
>  AV_PIX_FMT_YUV444P,
> +AV_PIX_FMT_GBRP10,

Fixed locally to AV_PIX_FMT_GBRP;

Carl Eugen

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

[FFmpeg-devel] [PATCH]lavc/hevc: Support GBR encoding and decoding

2016-03-12 Thread Carl Eugen Hoyos 
Hi!

Attached patch allows GBR encoding and decoding for hevc.

Please comment, Carl Eugen
diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c
index bcb63a3..7237ee6 100644
--- a/libavcodec/hevc_ps.c
+++ b/libavcodec/hevc_ps.c
@@ -561,6 +561,17 @@ static void decode_vui(GetBitContext *gb, AVCodecContext 
*avctx,
 vui->transfer_characteristic = AVCOL_TRC_UNSPECIFIED;
 if (vui->matrix_coeffs >= AVCOL_SPC_NB)
 vui->matrix_coeffs = AVCOL_SPC_UNSPECIFIED;
+if (vui->matrix_coeffs == AVCOL_SPC_RGB)
+switch (sps->pix_fmt) {
+case AV_PIX_FMT_YUV444P:
+sps->pix_fmt = AV_PIX_FMT_GBRP;
+break;
+case AV_PIX_FMT_YUV444P10:
+sps->pix_fmt = AV_PIX_FMT_GBRP10;
+break;
+case AV_PIX_FMT_YUV444P12:
+sps->pix_fmt = AV_PIX_FMT_GBRP12;
+}
 }
 }
 
diff --git a/libavcodec/libx265.c b/libavcodec/libx265.c
index 68c7fba..072bace 100644
--- a/libavcodec/libx265.c
+++ b/libavcodec/libx265.c
@@ -154,6 +154,12 @@ static av_cold int libx265_encode_init(AVCodecContext 
*avctx)
 case AV_PIX_FMT_YUV422P12:
 ctx->params->internalCsp = X265_CSP_I422;
 break;
+case AV_PIX_FMT_GBRP:
+case AV_PIX_FMT_GBRP10:
+case AV_PIX_FMT_GBRP12:
+ctx->params->vui.matrixCoeffs = AVCOL_SPC_RGB;
+ctx->params->vui.bEnableVideoSignalTypePresentFlag  = 1;
+ctx->params->vui.bEnableColorDescriptionPresentFlag = 1;
 case AV_PIX_FMT_YUV444P:
 case AV_PIX_FMT_YUV444P10:
 case AV_PIX_FMT_YUV444P12:
@@ -257,6 +263,7 @@ static int libx265_encode_frame(AVCodecContext *avctx, 
AVPacket *pkt,
 
 x265pic.pts  = pic->pts;
 x265pic.bitDepth = av_pix_fmt_desc_get(avctx->pix_fmt)->comp[0].depth;
+printf("x265pic.bitDepth: %d \n", x265pic.bitDepth);
 
 x265pic.sliceType = pic->pict_type == AV_PICTURE_TYPE_I ? X265_TYPE_I :
 pic->pict_type == AV_PICTURE_TYPE_P ? X265_TYPE_P :
@@ -318,6 +325,7 @@ static const enum AVPixelFormat x265_csp_eight[] = {
 AV_PIX_FMT_YUV420P,
 AV_PIX_FMT_YUV422P,
 AV_PIX_FMT_YUV444P,
+AV_PIX_FMT_GBRP,
 AV_PIX_FMT_NONE
 };
 
@@ -325,9 +333,11 @@ static const enum AVPixelFormat x265_csp_ten[] = {
 AV_PIX_FMT_YUV420P,
 AV_PIX_FMT_YUV422P,
 AV_PIX_FMT_YUV444P,
+AV_PIX_FMT_GBRP10,
 AV_PIX_FMT_YUV420P10,
 AV_PIX_FMT_YUV422P10,
 AV_PIX_FMT_YUV444P10,
+AV_PIX_FMT_GBRP10,
 AV_PIX_FMT_NONE
 };
 
@@ -335,12 +345,15 @@ static const enum AVPixelFormat x265_csp_twelve[] = {
 AV_PIX_FMT_YUV420P,
 AV_PIX_FMT_YUV422P,
 AV_PIX_FMT_YUV444P,
+AV_PIX_FMT_GBRP,
 AV_PIX_FMT_YUV420P10,
 AV_PIX_FMT_YUV422P10,
 AV_PIX_FMT_YUV444P10,
+AV_PIX_FMT_GBRP10,
 AV_PIX_FMT_YUV420P12,
 AV_PIX_FMT_YUV422P12,
 AV_PIX_FMT_YUV444P12,
+AV_PIX_FMT_GBRP12,
 AV_PIX_FMT_NONE
 };
 
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel