Re: [libav-devel] [PATCH 02/14] lavc: Add coded bitstream read/write support for H.264

2017-08-12 Thread Mark Thompson 
On 12/08/17 19:45, Diego Biurrun wrote:
> On Sat, Aug 12, 2017 at 07:23:50PM +0100, Mark Thompson wrote:
>> On 12/08/17 09:44, Diego Biurrun wrote:
>>> On Fri, Aug 11, 2017 at 12:36:57AM +0100, Mark Thompson wrote:
 --- /dev/null
 +++ b/libavcodec/cbs_h2645.c
 @@ -0,0 +1,997 @@
 +
 +#define FUNC(name) FUNC_H264(READWRITE, name)
 +#include "cbs_h264_syntax.c"
 +#undef FUNC
 +
 +#define FUNC(name) FUNC_H264(READWRITE, name)
 +#include "cbs_h264_syntax.c"
 +#undef FUNC
>>>
>>> The convention is for these files to be called *_template.c.
>>
>> Given that I'm making a load of these files and they have pretty 
>> clearly-defined contents, can I create a new convention?
> 
> Well, they really are template files - what's the problem, the name
> gets too long for your taste? cbs_h264_template.c would work as well,
> unless you also have cbs_h264_something_else.c in the queue.

Yeah, mainly thinking it's uglier than without _template.

> I do believe that naming conventions do help organize things sensibly,
> so I'm reluctant to let this go to be honest...

Ok, changed (to cbs_foo_syntax_template.c).

New version reflecting all review comments is going through oracle now.  
(Excepting adding spaces to pointer casts, which other people have argued 
against and I don't think was ever agreed to be a thing to do generally.)

Assuming oracle doesn't find any more problems (no more anonymous unions or 
anything) and there are no more comments then I'll push 1-12 (11 split into 
two) later tomorrow.  13/14 are waiting for testing on Windows, and I'll post 
MPEG-2 support as a new set at some point in future.

Thanks,

- Mark
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

[libav-devel] [PATCH] mpeg2enc: Don't mark all streams as component video

2017-08-12 Thread Mark Thompson 
Since there is no information about the source format, "unspecified"
is the correct value to write here.
---
 libavcodec/mpeg12enc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavcodec/mpeg12enc.c b/libavcodec/mpeg12enc.c
index 103f3aaa7..406950901 100644
--- a/libavcodec/mpeg12enc.c
+++ b/libavcodec/mpeg12enc.c
@@ -297,7 +297,7 @@ static void mpeg1_encode_sequence_header(MpegEncContext *s)
 
 put_header(s, EXT_START_CODE);
 put_bits(>pb, 4, 2); // sequence 
display extension
-put_bits(>pb, 3, 0); // video_format: 0 
is components
+put_bits(>pb, 3, 5); // video_format: 5 
is unspecified
 put_bits(>pb, 1, 1); // 
colour_description
 put_bits(>pb, 8, s->avctx->color_primaries); // colour_primaries
 put_bits(>pb, 8, s->avctx->color_trc);   // 
transfer_characteristics
-- 
2.11.0
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

[libav-devel] [PATCH 13/21] vaapi_h265: Reduce the amount of padding in the stream

2017-08-12 Thread Mark Thompson 
It is not necessary to pad to the CTU size.  The CB size of 8x8 should be
sufficient, but due to constraints in the Intel driver (the one usable
implementation of this) it has to be padded to 16x16 like in H.264.
---
On 12/08/17 17:02, Anton Khirnov wrote:
> Quoting Mark Thompson (2017-08-11 01:37:06)
>>  
>> -priv->ctu_width = FFALIGN(ctx->surface_width,  32) / 32;
>> -priv->ctu_height= FFALIGN(ctx->surface_height, 32) / 32;
>> +// This seems to be an Intel driver constraint.  Despite MinCbSizeY
>> +// being 8, we are still required to encode at 16-pixel alignment and
>> +// then crop back (so 1080 lines is still encode as 1088 + cropping).
>> +priv->ctu_width = FFALIGN(ctx->surface_width,  16) / 16;
>> +priv->ctu_height= FFALIGN(ctx->surface_height, 16) / 16;
> 
> Is this related to the rest of the patch? Does not seem so.
> 
> Otherwise looks ok as far as I can tell.

Split off as enclosing patch.

- Mark


 libavcodec/vaapi_encode_h265.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/libavcodec/vaapi_encode_h265.c b/libavcodec/vaapi_encode_h265.c
index 477065e2c..165b6ffde 100644
--- a/libavcodec/vaapi_encode_h265.c
+++ b/libavcodec/vaapi_encode_h265.c
@@ -815,8 +815,11 @@ static av_cold int 
vaapi_encode_h265_configure(AVCodecContext *avctx)
 if (err < 0)
 return err;
 
-priv->ctu_width = FFALIGN(ctx->surface_width,  32) / 32;
-priv->ctu_height= FFALIGN(ctx->surface_height, 32) / 32;
+// This is an Intel driver constraint.  Despite MinCbSizeY being 8,
+// we are still required to encode at 16-pixel alignment and then
+// crop back (so 1080 lines is still encoded as 1088 + cropping).
+priv->ctu_width = FFALIGN(ctx->surface_width,  16) / 16;
+priv->ctu_height= FFALIGN(ctx->surface_height, 16) / 16;
 
 av_log(avctx, AV_LOG_VERBOSE, "Input %ux%u -> Surface %ux%u -> CTU 
%ux%u.\n",
avctx->width, avctx->height, ctx->surface_width,
-- 
2.11.0
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 01/14] lavc: Add coded bitstream read/write API

2017-08-12 Thread Diego Biurrun 
On Sat, Aug 12, 2017 at 07:21:49PM +0100, Mark Thompson wrote:
> On 12/08/17 09:40, Diego Biurrun wrote:
> > On Fri, Aug 11, 2017 at 12:36:56AM +0100, Mark Thompson wrote:
> >> --- /dev/null
> >> +++ b/libavcodec/cbs.h
> >> @@ -0,0 +1,274 @@
> >> +
> >> +/**
> >> + * Write the content of the fragment to its own internal buffer.
> >> + *
> >> + * Writes the content of all units and then assembles them into a new
> >> + * data buffer.  When modifying the content of decomposed units, this
> >> + * can be used to regenerate the bitstream form of units or the whole
> >> + * fragment so that it can be extracted for other use.
> >> + */
> >> +int ff_cbs_write_fragment_data(CodedBitstreamContext *ctx,
> >> +   CodedBitstreamFragment *frag);
> > 
> > It's odd to see a Brit like you use American double spaces after a period :)
> > I recently dropped the habit of double spaces, the benefit appears dubious
> > and since the use is not that widespread - even among USians it seems - so
> > that I fit in better with single spaces.
> > 
> > In case you're wondering or some outside reader is confused: this is not an
> > actionable review comment, it's just random stylistic musing from my side.
>
> As far as I know it isn't a pond-side thing, merely personal
> preference. And I do prefer it, especially in monospaced text.

Possibly I was confused by having to turn on French Spacing in LaTeX.
I certainly learned through using LaTeX that such a thing as two spaces
after a period was an existing convention.  But I think I only ever saw
it in US context and/or used by Americans.

> (the supreme arbiter of all human knowledge appears to agree
> with me: )

I love what you call Wikipedia :) - in Germany we like to call it the
all-knowing Trash Heap, which you possibly remember from Fraggle Rock.

> In any case, if you notice this sort of thing then I have to say I'm
> rather surprised that you've only just seen it now - I consistently
> use it everywhere, including IRC.

I'm a stickler for detail, but that's no news to you I assume. Contrary
to the legends surrounding me, I don't comment on everything I notice,
far from it. So yes, I noticed before, I just didn't mention it. ;)

Diego
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 02/14] lavc: Add coded bitstream read/write support for H.264

2017-08-12 Thread Diego Biurrun 
On Sat, Aug 12, 2017 at 07:23:50PM +0100, Mark Thompson wrote:
> On 12/08/17 09:44, Diego Biurrun wrote:
> > On Fri, Aug 11, 2017 at 12:36:57AM +0100, Mark Thompson wrote:
> >> --- /dev/null
> >> +++ b/libavcodec/cbs_h2645.c
> >> @@ -0,0 +1,997 @@
> >> +
> >> +#define FUNC(name) FUNC_H264(READWRITE, name)
> >> +#include "cbs_h264_syntax.c"
> >> +#undef FUNC
> >> +
> >> +#define FUNC(name) FUNC_H264(READWRITE, name)
> >> +#include "cbs_h264_syntax.c"
> >> +#undef FUNC
> > 
> > The convention is for these files to be called *_template.c.
> 
> Given that I'm making a load of these files and they have pretty 
> clearly-defined contents, can I create a new convention?

Well, they really are template files - what's the problem, the name
gets too long for your taste? cbs_h264_template.c would work as well,
unless you also have cbs_h264_something_else.c in the queue.

I do believe that naming conventions do help organize things sensibly,
so I'm reluctant to let this go to be honest...

Diego
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 02/14] lavc: Add coded bitstream read/write support for H.264

2017-08-12 Thread Mark Thompson 
On 12/08/17 09:44, Diego Biurrun wrote:
> On Fri, Aug 11, 2017 at 12:36:57AM +0100, Mark Thompson wrote:
>> --- /dev/null
>> +++ b/libavcodec/cbs_h2645.c
>> @@ -0,0 +1,997 @@
>> +
>> +#define FUNC(name) FUNC_H264(READWRITE, name)
>> +#include "cbs_h264_syntax.c"
>> +#undef FUNC
>> +
>> +#define FUNC(name) FUNC_H264(READWRITE, name)
>> +#include "cbs_h264_syntax.c"
>> +#undef FUNC
> 
> The convention is for these files to be called *_template.c.
Given that I'm making a load of these files and they have pretty 
clearly-defined contents, can I create a new convention?

- Mark
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 01/14] lavc: Add coded bitstream read/write API

2017-08-12 Thread Mark Thompson 
On 12/08/17 09:40, Diego Biurrun wrote:
> On Fri, Aug 11, 2017 at 12:36:56AM +0100, Mark Thompson wrote:
>> --- /dev/null
>> +++ b/libavcodec/cbs.h
>> @@ -0,0 +1,274 @@
>> +
>> +/**
>> + * Write the content of the fragment to its own internal buffer.
>> + *
>> + * Writes the content of all units and then assembles them into a new
>> + * data buffer.  When modifying the content of decomposed units, this
>> + * can be used to regenerate the bitstream form of units or the whole
>> + * fragment so that it can be extracted for other use.
>> + */
>> +int ff_cbs_write_fragment_data(CodedBitstreamContext *ctx,
>> +   CodedBitstreamFragment *frag);
>> +
> 
> It's odd to see a Brit like you use American double spaces after a period :)
> I recently dropped the habit of double spaces, the benefit appears dubious
> and since the use is not that widespread - even among USians it seems - so
> that I fit in better with single spaces.
> 
> In case you're wondering or some outside reader is confused: this is not an
> actionable review comment, it's just random stylistic musing from my side.

As far as I know it isn't a pond-side thing, merely personal preference (the 
supreme arbiter of all human knowledge appears to agree with me: 
).  And I do prefer it, 
especially in monospaced text.

In any case, if you notice this sort of thing then I have to say I'm rather 
surprised that you've only just seen it now - I consistently use it everywhere, 
including IRC.

- Mark
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH V2] libavcodec/mjpeg_qsv: Add the QSV MJPEG encoder

2017-08-12 Thread Diego Biurrun 
On Fri, Jul 21, 2017 at 10:08:15AM +0800, Huang, Zhengxu wrote:
> 
> 
> On 2017/7/20 20:07, Diego Biurrun wrote:
> > On Wed, Jul 19, 2017 at 02:41:31PM +0800, Huang, Zhengxu wrote:
> > > --- a/libavcodec/qsvenc.c
> > > +++ b/libavcodec/qsvenc.c
> > > @@ -568,6 +627,24 @@ FF_ENABLE_DEPRECATION_WARNINGS
> > > +static int qsv_retrieve_enc_jpeg_params(AVCodecContext *avctx, 
> > > QSVEncContext *q)
> > > +{
> > > +int ret = 0;
> > > +
> > > +ret = MFXVideoENCODE_GetVideoParam(q->session, >param);
> > pointless initialization
> In my opinion maybe keep this code much better in case the QSV MSDK modify
> mjpeg code logic in the later release.

I have seen code waiting more than ten years for some future change in
external libraries. It's better to modify code when the time comes than
wait in vain ;)

Diego
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] h264dec: add a CUVID hwaccel

2017-08-12 Thread Diego Biurrun 
On Thu, Jul 27, 2017 at 01:30:52PM +0300, Martin Storsjö wrote:
> On Thu, 27 Jul 2017, Anton Khirnov wrote:
> > Quoting Sean McGovern (2017-07-27 07:42:29)
> > > On Mon, Jul 24, 2017 at 12:50 PM, Anton Khirnov  wrote:
> > > > Quoting Luca Barbato (2017-07-24 18:21:20)
> > > >> On 24/07/2017 15:15, Anton Khirnov wrote:
> > > >> > Some parts of the code are based on a patch by
> > > >> > Timo Rothenpieler 
> > > 
> > > Looking at FATE, this may need a proper header guard -- or should the
> > > entire module be disabled on systems that don't have CUVID (like my
> > > Solaris instances)?
> > 
> > What part of FATE? I don't see anything related to this set.
> 
> make checkheaders fails.

This, Anton, is why I keep *begging* you to use Oracle.

Diego
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 14/14] hevc: Remove unused hevc_ps_enc.c

2017-08-12 Thread Anton Khirnov 
Quoting Mark Thompson (2017-08-11 01:37:09)
> Replaced with more complete implementation via coded bitstream infrastructure.
> ---
> Unchanged.

Ok

-- 
Anton Khirnov
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 13/14] qsvenc_hevc: Replace ad-hoc VPS writing with CBS implementation

2017-08-12 Thread Anton Khirnov 
Quoting Mark Thompson (2017-08-11 01:37:08)
> This copies more information which should be present from the SPS.
> It also fixes the value of vps_temporal_id_nesting_flag, which was
> previously incorrect for a single-layer stream (the standard states
> that it must be 1, and the reference decoder barfs if it isn't).
> ---
> Build system change only.

Looks ok. I'll try to test it on Windows later. Poke me if I forget.

-- 
Anton Khirnov
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 12/14] vaapi_h265: Add support for AUD NAL units

2017-08-12 Thread Anton Khirnov 
Quoting Mark Thompson (2017-08-11 01:37:07)
> Matching the H.264 encoder.
> ---

Looks ok

-- 
Anton Khirnov
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 11/14] vaapi_h265: Convert to use coded bitstream infrastructure

2017-08-12 Thread Anton Khirnov 
Quoting Mark Thompson (2017-08-11 01:37:06)
>  
> -priv->ctu_width = FFALIGN(ctx->surface_width,  32) / 32;
> -priv->ctu_height= FFALIGN(ctx->surface_height, 32) / 32;
> +// This seems to be an Intel driver constraint.  Despite MinCbSizeY
> +// being 8, we are still required to encode at 16-pixel alignment and
> +// then crop back (so 1080 lines is still encode as 1088 + cropping).
> +priv->ctu_width = FFALIGN(ctx->surface_width,  16) / 16;
> +priv->ctu_height= FFALIGN(ctx->surface_height, 16) / 16;

Is this related to the rest of the patch? Does not seem so.

Otherwise looks ok as far as I can tell.

-- 
Anton Khirnov
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 10/14] vaapi_h264: Add support for SEI recovery points

2017-08-12 Thread Anton Khirnov 
Quoting Mark Thompson (2017-08-11 01:37:05)
> Included by default with non-IDR intra frames.

LGTM

-- 
Anton Khirnov
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 2/2 V2] libavfilter/vf_vpp: Add common filters of the qsv vpp

2017-08-12 Thread Anton Khirnov 
Quoting Huang, Zhengxu (2017-08-04 05:15:26)
>From 0d3fcc7322b86e236596aad21f704b5844d14446 Mon Sep 17 00:00:00 2001
>From: "Huang, Zhengxu" 
>Date: Wed, 26 Jul 2017 04:50:06 +0800
>Subject: [PATCH 2/2] libavfilter/vf_vpp: Add common filters of the qsv vpp
>
>Add common filters of the qsv vpp features including scale denosie deinterlace
>frc crop and procAmp.
>
>Performance will be significantly reduced in the test if using cascade mode 
>just
>like qsv framerate + qsv scale + qsv deinterlace + qsv denoise in separated way
>no matter in system or video memmory cases. And the code is so redundant 
>because
>so much the same just as session and surface's creation and management. So we 
>add
>a common qsv filter.
>
>Usage:
>-hwaccel qsv -c:v h264_qsv -r 25 -i in -vf 
>vpp_qsv=w=iw/2:h=400:deinterlace=1:framerate=60:detail=50:denoise=50
>-b 2M -maxrate 3M -c:v h264_qsv -y out.h264
>
>Signed-off-by: ChaoX A Liu 
>Signed-off-by: Zhengxu Huang 
>Signed-off-by: Andrew Zhang 
>Change-Id: I130392ce722138c209ab658c5f03f0009b6e8024
>---
> configure|   2 +
> libavfilter/Makefile |   1 +
> libavfilter/allfilters.c |   1 +
> libavfilter/vf_vpp_qsv.c | 405 +++
> 4 files changed, 409 insertions(+)
> create mode 100644 libavfilter/vf_vpp_qsv.c
>
>diff --git a/configure b/configure
>index e6ea18a..70eee5e 100755
>--- a/configure
>+++ b/configure
>@@ -2535,6 +2535,8 @@ resample_filter_deps="avresample"
> scale_filter_deps="swscale"
> scale_qsv_filter_deps="libmfx"
> scale_vaapi_filter_deps="vaapi VAProcPipelineParameterBuffer"
>+vpp_qsv_filter_deps="libmfx"
>+vpp_qsv_filter_select="qsvvpp"
> 
> # examples
> decode_audio_example_deps="avcodec avutil"
>diff --git a/libavfilter/Makefile b/libavfilter/Makefile
>index 8d9ed6a..3f13d29 100644
>--- a/libavfilter/Makefile
>+++ b/libavfilter/Makefile
>@@ -94,6 +94,7 @@ OBJS-$(CONFIG_TRANSPOSE_FILTER)  += 
>vf_transpose.o
> OBJS-$(CONFIG_TRIM_FILTER)   += trim.o
> OBJS-$(CONFIG_UNSHARP_FILTER)+= vf_unsharp.o
> OBJS-$(CONFIG_VFLIP_FILTER)  += vf_vflip.o
>+OBJS-$(CONFIG_VPP_QSV_FILTER)+= qsvvpp.o vf_vpp_qsv.o

Needs the same changes you did to the overlay filter.

> OBJS-$(CONFIG_YADIF_FILTER)  += vf_yadif.o
> 
> OBJS-$(CONFIG_NULLSINK_FILTER)   += vsink_nullsink.o
>diff --git a/libavfilter/allfilters.c b/libavfilter/allfilters.c
>index dc59ccd..2b3a672 100644
>--- a/libavfilter/allfilters.c
>+++ b/libavfilter/allfilters.c
>@@ -117,6 +117,7 @@ void avfilter_register_all(void)
> REGISTER_FILTER(TRIM,   trim,   vf);
> REGISTER_FILTER(UNSHARP,unsharp,vf);
> REGISTER_FILTER(VFLIP,  vflip,  vf);
>+REGISTER_FILTER(VPP_QSV,vpp_qsv,vf);
> REGISTER_FILTER(YADIF,  yadif,  vf);
> 
> REGISTER_FILTER(COLOR,  color,  vsrc);
>diff --git a/libavfilter/vf_vpp_qsv.c b/libavfilter/vf_vpp_qsv.c
>new file mode 100644
>index 000..4e14245
>--- /dev/null
>+++ b/libavfilter/vf_vpp_qsv.c
>@@ -0,0 +1,405 @@
>+/*
>+ * This file is part of Libav.
>+ *
>+ * Libav is free software; you can redistribute it and/or
>+ * modify it under the terms of the GNU Lesser General Public
>+ * License as published by the Free Software Foundation; either
>+ * version 2.1 of the License, or (at your option) any later version.
>+ *
>+ * Libav is distributed in the hope that it will be useful,
>+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
>+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>+ * Lesser General Public License for more details.
>+ *
>+ * You should have received a copy of the GNU Lesser General Public
>+ * License along with Libav; if not, write to the Free Software
>+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 
>USA
>+ */
>+
>+#include 
>+#include "internal.h"
>+#include "formats.h"
>+#include "libavutil/eval.h"
>+#include "avfilter.h"
>+#include "libavutil/avassert.h"
>+#include "libavutil/opt.h"
>+#include "libavcodec/avcodec.h"
>+#include "libavformat/avformat.h"
>+#include "libavutil/pixdesc.h"
>+#include "qsvvpp.h"
>+
>+#define OFFSET(x) offsetof(VPPContext, x)
>+#define FLAGS AV_OPT_FLAG_VIDEO_PARAM
>+
>+/* number of video enhancement filters */
>+#define ENH_FILTERS_COUNT (5)
>+
>+typedef struct VPPContext{
>+const AVClass *class;
>+
>+FFQSVVPPContext *qsv;
>+
>+/* Video Enhancement Algorithms */
>+mfxExtVPPDeinterlacing  deinterlace_conf;
>+mfxExtVPPFrameRateConversion frc_conf;
>+mfxExtVPPDenoise denoise_conf;
>+mfxExtVPPDetail detail_conf;
>+mfxExtVPPProcAmp procamp_conf;
>+
>+int out_width;
>+int out_height;
>+
>+AVRational framerate;   ///< target framerate
>+/**
>+ * destination picture structure
>+

Re: [libav-devel] [PATCH 1/2 V6] libavfilter/overlay_qsv: Add QSV overlay vpp filter

2017-08-12 Thread Anton Khirnov 
Quoting Diego Biurrun (2017-08-12 10:10:30)
> On Fri, Aug 11, 2017 at 09:46:17AM +0800, Huang, Zhengxu wrote:
> > +s->surface_ptrs_out = 
> > av_mallocz_array(out_frames_hwctx->nb_surfaces,
> > +   
> > sizeof(*s->surface_ptrs_out));
> > +if (!s->surface_ptrs_out) {
> > +av_buffer_unref(_frames_ref);
> > +return AVERROR(ENOMEM);
> > +}
> > +
> > +for (i = 0; i < out_frames_hwctx->nb_surfaces; i++)
> > +s->surface_ptrs_out[i] = out_frames_hwctx->surfaces + i;
> > +s->nb_surface_ptrs_out = out_frames_hwctx->nb_surfaces;
> > +
> > +av_buffer_unref(>hw_frames_ctx);
> > +outlink->hw_frames_ctx = out_frames_ref;
> > +} else
> > +s->out_mem_mode = MFX_MEMTYPE_SYSTEM_MEMORY;
> > +
> > +/* extract the properties of the "master" session given to us */
> > +ret = MFXQueryIMPL(device_hwctx->session, );
> > +if (ret == MFX_ERR_NONE)
> > +ret = MFXQueryVersion(device_hwctx->session, );
> > +if (ret != MFX_ERR_NONE) {
> > +av_log(avctx, AV_LOG_ERROR, "Error querying the session 
> > attributes\n");
> > +return AVERROR_UNKNOWN;
> > +}
> 
> Using plain "else" instead of another if would be clearer.

That's not the same thing as what the code does now. Read it more
carefully.

> 
> > +/* create a "slave" session with those same properties, to be used for 
> > vpp*/
> 
> space before *
> 
> > +int ff_qsvvpp_create(AVFilterContext *avctx, QSVVPPContext **vpp, 
> > QSVVPPParam *param)
> > +{
> > +int ret = 0, i;
> 
> The init is pointless.
> 
> > +QSVVPPContext *s;
> > +
> > +s = av_mallocz(sizeof(*s));
> > +if (!s) {
> > +ret = AVERROR(ENOMEM);
> > +goto failed;
> > +}
> 
> No need for the goto here, you can just return directly.
> 
> > +/* Init each input's information*/
> 
> space before *
> 
> > +failed:
> > +if (s)
> > +ff_qsvvpp_free();
> > +
> > +return ret;
> > +}
> 
> The if is unnecessary if you dropped the goto above.

It's actually unnecessary in all cases, since ff_qsvvpp_free() is a
no-op on NULL.

> 
> > +int ff_qsvvpp_filter_frame(QSVVPPContext *s, AVFilterLink *inlink, AVFrame 
> > *picref)
> > +{
> > +in_frame = submit_frame(s, inlink, picref);
> > +if (!in_frame) {
> > +av_log(ctx, AV_LOG_ERROR, "Fail to submit frame on input[%d]\n",
> > +FF_INLINK_IDX(inlink));
> 
> "Failed", indentation is off.
> 
> > +return AVERROR(EINVAL);
> 
> Is this an error due to user-supplied data or user-supplied configuration?
> Because that's what EINVAL is for.

Should be ENOMEM

> 
> Don't you leak vpp->comp_conf.InputStream here?
> 

No, uninit is always called even if init fails.

-- 
Anton Khirnov
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 05/14] lavc: Add h264_metadata bitstream filter

2017-08-12 Thread Hendrik Leppkes 
On Sat, Aug 12, 2017 at 10:51 AM, Diego Biurrun  wrote:
> On Fri, Aug 11, 2017 at 12:37:00AM +0100, Mark Thompson wrote:
>> --- /dev/null
>> +++ b/libavcodec/h264_metadata_bsf.c
>> @@ -0,0 +1,511 @@
>> +
>> +typedef struct H264MetadataContext {
>> +const AVClass *class;
>
> Isn't the class member unused? Or did I just miss where this is used for 
> logging?
>

The priv_data of a BSF needs to have an AVClass member for AVOption support.

- Hendrik
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 1/2 V4] libavfilter/overlay_qsv: Add QSV overlay vpp filter

2017-08-12 Thread Diego Biurrun 
On Thu, Aug 10, 2017 at 09:21:25AM +0800, Huang, Zhengxu wrote:
> The second one I am not quite sure the format of the filters.texi. So I don't 
> do this in the new patch.

Look at filters.texi, experiment a bit; it should not be hard. If you have
trouble, just ask.

Diego
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 00/14] Coded bitstream editing (v6)

2017-08-12 Thread Diego Biurrun 
On Fri, Aug 11, 2017 at 12:36:55AM +0100, Mark Thompson wrote:
> The build system changes want some review, but after that hopefully 1-9 are 
> done.

OK, minor nits noted as replies.

Kudos for this whole effort from my side btw.

Diego
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 13/14] qsvenc_hevc: Replace ad-hoc VPS writing with CBS implementation

2017-08-12 Thread Diego Biurrun 
On Fri, Aug 11, 2017 at 12:37:08AM +0100, Mark Thompson wrote:
> --- a/configure
> +++ b/configure
> @@ -2288,7 +2288,7 @@ 
> h264_vaapi_encoder_deps="VAEncPictureParameterBufferH264"
>  h264_vaapi_encoder_select="vaapi_encode cbs_h264 golomb"
>  hevc_nvenc_encoder_deps="nvenc"
>  hevc_qsv_decoder_select="hevc_mp4toannexb_bsf hevc_parser hevc_qsv_hwaccel 
> qsvdec"
> -hevc_qsv_encoder_select="hevc_ps qsvenc"
> +hevc_qsv_encoder_select="qsvenc cbs_h265"

Please order the entries :)

Diego
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 11/14] vaapi_h265: Convert to use coded bitstream infrastructure

2017-08-12 Thread Diego Biurrun 
On Fri, Aug 11, 2017 at 12:37:06AM +0100, Mark Thompson wrote:
> --- a/configure
> +++ b/configure
> @@ -2290,7 +2290,7 @@ hevc_nvenc_encoder_deps="nvenc"
>  hevc_qsv_decoder_select="hevc_mp4toannexb_bsf hevc_parser hevc_qsv_hwaccel 
> qsvdec"
>  hevc_qsv_encoder_select="hevc_ps qsvenc"
>  hevc_vaapi_encoder_deps="VAEncPictureParameterBufferHEVC"
> -hevc_vaapi_encoder_select="vaapi_encode golomb"
> +hevc_vaapi_encoder_select="vaapi_encode cbs_h265"

Please order the entries :)

Diego
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 08/14] vaapi_h264: Convert to use coded bitstream infrastructure

2017-08-12 Thread Diego Biurrun 
On Fri, Aug 11, 2017 at 12:37:03AM +0100, Mark Thompson wrote:
> --- a/configure
> +++ b/configure
> @@ -2285,7 +2285,7 @@ h264_omx_encoder_deps="omx"
>  h264_qsv_decoder_select="h264_mp4toannexb_bsf h264_parser qsvdec 
> h264_qsv_hwaccel"
>  h264_qsv_encoder_select="qsvenc"
>  h264_vaapi_encoder_deps="VAEncPictureParameterBufferH264"
> -h264_vaapi_encoder_select="vaapi_encode golomb"
> +h264_vaapi_encoder_select="vaapi_encode cbs_h264"

Please order the entries :)

Diego
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 07/14] lavc: Add hevc_metadata bitstream filter

2017-08-12 Thread Diego Biurrun 
On Fri, Aug 11, 2017 at 12:37:02AM +0100, Mark Thompson wrote:
> --- /dev/null
> +++ b/libavcodec/h265_metadata_bsf.c
> @@ -0,0 +1,458 @@
> +
> +typedef struct H265MetadataContext {
> +const AVClass *class;

Unused, the same as with H.264?

Diego
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 05/14] lavc: Add h264_metadata bitstream filter

2017-08-12 Thread Diego Biurrun 
On Fri, Aug 11, 2017 at 12:37:00AM +0100, Mark Thompson wrote:
> --- /dev/null
> +++ b/libavcodec/h264_metadata_bsf.c
> @@ -0,0 +1,511 @@
> +
> +typedef struct H264MetadataContext {
> +const AVClass *class;

Isn't the class member unused? Or did I just miss where this is used for 
logging?

Diego
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 02/14] lavc: Add coded bitstream read/write support for H.264

2017-08-12 Thread Diego Biurrun 
On Fri, Aug 11, 2017 at 12:36:57AM +0100, Mark Thompson wrote:
> --- /dev/null
> +++ b/libavcodec/cbs_h2645.c
> @@ -0,0 +1,997 @@
> +
> +#define FUNC(name) FUNC_H264(READWRITE, name)
> +#include "cbs_h264_syntax.c"
> +#undef FUNC
> +
> +#define FUNC(name) FUNC_H264(READWRITE, name)
> +#include "cbs_h264_syntax.c"
> +#undef FUNC

The convention is for these files to be called *_template.c.

Diego
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 01/14] lavc: Add coded bitstream read/write API

2017-08-12 Thread Diego Biurrun 
On Fri, Aug 11, 2017 at 12:36:56AM +0100, Mark Thompson wrote:
> --- /dev/null
> +++ b/libavcodec/cbs.c
> @@ -0,0 +1,461 @@
> +
> +int ff_cbs_init(CodedBitstreamContext *ctx,
> +enum AVCodecID codec_id, void *log_ctx)
> +{
> +ctx->log_ctx = log_ctx;
> +
> +ctx->codec = type;

nit: align

> +void ff_cbs_fragment_uninit(CodedBitstreamContext *ctx,
> +CodedBitstreamFragment *frag)
> +{
> +av_freep(>data);
> +frag->data_size = 0;
> +frag->data_bit_padding = 0;

same

> +int ff_cbs_read(CodedBitstreamContext *ctx,
> +CodedBitstreamFragment *frag,
> +const uint8_t *data, size_t size)
> +{
> +int err;
> +
> +memset(frag, 0, sizeof(*frag));
> +
> +// (We won't write to this during split.)
> +frag->data  = (uint8_t*)data;

nit: space before *

> +int ff_cbs_write_packet(CodedBitstreamContext *ctx,
> +   AVPacket *pkt,
> +   CodedBitstreamFragment *frag)

Indentation is off.

> +int ff_cbs_insert_unit_content(CodedBitstreamContext *ctx,
> +   CodedBitstreamFragment *frag,
> +   int position, uint32_t type,
> +   void *content)
> +{
> +frag->units[position].type= type;
> +frag->units[position].content = content;
> +frag->units[position].content_external = 1;

nit: align

> --- /dev/null
> +++ b/libavcodec/cbs.h
> @@ -0,0 +1,274 @@
> +
> +/**
> + * Write the content of the fragment to its own internal buffer.
> + *
> + * Writes the content of all units and then assembles them into a new
> + * data buffer.  When modifying the content of decomposed units, this
> + * can be used to regenerate the bitstream form of units or the whole
> + * fragment so that it can be extracted for other use.
> + */
> +int ff_cbs_write_fragment_data(CodedBitstreamContext *ctx,
> +   CodedBitstreamFragment *frag);
> +

It's odd to see a Brit like you use American double spaces after a period :)
I recently dropped the habit of double spaces, the benefit appears dubious
and since the use is not that widespread - even among USians it seems - so
that I fit in better with single spaces.

In case you're wondering or some outside reader is confused: this is not an
actionable review comment, it's just random stylistic musing from my side.

Diego
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 1/2 V6] libavfilter/overlay_qsv: Add QSV overlay vpp filter

2017-08-12 Thread Diego Biurrun 
On Fri, Aug 11, 2017 at 09:46:17AM +0800, Huang, Zhengxu wrote:
> From c4e8d8c22f2100bd9bffaccca628c5bd3bfd7281 Mon Sep 17 00:00:00 2001
> From: "Huang, Zhengxu" 
> Date: Tue, 25 Jul 2017 21:55:50 +0800
> Subject: [PATCH] libavfilter/overlay_qsv: Add QSV overlay vpp filter

No need for the module prefix as you are adding it and overlay_qsv does
not exist yet, so you cannot be modifying it.

> Add intel QSV overlay filter. Now it supports two input. And it also supports
> the second input scale(implicit) during composition compared to the sw 
> overlay.

There is no need to repeat that you added a filter because you just said
that you added a filter in the title and hence repeating that you added a
filter is as redundant as me repeating here that you should not repeat
what you just said because you already said it. ;)

Instead:

The filter supports two inputs and (implicitly) scaling the second input
during composition, unlike the software overlay.

> Code has been separated into common interface part and qsv overlay implement 
> part.

The code has been separated into common interface and qsv overlay 
implementation.

> The common part mainly creates the qsv session and manages the surface which 
> are

Either "surface which is" or "surfaces which are".

> nearly the same for all qsv filters. So the qsvvpp.c/qsvvpp.h API can be used 
> by
> other QSV vpp filters to reduce code redundancy.
> 
> Usage:
>  -hwaccel qsv -c:v mpeg2_qsv -r 25 -i in.m2v -hwaccel qsv -c:v h264_qsv -i 
> in.h264 -filter_complex
> "overlay_qsv=eof_action=repeat:x=(W-w)/2:y=(H-h)/2"  -b 2M -maxrate 3M  -c:v 
> h264_qsv -y out.h264
> 
> two input should have different size otherwise one will be completely covered 
> or you need scale the
> second input as follow:

Two inputs should have different sizes otherwise one will be completely
covered or you need to scale the second input as follows:

> --- /dev/null
> +++ b/libavfilter/qsvvpp.c
> @@ -0,0 +1,737 @@
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with Libav; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 
> USA
> + */
> +/**
> + * @file
> + * Intel Quick Sync Video VPP base function
> + */

nit: Add an empty line between the two comment blocks..

> +#include "libavutil/common.h"
> +#include "libavutil/mathematics.h"
> +#include "libavutil/hwcontext.h"
> +#include "libavutil/hwcontext_qsv.h"
> +#include "libavutil/time.h"
> +#include "libavutil/pixdesc.h"
> +#include "internal.h"
> +#include "qsvvpp.h"
> +#include "video.h"

.. and between the libavutil header block and the other headers.

> +#define IS_VIDEO_MEMORY(mode) (mode & 
> (MFX_MEMTYPE_VIDEO_MEMORY_DECODER_TARGET | \
> +   MFX_MEMTYPE_VIDEO_MEMORY_PROCESSOR_TARGET))
> +#define IS_OPAQUE_MEMORY(mode) (mode & MFX_MEMTYPE_OPAQUE_FRAME)
> +#define IS_SYSTEM_MEMORY(mode) (mode & MFX_MEMTYPE_SYSTEM_MEMORY)

This would be more readable like this:

  #define IS_VIDEO_MEMORY(mode)  (mode & 
(MFX_MEMTYPE_VIDEO_MEMORY_DECODER_TARGET | \
  
MFX_MEMTYPE_VIDEO_MEMORY_PROCESSOR_TARGET))
  #define IS_OPAQUE_MEMORY(mode) (mode & MFX_MEMTYPE_OPAQUE_FRAME)
  #define IS_SYSTEM_MEMORY(mode) (mode & MFX_MEMTYPE_SYSTEM_MEMORY)

> +typedef struct QSVFrame {
> +AVFrame  *frame;
> +mfxFrameSurface1 *surface;
> +mfxFrameSurface1  surface_internal;  ///< for system memory

Why is this a Doxygen comment? More below ..

> +/* abstract struct for all QSV filters */
> +struct QSVVPPContext {
> +mfxSession  session;
> +int (*filter_frame) (AVFilterLink *outlink, AVFrame *frame);///< 
> callback function
> +enum AVPixelFormat  out_sw_format;   ///< Real output format
> +mfxVideoParam   vpp_param;
> +mfxFrameInfo   *frame_infos; ///< frame info for each input
> +
> +/* members releated to the input/output surface */

rel_ated

> +/* MFXVPP extern parameters*/

space before *

> +static const AVRational default_tb = {1, 9};

and inside {}

> +if (req->Type & MFX_MEMTYPE_FROM_VPPIN) {
> +resp->mids = av_mallocz(s->nb_surface_ptrs_in * sizeof(*resp->mids));
> +if (!resp->mids)
> +return AVERROR(ENOMEM);
> +resp->NumFrameActual = s->nb_surface_ptrs_in;
> +} else {
> +resp->mids = av_mallocz(s->nb_surface_ptrs_in * sizeof(*resp->mids));
> +if (!resp->mids)
> +return AVERROR(ENOMEM);

Either there is an in/out typo or the code is duplicated and could be
moved before the if.

> +static int pix_fmt_to_mfx_fourcc(int format)
> +{
> +switch(format) {

switch (

> +static void clear_frame_list(QSVFrame **list)
> +{
> +QSVFrame *frame;
> +
> +while (*list) {
> +frame = *list;
> +*list = (*list)->next;
> +av_frame_free(>frame);
> +av_freep();
> +}
> +}

You could move the

Re: [libav-devel] [PATCH] xwddec: support 8bpp grayscale

2017-08-12 Thread Luca Barbato 
On 12/08/2017 09:02, Diego Biurrun wrote:
> From: Piotr Bandurski 
> 
> (cherry picked from commit b9c94e826e7551027754ecfa60e3e487e0c28fcb)
> Signed-off-by: Diego Biurrun 
> ---
> 
> Includes the check for bpp == pixdepth now.
> 
>  libavcodec/xwddec.c | 7 +--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/libavcodec/xwddec.c b/libavcodec/xwddec.c
> index 387b697491..3be66dbb65 100644
> --- a/libavcodec/xwddec.c
> +++ b/libavcodec/xwddec.c
> @@ -155,10 +155,13 @@ static int xwd_decode_frame(AVCodecContext *avctx, void 
> *data,
>  switch (vclass) {
>  case XWD_STATIC_GRAY:
>  case XWD_GRAY_SCALE:
> -if (bpp != 1)
> +if (bpp != 1 && bpp != 8 || bpp != pixdepth)
>  return AVERROR_INVALIDDATA;
> -if (pixdepth == 1)
> +if (pixdepth == 1) {
>  avctx->pix_fmt = AV_PIX_FMT_MONOWHITE;
> +} else if (pixdepth == 8) {
> +avctx->pix_fmt = AV_PIX_FMT_GRAY8;
> +}
>  break;
>  case XWD_STATIC_COLOR:
>  case XWD_PSEUDO_COLOR:
> 

Sounds good.
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

[libav-devel] [PATCH] xwddec: support 8bpp grayscale

2017-08-12 Thread Diego Biurrun 
From: Piotr Bandurski 

(cherry picked from commit b9c94e826e7551027754ecfa60e3e487e0c28fcb)
Signed-off-by: Diego Biurrun 
---

Includes the check for bpp == pixdepth now.

 libavcodec/xwddec.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/libavcodec/xwddec.c b/libavcodec/xwddec.c
index 387b697491..3be66dbb65 100644
--- a/libavcodec/xwddec.c
+++ b/libavcodec/xwddec.c
@@ -155,10 +155,13 @@ static int xwd_decode_frame(AVCodecContext *avctx, void 
*data,
 switch (vclass) {
 case XWD_STATIC_GRAY:
 case XWD_GRAY_SCALE:
-if (bpp != 1)
+if (bpp != 1 && bpp != 8 || bpp != pixdepth)
 return AVERROR_INVALIDDATA;
-if (pixdepth == 1)
+if (pixdepth == 1) {
 avctx->pix_fmt = AV_PIX_FMT_MONOWHITE;
+} else if (pixdepth == 8) {
+avctx->pix_fmt = AV_PIX_FMT_GRAY8;
+}
 break;
 case XWD_STATIC_COLOR:
 case XWD_PSEUDO_COLOR:
-- 
2.11.0

___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 2/2] xwddec: Check bpp more completely

2017-08-12 Thread Diego Biurrun 
On Fri, Aug 11, 2017 at 09:35:32PM +0200, Luca Barbato wrote:
> On 11/08/2017 20:46, Diego Biurrun wrote:
> > From: Michael Niedermayer 
> > 
> > Fixes out of array access
> > Fixes: 1399/clusterfuzz-testcase-minimized-4866094172995584
> > Bug-Id: CVE-2017-9991
> > CC: libav-sta...@libav.org
> > 
> > Found-by: continuous fuzzing process 
> > https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg
> > Signed-off-by: Michael Niedermayer 
> > (cherry picked from commit 441026fcb13ac23aa10edc312bdacb6445a0ad06)
> > Signed-off-by: Diego Biurrun 
> > ---
> >  libavcodec/xwddec.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/libavcodec/xwddec.c b/libavcodec/xwddec.c
> > index cae6287987..22b6e52d5d 100644
> > --- a/libavcodec/xwddec.c
> > +++ b/libavcodec/xwddec.c
> > @@ -157,9 +157,9 @@ static int xwd_decode_frame(AVCodecContext *avctx, void 
> > *data,
> >  case XWD_GRAY_SCALE:
> >  if (bpp != 1 && bpp != 8)
> >  return AVERROR_INVALIDDATA;
> > -if (pixdepth == 1) {
> > +if (bpp == 1 && pixdepth == 1) {
> >  avctx->pix_fmt = AV_PIX_FMT_MONOWHITE;
> > -} else if (pixdepth == 8) {
> > +} else if (bpp == 8 && pixdepth == 8) {
> >  avctx->pix_fmt = AV_PIX_FMT_GRAY8;
> >  }
> >  break;
> > 
> 
> if the condition bpp != pixdepth is not valid for grayscale maybe would
> be better to error out with something like
> 
>if (bpp != 1 && bpp != 8 && bpp != pixdepth)

You want || for the third condition there, sending an updated patch now.

Diego
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel