Re: [FFmpeg-devel] [PATCH v2 1/4] lavc/vaapi_encode: add EQUAL_MULTI_ROWS support for slice structure

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of
> Mark Thompson
> Sent: Tuesday, July 28, 2020 05:52
> To: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH v2 1/4] lavc/vaapi_encode: add
> EQUAL_MULTI_ROWS support for slice structure
> 
> On 19/07/2020 08:00, Linjie Fu wrote:
> > On Thu, Jun 18, 2020 at 1:36 PM Linjie Fu 
> wrote:
> >>
> >> On Tue, May 12, 2020 at 9:49 PM Linjie Fu  wrote:
> >>>
> >>> VA_ENC_SLICE_STRUCTURE_EQUAL_MULTI_ROWS is added to in the
> latest
> >>> libva (1.8.0) which matches the hardware behaviour:
> >>>
> >>> /** \brief Driver supports any number of rows per slice but they must
> be the same
> >>> *   for all slices except for the last one, which must be equal or 
> >>> smaller
> >>> *   to the previous slices. */
> >>>
> >>> And VA_ENC_SLICE_STRUCTURE_EQUAL_ROWS is kind of deprecated
> for iHD
> >>> since it's somehow introduced in [1] however misleading from what we
> >>> actually handle, and one row per slice would not get a good quality.
> >>>
> >>> Caps query support in driver is WIP, and this would fix the multi slice
> >>> encoding for VAEntrypointEncSliceLP.
> >>>
> >>>
> [1] 4d10fd644778>
> >>>
> >>> Signed-off-by: Linjie Fu 
> >>> ---
> >>>   libavcodec/vaapi_encode.c | 3 +++
> >>>   1 file changed, 3 insertions(+)
> >>>
> >>> diff --git a/libavcodec/vaapi_encode.c b/libavcodec/vaapi_encode.c
> >>> index cb05ebd..234618a 100644
> >>> --- a/libavcodec/vaapi_encode.c
> >>> +++ b/libavcodec/vaapi_encode.c
> >>> @@ -1888,6 +1888,9 @@ static av_cold int
> vaapi_encode_init_slice_structure(AVCodecContext *avctx)
> >>>   req_slices = avctx->slices;
> >>>   }
> >>>   if (slice_structure & VA_ENC_SLICE_STRUCTURE_ARBITRARY_ROWS
> ||
> >>> +#if VA_CHECK_VERSION(1, 8, 0)
> >>> +slice_structure &
> VA_ENC_SLICE_STRUCTURE_EQUAL_MULTI_ROWS ||
> >>> +#endif
> >>>   slice_structure &
> VA_ENC_SLICE_STRUCTURE_ARBITRARY_MACROBLOCKS) {
> >>>   ctx->nb_slices  = req_slices;
> >>>   ctx->slice_size = ctx->slice_block_rows / ctx->nb_slices;
> >>> --
> >>> 2.7.4
> >>
> >> Full support is provided in libva[1] and media-driver[2], and we've
> >> observed it works for multi slice encoding for AVC. (300+ cases)
> >>
> >> Prefer to apply this soon with commit message refined.
> >>
> > Applied, thx.
> Treating EQUAL_MULTI_ROWS in the same way as the arbitrary-size cases is
> just wrong.
> 
> Consider 9 rows, 4 slices - we pick 4 slices with sizes { 3, 2, 2, 2 }, which
> EQUAL_MULTI_ROWS does not allow.
> 
> It isn't possible to split the frame into 4 slices at all with the
> EQUAL_MULTI_ROWS structure - the closest options are 3 slices with sizes { 3,
> 3, 3 } or 5 slices with sizes { 2, 2, 2, 2, 1 }.
> 
> You can see the same problem at 1080p with five slices.

Umm.. saw the conflicts with the definition/notation in LIBVA.

Actually, I doubt whether Hardware still have such limitation which needs the 
last
slice to be no larger than the previous. In the case of 9 rows, 4 slices, the 
current
implementation of rounding works well. {3, 2, 2, 2} .

Had sent a mail to the media-driver/libva scrum team to double confirm this, 
thx.

- Linjie




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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/6] vaapi_encode_h265: Remove confusing and redundant tile options

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of
> Mark Thompson
> Sent: Wednesday, July 29, 2020 06:50
> To: ffmpeg-devel@ffmpeg.org
> Subject: [FFmpeg-devel] [PATCH 1/6] vaapi_encode_h265: Remove
> confusing and redundant tile options
> 
> The tile_rows/cols options currently do a confusingly different thing to
> the options of the same name on other encoders like libvpx and libaom.
> There is no backward-compatibility reason to implement the log2 behaviour
> as there was for libaom, so just get rid of them entirely.

Ok, previously I'm following the implementation in librav1e [1] which uses a 
non-log2
value as the input of "tile-rows" and "tile-columns".
(Do we need to make it consistent as well?)

[1] https://github.com/FFmpeg/FFmpeg/blob/master/libavcodec/librav1e.c#L567

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] lavc/qsvenc_hevc: add qmax/qmin support for HEVC encoding

[Fu, Linjie]

> From: Zhong Li 
> Sent: Thursday, March 12, 2020 10:07
> To: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Cc: Rogozhkin, Dmitry V ; Fu, Linjie
> 
> Subject: Re: [FFmpeg-devel] [PATCH] lavc/qsvenc_hevc: add qmax/qmin
> support for HEVC encoding
> 
> Linjie Fu  于2020年3月11日周三 下午6:44写道：
> >
> > Add qmax/qmin support for HEVC software bitrate control(SWBRC).
> >
> > Limitations:
> > - RateControlMethod != MFX_RATECONTROL_CQP
> > - with EXTBRC ON
> >
> > Signed-off-by: Dmitry Rogozhkin 
> > Signed-off-by: Linjie Fu 
> > ---
> >
> > Relative code in MSDK for the limitation:
> > https://github.com/Intel-Media-
> SDK/MediaSDK/blob/master/_studio/mfx_lib/encode_hw/hevc/agnostic/g9
> /hevcehw_g9_legacy.cpp#L4267
> >
> >  libavcodec/qsvenc.c  | 11 +--
> >  libavcodec/qsvenc_hevc.c |  2 ++
> >  2 files changed, 7 insertions(+), 6 deletions(-)
> >
> > diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c
> > index 52b4e43..2c22eb7 100644
> > --- a/libavcodec/qsvenc.c
> > +++ b/libavcodec/qsvenc.c
> > @@ -732,6 +732,11 @@ FF_ENABLE_DEPRECATION_WARNINGS
> >  if (q->adaptive_b >= 0)
> >  q->extco2.AdaptiveB = q->adaptive_b ?
> MFX_CODINGOPTION_ON : MFX_CODINGOPTION_OFF;
> >  #endif
> > +}
> > +
> > +if (avctx->codec_id == AV_CODEC_ID_H264 || avctx->codec_id ==
> AV_CODEC_ID_HEVC) {
> > +if (q->extbrc >= 0)
> > +q->extco2.ExtBRC = q->extbrc ? MFX_CODINGOPTION_ON :
> MFX_CODINGOPTION_OFF;
> >
> >  #if QSV_VERSION_ATLEAST(1, 9)
> >  if (avctx->qmin >= 0 && avctx->qmax >= 0 && avctx->qmin > 
> > avctx-
> >qmax) {
> > @@ -747,12 +752,6 @@ FF_ENABLE_DEPRECATION_WARNINGS
> >  q->extco2.MaxQPP = q->extco2.MaxQPB = q->extco2.MaxQPI;
> >  }
> >  #endif
> > -}
> > -
> > -if (avctx->codec_id == AV_CODEC_ID_H264 || avctx->codec_id ==
> AV_CODEC_ID_HEVC) {
> > -if (q->extbrc >= 0)
> > -q->extco2.ExtBRC = q->extbrc ? MFX_CODINGOPTION_ON :
> MFX_CODINGOPTION_OFF;
> > -
> >  q->extco2.Header.BufferId = MFX_EXTBUFF_CODING_OPTION2;
> >  q->extco2.Header.BufferSz = sizeof(q->extco2);
> >
> > diff --git a/libavcodec/qsvenc_hevc.c b/libavcodec/qsvenc_hevc.c
> > index 27e2232..3bdca7d 100644
> > --- a/libavcodec/qsvenc_hevc.c
> > +++ b/libavcodec/qsvenc_hevc.c
> > @@ -262,6 +262,8 @@ static const AVCodecDefault qsv_enc_defaults[] = {
> >  // same as the x264 default
> >  { "g", "248"   },
> >  { "bf","8" },
> > +{ "qmin",  "-1"},
> > +{ "qmax",  "-1"},
> >  { "trellis",   "-1"},
> >  { "flags", "+cgop" },
> >  #if FF_API_PRIVATE_OPT
> > --
> > 2.7.4
> 
> LGTM, will apply
A kind ping.

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] lavfi/vf_vpp_qsv: fix the infinite loop while framerate lower than input

[Fu, Linjie]

> From: Zhong Li 
> Sent: Sunday, April 19, 2020 23:00
> To: Fu, Linjie 
> Cc: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH] lavfi/vf_vpp_qsv: fix the infinite loop
> while framerate lower than input
> 
> Fu, Linjie  于2020年2月29日周六 下午5:35写道：
> >
> > > -Original Message-
> > > From: Zhong Li 
> > > Sent: Saturday, February 29, 2020 13:14
> > > To: FFmpeg development discussions and patches  > > de...@ffmpeg.org>
> > > Cc: Fu, Linjie 
> > > Subject: Re: [FFmpeg-devel] [PATCH] lavfi/vf_vpp_qsv: fix the infinite
> loop
> > > while framerate lower than input
> > >
> > > Linjie Fu  于2020年2月28日周五 下午11:34写
> 道：
> > > >
> > > > There are frame droppings in frc while converting into a lower
> framerate,
> > > > and MSDK returns ERROR_MORE_DATA which should be ignored.
> > >
> > > Should be fixed in MSDK instead of working around in FFmpeg?
> >
> > MSDK made decision regarding frame rate conversion. If it's the framerate
> down case,
> > FRC would skip frame without producing an output [1], and request a new
> input frame.
> 
> I can't see the benefit to use MSDK framerate conversion function. Is
> it a good idea to drop it and use ffmpeg native fps filter instead?

Reconsidering this, leaving the filter buggy doesn't seem to be comfortable to 
me,
hence IMHO it'll be better to have this bug fixed.

Ping for this, thx.

- Linjie

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/3] lavc/avcodec: Add caps for the support of variable dimension encoding

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of
> Lynne
> Sent: Thursday, June 18, 2020 16:04
> To: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 1/3] lavc/avcodec: Add caps for the
> support of variable dimension encoding
> 
> Jun 18, 2020, 04:02 by linjie...@intel.com:
> 
> >> From: ffmpeg-devel  On Behalf Of
> Fu,
> >> Linjie
> >> Sent: Tuesday, June 9, 2020 17:02
> >> To: FFmpeg development discussions and patches  >> de...@ffmpeg.org>
> >> Subject: Re: [FFmpeg-devel] [PATCH 1/3] lavc/avcodec: Add caps for the
> >> support of variable dimension encoding
> >>
> >> > From: ffmpeg-devel  On Behalf
> Of
> >> > Linjie Fu
> >> > Sent: Monday, June 8, 2020 23:11
> >> > To: FFmpeg development discussions and patches  >> > de...@ffmpeg.org>
> >> > Subject: Re: [FFmpeg-devel] [PATCH 1/3] lavc/avcodec: Add caps for the
> >> > support of variable dimension encoding
> >> >
> >> > > From: "Anton Khirnov" 
> >> > > Sent Time: 2020-06-08 18:50:43 (Monday)
> >> > > To: "FFmpeg development discussions and patches"  >> > de...@ffmpeg.org>
> >> > > Cc:
> >> > > Subject: Re: [FFmpeg-devel] [PATCH 1/3] lavc/avcodec: Add caps for
> the
> >> > support  of variable dimension encoding
> >> > >
> >> > > Quoting Linjie Fu (2020-06-08 10:58:03)
> >> > > > And declare AV_CODEC_CAP_VARIABLE_DIMENSIONS for rawvideo
> >> and
> >> > > > wrapped_avframe.
> >> > > >
> >> > > > Signed-off-by: Linjie Fu 
> >> > > > ---
> >> > > >  doc/APIchanges   | 3 +++
> >> > > >  fftools/cmdutils.c   | 2 ++
> >> > > >  libavcodec/avcodec.h | 4 +++-
> >> > > >  libavcodec/codec.h   | 5 +
> >> > > >  libavcodec/rawenc.c  | 1 +
> >> > > >  libavcodec/version.h | 2 +-
> >> > > >  libavcodec/wrapped_avframe.c | 1 +
> >> > > >  7 files changed, 16 insertions(+), 2 deletions(-)
> >> > >
> >> > > During the last iteration, I asked how is this preferable to just 
> >> > > making
> >> > > a new encoder instance. Don't think I got a sufficient reply.
> >> >
> >> > Thanks Anton for the remind, indeed making a new encoder instance
> >> would
> >> > be
> >> > more general and suitable for all encoders, with resolution changing in
> key
> >> > frames.
> >> >
> >> > In this patch set, I prefer to restrict the implementation/support to
> >> > rawvideo and wrapped_avframe encoders, since they don't need to be
> >> > recreated
> >> > when resolution/dimension changes and are required. at this moment.
> >> >
> >> > Also as we've discussed about whether it's worthwhile:
> >> >
> >> > >> Do we get sufficient benefits from having parameter change
> capability
> >> > inside
> >> > >> encoders over just creating a new encoder instance when needed?
> Do
> >> > people
> >> > >> really need to change resolution mid-GOP?
> >> >
> >> > This implementation didn't touch the logic of encoders which supports
> >> > variable
> >> > resolution encoding on inter frames(vp9, av1), since such
> >> > enhancement/requirement
> >> > seems to be rare for now.
> >>
> >> Sent the encoder instance recreate patch set for review as a follow-up
> step,
> >> please
> >> help to review:
> >> https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=1450
> >>
> >
> > Will apply this patch set (1-3) as discussed in:
> > https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2020-June/264649.html
> >
> 
> You said you'd only apply the first patch, not the entire patchset in your
> previous email.
> I'd like to wait to have a well engineered solution we can agree on instead of
> rushing this in, I too have an interest in variable frame size encoding.

Sure and totally agree, will only apply auto scale patch.
Thanks for the input.

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/3] lavc/avcodec: Add caps for the support of variable dimension encoding

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of Fu,
> Linjie
> Sent: Tuesday, June 9, 2020 17:02
> To: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 1/3] lavc/avcodec: Add caps for the
> support of variable dimension encoding
> 
> > From: ffmpeg-devel  On Behalf Of
> > Linjie Fu
> > Sent: Monday, June 8, 2020 23:11
> > To: FFmpeg development discussions and patches  > de...@ffmpeg.org>
> > Subject: Re: [FFmpeg-devel] [PATCH 1/3] lavc/avcodec: Add caps for the
> > support of variable dimension encoding
> >
> > > From: "Anton Khirnov" 
> > > Sent Time: 2020-06-08 18:50:43 (Monday)
> > > To: "FFmpeg development discussions and patches"  > de...@ffmpeg.org>
> > > Cc:
> > > Subject: Re: [FFmpeg-devel] [PATCH 1/3] lavc/avcodec: Add caps for the
> > support of variable dimension encoding
> > >
> > > Quoting Linjie Fu (2020-06-08 10:58:03)
> > > > And declare AV_CODEC_CAP_VARIABLE_DIMENSIONS for rawvideo
> and
> > > > wrapped_avframe.
> > > >
> > > > Signed-off-by: Linjie Fu 
> > > > ---
> > > >  doc/APIchanges   | 3 +++
> > > >  fftools/cmdutils.c   | 2 ++
> > > >  libavcodec/avcodec.h | 4 +++-
> > > >  libavcodec/codec.h   | 5 +
> > > >  libavcodec/rawenc.c  | 1 +
> > > >  libavcodec/version.h | 2 +-
> > > >  libavcodec/wrapped_avframe.c | 1 +
> > > >  7 files changed, 16 insertions(+), 2 deletions(-)
> > >
> > > During the last iteration, I asked how is this preferable to just making
> > > a new encoder instance. Don't think I got a sufficient reply.
> >
> > Thanks Anton for the remind, indeed making a new encoder instance
> would
> > be
> > more general and suitable for all encoders, with resolution changing in key
> > frames.
> >
> > In this patch set, I prefer to restrict the implementation/support to
> > rawvideo and wrapped_avframe encoders, since they don't need to be
> > recreated
> > when resolution/dimension changes and are required. at this moment.
> >
> > Also as we've discussed about whether it's worthwhile:
> >
> > >> Do we get sufficient benefits from having parameter change capability
> > inside
> > >> encoders over just creating a new encoder instance when needed? Do
> > people
> > >> really need to change resolution mid-GOP?
> >
> > This implementation didn't touch the logic of encoders which supports
> > variable
> > resolution encoding on inter frames(vp9, av1), since such
> > enhancement/requirement
> > seems to be rare for now.
> 
> Sent the encoder instance recreate patch set for review as a follow-up step,
> please
> help to review:
> https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=1450

Will apply this patch set (1-3) as discussed in:
https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2020-June/264649.html

Thx.

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v2 4/5] fftools/ffmpeg: flush and recreate encoder instance if resolution changes

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of
> Nicolas George
> Sent: Tuesday, June 16, 2020 21:40
> To: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH v2 4/5] fftools/ffmpeg: flush and
> recreate encoder instance if resolution changes
> 
> Fu, Linjie (12020-06-16):
> > Chapter 2.3 Parameter Sets in < High Efficiency Video Coding (HEVC)
> > Algorithms and Architectures> [1]:
> 
> That is ONE codec. Not all of them.
> 
> > Indeed, the definition in spec is just the conformance, and how an encoder
> is
> > implemented in Libavcodec (and external library if any) is the thing really
> matters.
> >
> > For encoders like libx264[2], libx265[3], libopenh264[4], if
> AV_CODEC_FLAG_GLOBAL_HEADER
> > flag is declared, the parameter Sets would be copied to extra data as the
> global header.
> > If not, parameter sets would be kept in the original bitstream, since it's
> fundamentally
> > supported in the encoder.
> 
> IIRC, encapsulation standards mandate one or the other for their
> respective formats. We cannot choose.
> 
> Fact is, you cannot concatenate two packet streams and expect it to
> work.
> 
Okay, there are also some discussions on IRC yesterday about this, and one
Idea from Anton is to provide some bigger-picture solutions for concatenating
streams, maybe a bitstream filter to accommodate streamcopy.

Hence, I'd like to keep it open for now until we reach the concensus.

And I'd like to apply the patch set [1] soon for auto scale if no objections, 
as the first step
and touches raw video only for restricted usage for now (which is documented 
clearly).
This is already being waited for some time.

- Linjie

[1] https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=1434

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v2 4/5] fftools/ffmpeg: flush and recreate encoder instance if resolution changes

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of
> Nicolas George
> Sent: Tuesday, June 16, 2020 18:55
> To: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH v2 4/5] fftools/ffmpeg: flush and
> recreate encoder instance if resolution changes
> 
> Fu, Linjie (12020-06-12):
> > IIRC, the global header in extra data is optional in codec level.
> 
> Where did you take that?

Chapter 2.3 Parameter Sets in < High Efficiency Video Coding (HEVC)
Algorithms and Architectures> [1]:

"Reusing parameter sets is bit rate efficient because it avoids the
necessity to send shared information multiple times. It is also loss
robust because it allows parameter set content to be carried by some
more reliable external communication link or to be repeated frequently
within the bitstream to ensure that it will not get lost."

> The way I understand it, people who design codec will decide if they use
> global extradata or not, but if they decide to, it is necessary to
> decode the data. Otherwise, it would not be there!

Indeed, the definition in spec is just the conformance, and how an encoder is
implemented in Libavcodec (and external library if any) is the thing really 
matters.

For encoders like libx264[2], libx265[3], libopenh264[4], if 
AV_CODEC_FLAG_GLOBAL_HEADER
flag is declared, the parameter Sets would be copied to extra data as the 
global header.
If not, parameter sets would be kept in the original bitstream, since it's 
fundamentally
supported in the encoder.
(BTW, librav1e seems to be identical, but I didn't check all the encoder and 
details for now)

And for encoders like libvpx-vp9, they don't implement the global header 
support, neither does
the libavformat(container) like ivf or webm.

(Please correct me if I missed anything or understood something wrongly, thx)

- Linjie

[1] https://link.springer.com/content/pdf/10.1007%2F978-3-319-06895-4.pdf
[2] https://github.com/FFmpeg/FFmpeg/blob/master/libavcodec/libx264.c#L924
[3] https://github.com/FFmpeg/FFmpeg/blob/master/libavcodec/libx265.c#L384
[4] 
https://github.com/FFmpeg/FFmpeg/blob/master/libavcodec/libopenh264enc.c#L338

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v2 4/5] fftools/ffmpeg: flush and recreate encoder instance if resolution changes

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of Fu,
> Linjie
> Sent: Friday, June 12, 2020 10:39
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v2 4/5] fftools/ffmpeg: flush and
> recreate encoder instance if resolution changes
> 
> > From: Nicolas George 
> > Sent: Friday, June 12, 2020 00:49
> > To: FFmpeg development discussions and patches  > de...@ffmpeg.org>
> > Cc: Fu, Linjie 
> > Subject: Re: [FFmpeg-devel] [PATCH v2 4/5] fftools/ffmpeg: flush and
> > recreate encoder instance if resolution changes
> >
> > Linjie Fu (12020-06-11):
> > > Add recreate_encoder_instance() function.
> > >
> > > If resolution changing is allowed, discard
> > AV_CODEC_FLAG_GLOBAL_HEADER
> > > even if the avformat/container declares AVFMT_GLOBALHEADER flag.
> > Place
> > > header information in every keyframe instead of single global header.
> >
> > Why? How is it valid? If the codec requires global header, we cannot
> > just ignore them.
> 
> IIRC, the global header in extra data is optional in codec level. By storing 
> the
> parameter set in global header, it allows reusing of the shared information
> and hence is bitrate efficient. Also it's loss robust since it allows 
> parameter
> set content to be carried in a more reliable way.(or repeated frequently)
> 
> Hence IMHO fallback to store the sequence header in key frame may lose
> the advantage of bitrate efficient and loss robust, but it seems to be correct
> since it's more fundamental. (And the test result shows it works)
> 
> (Please correct if my understanding is not correct)
> 
> > And if the codec can change resolution, there is no  point in recreating it.
> 
> Agree with this, for these codecs I'd prefer to implement corresponding
> method
> In specific encoder (like libvpx-vp9):
> https://patchwork.ffmpeg.org/project/ffmpeg/patch/1565688544-9043-1-
> git-send-email-linjie...@intel.com/
> 
Ping for this.

The patch set in series[1] would be the first step to settle down the whole 
solution,
which supports raw video only.

Then we'd focus on the common solutions(like recreating encoder instance) for
the encoders who declare such capabilities.

- Linjie

[1] https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=1434



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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 4/5] fftools/ffmpeg: flush and recreate encoder instance if resolution changes

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of
> Nicolas George
> Sent: Friday, June 12, 2020 01:52
> To: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 4/5] fftools/ffmpeg: flush and recreate
> encoder instance if resolution changes
> 
> Fu, Linjie (12020-06-11):
> > If rawvideo here means .h264, attached the output file produced by libx264:
> 
> Now, rawvideo means rawvideo.
> 
For Raw YUV video, it is not able to notify player/user about the resolution 
changing
since it only contains data information.

The usage for raw video is discussed previously. For now we'd like to use 
rawvideo encoder
To do some bit-exact compare to make sure the result of decoder is good without 
scaling. 
(matches the output of other media decoder component like gstreamer)

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v2 4/5] fftools/ffmpeg: flush and recreate encoder instance if resolution changes

[Fu, Linjie]

> From: Nicolas George 
> Sent: Friday, June 12, 2020 00:49
> To: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Cc: Fu, Linjie 
> Subject: Re: [FFmpeg-devel] [PATCH v2 4/5] fftools/ffmpeg: flush and
> recreate encoder instance if resolution changes
> 
> Linjie Fu (12020-06-11):
> > Add recreate_encoder_instance() function.
> >
> > If resolution changing is allowed, discard
> AV_CODEC_FLAG_GLOBAL_HEADER
> > even if the avformat/container declares AVFMT_GLOBALHEADER flag.
> Place
> > header information in every keyframe instead of single global header.
> 
> Why? How is it valid? If the codec requires global header, we cannot
> just ignore them. 

IIRC, the global header in extra data is optional in codec level. By storing the
parameter set in global header, it allows reusing of the shared information
and hence is bitrate efficient. Also it's loss robust since it allows parameter
set content to be carried in a more reliable way.(or repeated frequently)

Hence IMHO fallback to store the sequence header in key frame may lose
the advantage of bitrate efficient and loss robust, but it seems to be correct
since it's more fundamental. (And the test result shows it works)

(Please correct if my understanding is not correct)

> And if the codec can change resolution, there is no  point in recreating it.

Agree with this, for these codecs I'd prefer to implement corresponding method
In specific encoder (like libvpx-vp9):
https://patchwork.ffmpeg.org/project/ffmpeg/patch/1565688544-9043-1-git-send-email-linjie...@intel.com/

- Linjie


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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 4/5] fftools/ffmpeg: flush and recreate encoder instance if resolution changes

[Fu, Linjie]

> From: Nicolas George 
> Sent: Wednesday, June 10, 2020 19:54
> To: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Cc: Fu, Linjie 
> Subject: Re: [FFmpeg-devel] [PATCH 4/5] fftools/ffmpeg: flush and recreate
> encoder instance if resolution changes
> 
> Linjie Fu (12020-06-09):
> > Signed-off-by: Linjie Fu 
> > ---
> > Should be squashed with:
> > https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=1434
> 
> Apart from the issue of accessing non-public fields, this needs to be
> intensively tested. If it allows to create unplayable files, then it is
> not acceptable. It should be tested with codecs that use extradata (x264
> for example) and also with rawvideo.
> 
Indeed, tested with .mp4 and .h264 for encoder libx264, the results are 
playable.
(discarding global header if resolution changing is allowed, then we can keep 
the
Sequence header in each Key frame and make the resolution changing noticeable)

Please help to comment, thx.
https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=1470

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 4/5] fftools/ffmpeg: flush and recreate encoder instance if resolution changes

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of
> Anton Khirnov
> Sent: Wednesday, June 10, 2020 16:31
> To: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 4/5] fftools/ffmpeg: flush and recreate
> encoder instance if resolution changes
> 
> Quoting Linjie Fu (2020-06-09 10:48:46)
> > Signed-off-by: Linjie Fu 
> > ---
> > Should be squashed with:
> > https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=1434
> >
> >  fftools/ffmpeg.c | 11 +++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> > index 5859781..8cdd532 100644
> > --- a/fftools/ffmpeg.c
> > +++ b/fftools/ffmpeg.c
> > @@ -130,6 +130,7 @@ static void do_video_stats(OutputStream *ost, int
> frame_size);
> >  static BenchmarkTimeStamps get_benchmark_time_stamps(void);
> >  static int64_t getmaxrss(void);
> >  static int ifilter_has_all_input_formats(FilterGraph *fg);
> > +static void flush_encoders(void);
> >
> >  static int run_as_daemon  = 0;
> >  static int nb_frames_dup = 0;
> > @@ -1058,11 +1059,21 @@ static void do_video_out(OutputFile *of,
> >
> >  if (next_picture && (enc->width != next_picture->width ||
> >   enc->height != next_picture->height)) {
> > +flush_encoders();
> > +avcodec_flush_buffers(enc);
> >  if (!(enc->codec->capabilities &
> AV_CODEC_CAP_VARIABLE_DIMENSIONS)) {
> >  av_log(NULL, AV_LOG_ERROR, "Variable dimension encoding "
> >  "is not supported by %s.\n", enc->codec->name);
> >  goto error;
> >  }
> > +
> > +enc->width  = next_picture->width;
> > +enc->height = next_picture->height;
> > +
> > +if (enc->codec->close(enc) < 0)
> > +goto error;
> > +if (enc->codec->init(enc) < 0)
> > +goto error;
> 
> Absolutely not.
> Those are private fields, they must not be accessed by libavcodec
> callers.
> 
> What I meant was freeing the encoder and creating a completely new
> encoder instance.

Got it and updated, please help to comment, thx.
https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=1470

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 4/5] fftools/ffmpeg: flush and recreate encoder instance if resolution changes

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of
> James Almer
> Sent: Wednesday, June 10, 2020 12:22
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH 4/5] fftools/ffmpeg: flush and recreate
> encoder instance if resolution changes
> 
> On 6/9/2020 5:48 AM, Linjie Fu wrote:
> > Signed-off-by: Linjie Fu 
> > ---
> > Should be squashed with:
> > https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=1434
> >
> >  fftools/ffmpeg.c | 11 +++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> > index 5859781..8cdd532 100644
> > --- a/fftools/ffmpeg.c
> > +++ b/fftools/ffmpeg.c
> > @@ -130,6 +130,7 @@ static void do_video_stats(OutputStream *ost, int
> frame_size);
> >  static BenchmarkTimeStamps get_benchmark_time_stamps(void);
> >  static int64_t getmaxrss(void);
> >  static int ifilter_has_all_input_formats(FilterGraph *fg);
> > +static void flush_encoders(void);
> >
> >  static int run_as_daemon  = 0;
> >  static int nb_frames_dup = 0;
> > @@ -1058,11 +1059,21 @@ static void do_video_out(OutputFile *of,
> >
> >  if (next_picture && (enc->width != next_picture->width ||
> >   enc->height != next_picture->height)) {
> > +flush_encoders();
> > +avcodec_flush_buffers(enc);
> 
> This only works for a limited amount of encoders that set the
> AV_CODEC_CAP_ENCODER_FLUSH codec capability, and it's a no-op after
> emitting a warning otherwise.

Yes, hence would like to declare VARIABLE_DIMENSIONS and ENCODER_FLUSH
capability for encoders as well:
https://patchwork.ffmpeg.org/project/ffmpeg/patch/1591692568-19385-1-git-send-email-linjie...@intel.com/

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 4/5] fftools/ffmpeg: flush and recreate encoder instance if resolution changes

[Fu, Linjie]

> From: Devin Heitmueller 
> Sent: Tuesday, June 9, 2020 23:47
> To: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Cc: Fu, Linjie 
> Subject: Re: [FFmpeg-devel] [PATCH 4/5] fftools/ffmpeg: flush and recreate
> encoder instance if resolution changes
> 
> On Tue, Jun 9, 2020 at 4:53 AM Linjie Fu  wrote:
> >
> > Signed-off-by: Linjie Fu 
> > ---
> > Should be squashed with:
> > https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=1434
> >
> >  fftools/ffmpeg.c | 11 +++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> > index 5859781..8cdd532 100644
> > --- a/fftools/ffmpeg.c
> > +++ b/fftools/ffmpeg.c
> > @@ -130,6 +130,7 @@ static void do_video_stats(OutputStream *ost, int
> frame_size);
> >  static BenchmarkTimeStamps get_benchmark_time_stamps(void);
> >  static int64_t getmaxrss(void);
> >  static int ifilter_has_all_input_formats(FilterGraph *fg);
> > +static void flush_encoders(void);
> >
> >  static int run_as_daemon  = 0;
> >  static int nb_frames_dup = 0;
> > @@ -1058,11 +1059,21 @@ static void do_video_out(OutputFile *of,
> >
> >  if (next_picture && (enc->width != next_picture->width ||
> >   enc->height != next_picture->height)) {
> > +flush_encoders();
> > +avcodec_flush_buffers(enc);
> >  if (!(enc->codec->capabilities &
> AV_CODEC_CAP_VARIABLE_DIMENSIONS)) {
> >  av_log(NULL, AV_LOG_ERROR, "Variable dimension encoding "
> >  "is not supported by %s.\n", enc->codec->name);
> >  goto error;
> >  }
> > +
> > +enc->width  = next_picture->width;
> > +enc->height = next_picture->height;
> 
> Perhaps from a workflow standpoint it makes more sense to move the
> code which changes the encoder parameters to after where you close the
> existing encoder (i.e. between the close and init calls).  I can't
> think of a specific case where this might break a downstream encoder,
That's the reason I simply set the width/height ahead.
> but it seems like a good practice to only have the parameters applied
> to the new encoder instance.
Indeed, fixed locally.

> > +
> > +if (enc->codec->close(enc) < 0)
> > +goto error;
> > +if (enc->codec->init(enc) < 0)
> > +goto error;
> >  }
> >
> >  if (ost->source_index >= 0)
> 
> In general do we really think this is a safe thing to do?  Does
> something also need to be propagated to the output as well?  I know
> that this would break use cases like the decklink output where the
> frame resolution suddenly changes in the middle of the stream without
> calling the output's write_header() routine.

Yes, noticed the constraints in sequence header and container.

Since resolution changing is allowed in single bitstream, one global header is 
not
enough anymore as Nicolas has pointed out in [1].

Hence as to the container level,  for the formats with AVFMT_GLOBALHEADER flag,
we should place sps/pps information in key frame instead of in extradata.
(e.g. disable AV_CODEC_FLAG_GLOBAL_HEADER)

-if (oc->oformat->flags & AVFMT_GLOBALHEADER)
+if (oc->oformat->flags & AVFMT_GLOBALHEADER && ost->autoscale)
 ost->enc_ctx->flags |= AV_CODEC_FLAG_GLOBAL_HEADER; 

Verified this works, at least for containers like mp4.

- Linjie

[1] 
https://patchwork.ffmpeg.org/project/ffmpeg/patch/1591606685-4450-1-git-send-email-linjie...@intel.com/#55293

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/3] lavc/avcodec: Add caps for the support of variable dimension encoding

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of
> Linjie Fu
> Sent: Monday, June 8, 2020 23:11
> To: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 1/3] lavc/avcodec: Add caps for the
> support of variable dimension encoding
> 
> > From: "Anton Khirnov" 
> > Sent Time: 2020-06-08 18:50:43 (Monday)
> > To: "FFmpeg development discussions and patches"  de...@ffmpeg.org>
> > Cc:
> > Subject: Re: [FFmpeg-devel] [PATCH 1/3] lavc/avcodec: Add caps for the
> support   of variable dimension encoding
> >
> > Quoting Linjie Fu (2020-06-08 10:58:03)
> > > And declare AV_CODEC_CAP_VARIABLE_DIMENSIONS for rawvideo and
> > > wrapped_avframe.
> > >
> > > Signed-off-by: Linjie Fu 
> > > ---
> > >  doc/APIchanges   | 3 +++
> > >  fftools/cmdutils.c   | 2 ++
> > >  libavcodec/avcodec.h | 4 +++-
> > >  libavcodec/codec.h   | 5 +
> > >  libavcodec/rawenc.c  | 1 +
> > >  libavcodec/version.h | 2 +-
> > >  libavcodec/wrapped_avframe.c | 1 +
> > >  7 files changed, 16 insertions(+), 2 deletions(-)
> >
> > During the last iteration, I asked how is this preferable to just making
> > a new encoder instance. Don't think I got a sufficient reply.
> 
> Thanks Anton for the remind, indeed making a new encoder instance would
> be
> more general and suitable for all encoders, with resolution changing in key
> frames.
> 
> In this patch set, I prefer to restrict the implementation/support to
> rawvideo and wrapped_avframe encoders, since they don't need to be
> recreated
> when resolution/dimension changes and are required. at this moment.
> 
> Also as we've discussed about whether it's worthwhile:
> 
> >> Do we get sufficient benefits from having parameter change capability
> inside
> >> encoders over just creating a new encoder instance when needed? Do
> people
> >> really need to change resolution mid-GOP?
> 
> This implementation didn't touch the logic of encoders which supports
> variable
> resolution encoding on inter frames(vp9, av1), since such
> enhancement/requirement
> seems to be rare for now.

Sent the encoder instance recreate patch set for review as a follow-up step, 
please
help to review:
https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=1450

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] fftools/ffmpeg_filter: add -autoscale to disable/enable the default scale

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of Fu,
> Linjie
> Sent: Sunday, June 7, 2020 12:58
> To: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH] fftools/ffmpeg_filter: add -autoscale
> to disable/enable the default scale
> 
> > From: ffmpeg-devel  On Behalf Of
> > Eoff, Ullysses A
> > Sent: Saturday, June 6, 2020 00:16
> > To: FFmpeg development discussions and patches  > de...@ffmpeg.org>
> > Subject: Re: [FFmpeg-devel] [PATCH] fftools/ffmpeg_filter: add -autoscale
> > to disable/enable the default scale
> >
> > > -Original Message-
> > > From: ffmpeg-devel  On Behalf Of
> > Fu, Linjie
> > > Sent: Friday, March 13, 2020 8:18 PM
> > > To: FFmpeg development discussions and patches  > de...@ffmpeg.org>
> > > Subject: Re: [FFmpeg-devel] [PATCH] fftools/ffmpeg_filter: add -
> autoscale
> > to disable/enable the default scale
> > >
> > > > From: ffmpeg-devel  On Behalf
> Of
> > > > Nicolas George
> > > > Sent: Tuesday, February 18, 2020 03:02
> > > > To: FFmpeg development discussions and patches  > > > de...@ffmpeg.org>
> > > > Subject: Re: [FFmpeg-devel] [PATCH] fftools/ffmpeg_filter: add -
> > autoscale
> > > > to disable/enable the default scale
> > > >
> > > > Gyan Doshi (12020-02-18):
> > > > > Protection in the command line tool will help steer the user towards
> > > > > choosing an accommodating encoder, which is possible once the flag
> > and a
> > > > > check is added.
> > > >
> > > > True, but minor and should not block the patch.
> > > >
> > >
> > > Ping, thanks.
> > >
> > > - Linjie
> >
> > Are all of the reviews addressed for this patch?  Is it ready to be merged,
> yet?
> >
> 
> Yes, and there are some discussions[1] previously about disabling autoscale,
> since
> this may lead to undefined behavior in the pipeline.
> 
> And a possible solution is to define a capability[2] and add the capability
> check[3] to
> restrict the usage and prevent the unexpected condition.
> 
> Please help to comment.
> 
> This choice is optional for user and many users are requesting for this.
> Hence if no objections, I'd like to have this set step further.
> 
> [1] https://patchwork.ffmpeg.org/project/ffmpeg/patch/1564461924-4772-
> 1-git-send-email-linjie...@intel.com/
> [2] https://patchwork.ffmpeg.org/project/ffmpeg/patch/1582820025-22302-
> 1-git-send-email-linjie...@intel.com/
> [3] https://patchwork.ffmpeg.org/project/ffmpeg/patch/1582820043-22426-
> 1-git-send-email-linjie...@intel.com/

Rebased and updated the patch set, prefer to apply soon if no objections.
https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=1434

Regards,
Linjie
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] fftools/ffmpeg_filter: add -autoscale to disable/enable the default scale

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of
> Eoff, Ullysses A
> Sent: Saturday, June 6, 2020 00:16
> To: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH] fftools/ffmpeg_filter: add -autoscale
> to disable/enable the default scale
> 
> > -Original Message-
> > From: ffmpeg-devel  On Behalf Of
> Fu, Linjie
> > Sent: Friday, March 13, 2020 8:18 PM
> > To: FFmpeg development discussions and patches  de...@ffmpeg.org>
> > Subject: Re: [FFmpeg-devel] [PATCH] fftools/ffmpeg_filter: add -autoscale
> to disable/enable the default scale
> >
> > > From: ffmpeg-devel  On Behalf Of
> > > Nicolas George
> > > Sent: Tuesday, February 18, 2020 03:02
> > > To: FFmpeg development discussions and patches  > > de...@ffmpeg.org>
> > > Subject: Re: [FFmpeg-devel] [PATCH] fftools/ffmpeg_filter: add -
> autoscale
> > > to disable/enable the default scale
> > >
> > > Gyan Doshi (12020-02-18):
> > > > Protection in the command line tool will help steer the user towards
> > > > choosing an accommodating encoder, which is possible once the flag
> and a
> > > > check is added.
> > >
> > > True, but minor and should not block the patch.
> > >
> >
> > Ping, thanks.
> >
> > - Linjie
> 
> Are all of the reviews addressed for this patch?  Is it ready to be merged, 
> yet?
> 

Yes, and there are some discussions[1] previously about disabling autoscale, 
since
this may lead to undefined behavior in the pipeline.

And a possible solution is to define a capability[2] and add the capability 
check[3] to
restrict the usage and prevent the unexpected condition.


Please help to comment.

This choice is optional for user and many users are requesting for this.
Hence if no objections, I'd like to have this set step further.

Regards,
Linjie

[1] 
https://patchwork.ffmpeg.org/project/ffmpeg/patch/1564461924-4772-1-git-send-email-linjie...@intel.com/
[2] 
https://patchwork.ffmpeg.org/project/ffmpeg/patch/1582820025-22302-1-git-send-email-linjie...@intel.com/
[3] 
https://patchwork.ffmpeg.org/project/ffmpeg/patch/1582820043-22426-1-git-send-email-linjie...@intel.com/


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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] lavc/vaapi_encode_h265: add low_delay_b option for HEVC

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of Fu,
> Linjie
> Sent: Tuesday, April 14, 2020 17:15
> To: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH] lavc/vaapi_encode_h265: add
> low_delay_b option for HEVC
> 
> > From: ffmpeg-devel  On Behalf Of
> > Mark Thompson
> > Sent: Monday, April 13, 2020 20:20
> > To: ffmpeg-devel@ffmpeg.org
> > Subject: Re: [FFmpeg-devel] [PATCH] lavc/vaapi_encode_h265: add
> > low_delay_b option for HEVC
> >
> > On 13/04/2020 05:32, Linjie Fu wrote:
> > > Low delay B-frame is supported on ICL+ platform.
> > >
> > > For low power encoding, low_delay_b should be enabled by default.
> > >
> > > Low delay B:
> > > <http://what-when-how.com/Tutorial/topic-397pct9eq3/High-Efficiency-
> > Video-Coding-HEVC-288.html>
> > >
> > > There is an on-going work in libva and media-driver to add querys
> > > support for low delay b, would add it once it's ready:
> > > https://github.com/intel/libva/pull/220
> > > https://github.com/intel/libva/pull/364
> > > https://github.com/intel/media-driver/issues/721
> > >
> > > Signed-off-by: Linjie Fu 
> > > ---
> > >  doc/encoders.texi  |  8 
> > >  libavcodec/vaapi_encode_h265.c | 19 +--
> > >  2 files changed, 25 insertions(+), 2 deletions(-)
> >
> > My understanding was that the only reason for this stuff existing was to
> work
> > around broken hardware which didn't support P frames.  Is this no longer
> > true?
> >
> > >
> > > diff --git a/doc/encoders.texi b/doc/encoders.texi
> > > index e23b6b3..b0812be 100644
> > > --- a/doc/encoders.texi
> > > +++ b/doc/encoders.texi
> > > @@ -3089,6 +3089,14 @@ Some combination of the following values:
> > >  Include HDR metadata if the input frames have it
> > >  (@emph{mastering_display_colour_volume} and
> > @emph{content_light_level}
> > >  messages).
> > > +
> > > +@item low_delay_b
> > > +Use low delay B-frames instead of P frames. Reordering of pictures is
> > > +not allowed. The first picture is encoded as an I picture and subsequent
> > > +pictures are encoded as B pictures. Moreover, since past B pictures are
> > > +used for prediction, a low coding delay but with higher coding efficiency
> > > +(because of bi-prediction) is achieved.
> >
> > This sounds like a marketing blurb.
> >
> > Not for the manual, but can you explain in detail what might actually make
> > this better, with actual numbers if possible?  Intuitively the coding
> efficiency
> > will be worse, because a number of the B-picture syntax elements are just
> > redundant but still have to be coded (picking between two options which
> are
> > actually identical).  The gain of allowing biprediction between two 
> > identical
> > pictures doesn't seem like a useful feature.
> >
> The story is, this is kind of the hardware limitation for VDENC as we've
> discussed
> on IRC.

Since the dependency in LIBVA and media-driver is addressed,  updated and resent
the patch.

> As to the performance/efficiency, I'm curious too and would take some
> experiments.
Did some quick experiments on ICL with IBBPBBP reference structure (not 
official data,
just to show the possible benefits):

Bdrate : -1.570% based on normal IPB structure if we convert P frames to B 
frames.

biprediction between two identical pictures seem to benefit the quality 
slightly.

Please help to comment:
https://patchwork.ffmpeg.org/project/ffmpeg/patch/1591019384-21910-1-git-send-email-linjie...@intel.com/

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] hevc_refs: reduce code duplication in find_ref_idx()

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of
> Anton Khirnov
> Sent: Wednesday, May 27, 2020 16:48
> To: ffmpeg-devel@ffmpeg.org
> Subject: [FFmpeg-devel] [PATCH] hevc_refs: reduce code duplication in
> find_ref_idx()
> 
> ---
>  libavcodec/hevc_refs.c | 22 ++
>  1 file changed, 6 insertions(+), 16 deletions(-)
> 
> diff --git a/libavcodec/hevc_refs.c b/libavcodec/hevc_refs.c
> index 73aa6d8caf..4f6d985ae6 100644
> --- a/libavcodec/hevc_refs.c
> +++ b/libavcodec/hevc_refs.c
> @@ -360,24 +360,14 @@ int ff_hevc_slice_rpl(HEVCContext *s)
> 
>  static HEVCFrame *find_ref_idx(HEVCContext *s, int poc, uint8_t use_msb)
>  {
> +int mask = use_msb ? ~0 : (1 << s->ps.sps->log2_max_poc_lsb) - 1;
>  int i;
> 
> -if (use_msb) {
> -for (i = 0; i < FF_ARRAY_ELEMS(s->DPB); i++) {
> -HEVCFrame *ref = >DPB[i];
> -if (ref->frame->buf[0] && (ref->sequence == s->seq_decode)) {
> -if (ref->poc == poc)
> -return ref;
> -}
> -}
> -} else {
> -int LtMask = (1 << s->ps.sps->log2_max_poc_lsb) - 1;
> -for (i = 0; i < FF_ARRAY_ELEMS(s->DPB); i++) {
> -HEVCFrame *ref = >DPB[i];
> -if (ref->frame->buf[0] && ref->sequence == s->seq_decode) {
> -if ((ref->poc & LtMask) == poc)
> -return ref;
> -}
> +for (i = 0; i < FF_ARRAY_ELEMS(s->DPB); i++) {
> +HEVCFrame *ref = >DPB[i];
> +if (ref->frame->buf[0] && ref->sequence == s->seq_decode) {
> +if ((ref->poc & mask) == poc)
> +return ref;
>  }
>  }
> 
> --
Thanks for the refine, it's more precise now, lgtm.

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] aubmit Verisilicon VPE hardware codec implementation

[Fu, Linjie]

Squash new pixel format into the ENUM structure would lead to an ABI break.
Would be better to append at the end. (Before AV_PIX_FMT_NB)

-Linjie

From: ffmpeg-devel  On Behalf Of Zhang, Guiyong
Sent: Thursday, May 28, 2020 10:12
To: ffmpeg-devel@ffmpeg.org
Subject: [FFmpeg-devel] aubmit Verisilicon VPE hardware codec implementation
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] lavc/vaapi_hevc: add missing max_8bit_constraint_flag

[Fu, Linjie]

> From: Fu, Linjie 
> Sent: Tuesday, May 12, 2020 21:47
> To: ffmpeg-devel@ffmpeg.org
> Cc: Fu, Linjie 
> Subject: [PATCH] lavc/vaapi_hevc: add missing max_8bit_constraint_flag
> 
> This is accidentally missed while rebasing.
> 
> Signed-off-by: Linjie Fu 
> ---
>  libavcodec/vaapi_hevc.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/libavcodec/vaapi_hevc.c b/libavcodec/vaapi_hevc.c
> index c83d481..9083331 100644
> --- a/libavcodec/vaapi_hevc.c
> +++ b/libavcodec/vaapi_hevc.c
> @@ -505,6 +505,7 @@ static int ptl_convert(const PTLCommon *general_ptl,
> H265RawProfileTierLevel *h2
>  copy_field(frame_only_constraint_flag);
>  copy_field(max_12bit_constraint_flag);
>  copy_field(max_10bit_constraint_flag);
> +copy_field(max_8bit_constraint_flag);
>  copy_field(max_422chroma_constraint_flag);
>  copy_field(max_420chroma_constraint_flag);
>  copy_field(max_monochrome_constraint_flag);
> --
> 2.7.4
Applied, thx.

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 2/2] lavc/hevc_refs: Fix the logic of find_ref_idx()

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of Fu,
> Linjie
> Sent: Thursday, May 21, 2020 14:38
> To: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Cc: Xu, Guangxin 
> Subject: Re: [FFmpeg-devel] [PATCH 2/2] lavc/hevc_refs: Fix the logic of
> find_ref_idx()
> 
> > From: ffmpeg-devel  On Behalf Of
> Fu,
> > Linjie
> > Sent: Monday, May 18, 2020 15:17
> > To: ffmpeg-devel@ffmpeg.org
> > Cc: Xu, Guangxin 
> > Subject: Re: [FFmpeg-devel] [PATCH 2/2] lavc/hevc_refs: Fix the logic of
> > find_ref_idx()
> >
> > > From: Fu, Linjie 
> > > Sent: Tuesday, May 12, 2020 21:44
> > > To: ffmpeg-devel@ffmpeg.org
> > > Cc: Xu, Guangxin ; Fu, Linjie
> 
> > > Subject: [PATCH 2/2] lavc/hevc_refs: Fix the logic of find_ref_idx()
> > >
> > > From: Xu Guangxin 
> > >
> > > Currently find_ref_idx() would trigger 2 scans in DPB to find the
> > > requested POC:
> > > 1. Firstly, ignore MSB of ref->poc and search for the requested POC;
> > > 2. Secondly, compare the entire ref->poc with requested POC;
> > >
> > > For long term reference, we are able to only check LSB if MSB is not
> > > presented(e.g. delta_poc_msb_present_flag == 0). However, for short
> > > term reference, we should never ignore poc's MSB and it should be
> > > kind of bit-exact. (Details in 8.3.2)
> > >
> > > Otherwise this leads to decoding failures like:
> > > [hevc @ 0x5638f4328600] Error constructing the frame RPS.
> > > [hevc @ 0x5638f4328600] Error parsing NAL unit #2.
> > > [hevc @ 0x5638f4338a80] Could not find ref with POC 21
> > > Error while decoding stream #0:0: Invalid data found when processing
> input
> > >
> > > Search the requested POC based on whether MSB is used, and avoid
> > > the 2-times scan for DPB buffer. This benefits both native HEVC
> > > decoder and integrated HW decoders.
> > >
> > > Signed-off-by: Linjie Fu 
> > > ---
> > >
> > > Since it's kind of difficult to generate an identical bitstream for
> > > fate or test, I'd like to elaborate more about one of the failures:
> > >
> > > requested POC = 5;
> > > LtMask = (1 << 4) - 1 = 15;
> > > ref[0]->poc = 21; // unexpected ref for poc = 5 (short term)
> > > ref[1]->poc = 5;  // expected ref for poc = 5 (short term)
> > >
> > > Hence find_ref_idx() would wrongly return a ref with poc = 21, which
> > > leads to the decoding error.
> > >
> > >  libavcodec/hevc_refs.c | 38 --
> > >  1 file changed, 20 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/libavcodec/hevc_refs.c b/libavcodec/hevc_refs.c
> > > index 7870a72..73aa6d8 100644
> > > --- a/libavcodec/hevc_refs.c
> > > +++ b/libavcodec/hevc_refs.c
> > > @@ -358,24 +358,26 @@ int ff_hevc_slice_rpl(HEVCContext *s)
> > >  return 0;
> > >  }
> > >
> > > -static HEVCFrame *find_ref_idx(HEVCContext *s, int poc)
> > > +static HEVCFrame *find_ref_idx(HEVCContext *s, int poc, uint8_t
> > use_msb)
> > >  {
> > >  int i;
> > > -int LtMask = (1 << s->ps.sps->log2_max_poc_lsb) - 1;
> > >
> > > -for (i = 0; i < FF_ARRAY_ELEMS(s->DPB); i++) {
> > > -HEVCFrame *ref = >DPB[i];
> > > -if (ref->frame->buf[0] && (ref->sequence == s->seq_decode)) {
> > > -if ((ref->poc & LtMask) == poc)
> > > -return ref;
> > > +if (use_msb) {
> > > +for (i = 0; i < FF_ARRAY_ELEMS(s->DPB); i++) {
> > > +HEVCFrame *ref = >DPB[i];
> > > +if (ref->frame->buf[0] && (ref->sequence == s->seq_decode)) {
> > > +if (ref->poc == poc)
> > > +return ref;
> > > +}
> > >  }
> > > -}
> > > -
> > > -for (i = 0; i < FF_ARRAY_ELEMS(s->DPB); i++) {
> > > -HEVCFrame *ref = >DPB[i];
> > > -if (ref->frame->buf[0] && ref->sequence == s->seq_decode) {
> > > -if (ref->poc == poc || (ref->poc & LtMask) == poc)
> > > -return ref;
> > > +} else {
> > > +int LtMask = (1 << s->ps.sps->log2_max_poc_lsb) - 1;
> > > +for (i = 0; i < FF_ARRAY_ELEMS(s->

Re: [FFmpeg-devel] [PATCH] avcodec/encode: restructure the core encoding code

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of
> James Almer
> Sent: Tuesday, May 26, 2020 22:36
> To: ffmpeg-devel@ffmpeg.org
> Subject: [FFmpeg-devel] [PATCH] avcodec/encode: restructure the core
> encoding code
> 
> This commit follows the same logic as 061a0c14bb, but for the encode API:
> The
> new public encoding API will no longer be a wrapper around the old
> deprecated
> one, and the internal API used by the encoders now consists of a single
> receive_packet() callback that pulls frames as required.
> 
> amf encoders adapted by James Almer
> librav1e encoder adapted by James Almer
> nvidia encoders adapted by James Almer
> MediaFoundation encoders adapted by James Almer
> vaapi encoders adapted by Linjie Fu
> v4l2_m2m encoders adapted by Andriy Gelman
> 
> Signed-off-by: James Almer 
> ---
> 
> diff --git a/libavcodec/vaapi_encode.c b/libavcodec/vaapi_encode.c
> index cb05ebd774..98026f451a 100644
> --- a/libavcodec/vaapi_encode.c
> +++ b/libavcodec/vaapi_encode.c
> @@ -25,6 +25,7 @@
>  #include "libavutil/pixdesc.h"
> 
>  #include "vaapi_encode.h"
> +#include "encode.h"
>  #include "avcodec.h"
> 
>  const AVCodecHWConfigInternal *ff_vaapi_encode_hw_configs[] = {
> @@ -1043,7 +1044,7 @@ static int
> vaapi_encode_check_frame(AVCodecContext *avctx,
>  return 0;
>  }
> 
> -int ff_vaapi_encode_send_frame(AVCodecContext *avctx, const AVFrame
> *frame)
> +static int vaapi_encode_send_frame(AVCodecContext *avctx, AVFrame
> *frame)
>  {
>  VAAPIEncodeContext *ctx = avctx->priv_data;
>  VAAPIEncodePicture *pic;
> @@ -1066,9 +1067,7 @@ int
> ff_vaapi_encode_send_frame(AVCodecContext *avctx, const AVFrame
> *frame)
>  err = AVERROR(ENOMEM);
>  goto fail;
>  }
> -err = av_frame_ref(pic->input_image, frame);
> -if (err < 0)
> -goto fail;
> +av_frame_move_ref(pic->input_image, frame);

Slight change for the vaapi part is needed here:

av_frame_move_ref() would reset src(frame), hence we need to get
all attributes passed from frame to pic before we call av_frame_move_ref().

Didn't notice this until encoding quality drop is observed in pre-check[1] for
this patch set while verifying. (and sorry for didn't catch this earlier)

After adding this fix, all pre-check is good. 

- Linjie

[1] https://github.com/intel-media-ci/ffmpeg/pull/206

Details: 
diff --git a/libavcodec/vaapi_encode.c b/libavcodec/vaapi_encode.c
index 98026f451a..e39db20200 100644
--- a/libavcodec/vaapi_encode.c
+++ b/libavcodec/vaapi_encode.c
@@ -1067,7 +1067,6 @@ static int vaapi_encode_send_frame(AVCodecContext *avctx, 
AVFrame *frame)
 err = AVERROR(ENOMEM);
 goto fail;
 }
-av_frame_move_ref(pic->input_image, frame);

 if (ctx->input_order == 0 || frame->pict_type == AV_PICTURE_TYPE_I)
 pic->force_idr = 1;
@@ -1075,6 +1074,8 @@ static int vaapi_encode_send_frame(AVCodecContext *avctx, 
AVFrame *frame)
 pic->input_surface = (VASurfaceID)(uintptr_t)frame->data[3];
 pic->pts = frame->pts;

+av_frame_move_ref(pic->input_image, frame);
+
 if (ctx->input_order == 0)
 ctx->first_pts = pic->pts;
 if (ctx->input_order == ctx->decode_delay)
--


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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 2/2] lavc/hevc_refs: Fix the logic of find_ref_idx()

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of Fu,
> Linjie
> Sent: Monday, May 18, 2020 15:17
> To: ffmpeg-devel@ffmpeg.org
> Cc: Xu, Guangxin 
> Subject: Re: [FFmpeg-devel] [PATCH 2/2] lavc/hevc_refs: Fix the logic of
> find_ref_idx()
> 
> > From: Fu, Linjie 
> > Sent: Tuesday, May 12, 2020 21:44
> > To: ffmpeg-devel@ffmpeg.org
> > Cc: Xu, Guangxin ; Fu, Linjie 
> > Subject: [PATCH 2/2] lavc/hevc_refs: Fix the logic of find_ref_idx()
> >
> > From: Xu Guangxin 
> >
> > Currently find_ref_idx() would trigger 2 scans in DPB to find the
> > requested POC:
> > 1. Firstly, ignore MSB of ref->poc and search for the requested POC;
> > 2. Secondly, compare the entire ref->poc with requested POC;
> >
> > For long term reference, we are able to only check LSB if MSB is not
> > presented(e.g. delta_poc_msb_present_flag == 0). However, for short
> > term reference, we should never ignore poc's MSB and it should be
> > kind of bit-exact. (Details in 8.3.2)
> >
> > Otherwise this leads to decoding failures like:
> > [hevc @ 0x5638f4328600] Error constructing the frame RPS.
> > [hevc @ 0x5638f4328600] Error parsing NAL unit #2.
> > [hevc @ 0x5638f4338a80] Could not find ref with POC 21
> > Error while decoding stream #0:0: Invalid data found when processing input
> >
> > Search the requested POC based on whether MSB is used, and avoid
> > the 2-times scan for DPB buffer. This benefits both native HEVC
> > decoder and integrated HW decoders.
> >
> > Signed-off-by: Linjie Fu 
> > ---
> >
> > Since it's kind of difficult to generate an identical bitstream for
> > fate or test, I'd like to elaborate more about one of the failures:
> >
> > requested POC = 5;
> > LtMask = (1 << 4) - 1 = 15;
> > ref[0]->poc = 21; // unexpected ref for poc = 5 (short term)
> > ref[1]->poc = 5;  // expected ref for poc = 5 (short term)
> >
> > Hence find_ref_idx() would wrongly return a ref with poc = 21, which
> > leads to the decoding error.
> >
> >  libavcodec/hevc_refs.c | 38 --
> >  1 file changed, 20 insertions(+), 18 deletions(-)
> >
> > diff --git a/libavcodec/hevc_refs.c b/libavcodec/hevc_refs.c
> > index 7870a72..73aa6d8 100644
> > --- a/libavcodec/hevc_refs.c
> > +++ b/libavcodec/hevc_refs.c
> > @@ -358,24 +358,26 @@ int ff_hevc_slice_rpl(HEVCContext *s)
> >  return 0;
> >  }
> >
> > -static HEVCFrame *find_ref_idx(HEVCContext *s, int poc)
> > +static HEVCFrame *find_ref_idx(HEVCContext *s, int poc, uint8_t
> use_msb)
> >  {
> >  int i;
> > -int LtMask = (1 << s->ps.sps->log2_max_poc_lsb) - 1;
> >
> > -for (i = 0; i < FF_ARRAY_ELEMS(s->DPB); i++) {
> > -HEVCFrame *ref = >DPB[i];
> > -if (ref->frame->buf[0] && (ref->sequence == s->seq_decode)) {
> > -if ((ref->poc & LtMask) == poc)
> > -return ref;
> > +if (use_msb) {
> > +for (i = 0; i < FF_ARRAY_ELEMS(s->DPB); i++) {
> > +HEVCFrame *ref = >DPB[i];
> > +if (ref->frame->buf[0] && (ref->sequence == s->seq_decode)) {
> > +if (ref->poc == poc)
> > +return ref;
> > +}
> >  }
> > -}
> > -
> > -for (i = 0; i < FF_ARRAY_ELEMS(s->DPB); i++) {
> > -HEVCFrame *ref = >DPB[i];
> > -if (ref->frame->buf[0] && ref->sequence == s->seq_decode) {
> > -if (ref->poc == poc || (ref->poc & LtMask) == poc)
> > -return ref;
> > +} else {
> > +int LtMask = (1 << s->ps.sps->log2_max_poc_lsb) - 1;
> > +for (i = 0; i < FF_ARRAY_ELEMS(s->DPB); i++) {
> > +HEVCFrame *ref = >DPB[i];
> > +if (ref->frame->buf[0] && ref->sequence == s->seq_decode) {
> > +if ((ref->poc & LtMask) == poc)
> > +return ref;
> > +}
> >  }
> >  }
> >
> > @@ -427,9 +429,9 @@ static HEVCFrame
> > *generate_missing_ref(HEVCContext *s, int poc)
> >
> >  /* add a reference with the given poc to the list and mark it as used in 
> > DPB
> > */
> >  static int add_candidate_ref(HEVCContext *s, RefPicList *list,
> > - int poc, int ref_flag)
> > +

Re: [FFmpeg-devel] [PATCH 2/2] lavc/hevc_refs: Fix the logic of find_ref_idx()

[Fu, Linjie]

> From: Fu, Linjie 
> Sent: Tuesday, May 12, 2020 21:44
> To: ffmpeg-devel@ffmpeg.org
> Cc: Xu, Guangxin ; Fu, Linjie 
> Subject: [PATCH 2/2] lavc/hevc_refs: Fix the logic of find_ref_idx()
> 
> From: Xu Guangxin 
> 
> Currently find_ref_idx() would trigger 2 scans in DPB to find the
> requested POC:
> 1. Firstly, ignore MSB of ref->poc and search for the requested POC;
> 2. Secondly, compare the entire ref->poc with requested POC;
> 
> For long term reference, we are able to only check LSB if MSB is not
> presented(e.g. delta_poc_msb_present_flag == 0). However, for short
> term reference, we should never ignore poc's MSB and it should be
> kind of bit-exact. (Details in 8.3.2)
> 
> Otherwise this leads to decoding failures like:
> [hevc @ 0x5638f4328600] Error constructing the frame RPS.
> [hevc @ 0x5638f4328600] Error parsing NAL unit #2.
> [hevc @ 0x5638f4338a80] Could not find ref with POC 21
> Error while decoding stream #0:0: Invalid data found when processing input
> 
> Search the requested POC based on whether MSB is used, and avoid
> the 2-times scan for DPB buffer. This benefits both native HEVC
> decoder and integrated HW decoders.
> 
> Signed-off-by: Linjie Fu 
> ---
> 
> Since it's kind of difficult to generate an identical bitstream for
> fate or test, I'd like to elaborate more about one of the failures:
> 
> requested POC = 5;
> LtMask = (1 << 4) - 1 = 15;
> ref[0]->poc = 21; // unexpected ref for poc = 5 (short term)
> ref[1]->poc = 5;  // expected ref for poc = 5 (short term)
> 
> Hence find_ref_idx() would wrongly return a ref with poc = 21, which
> leads to the decoding error.
> 
>  libavcodec/hevc_refs.c | 38 --
>  1 file changed, 20 insertions(+), 18 deletions(-)
> 
> diff --git a/libavcodec/hevc_refs.c b/libavcodec/hevc_refs.c
> index 7870a72..73aa6d8 100644
> --- a/libavcodec/hevc_refs.c
> +++ b/libavcodec/hevc_refs.c
> @@ -358,24 +358,26 @@ int ff_hevc_slice_rpl(HEVCContext *s)
>  return 0;
>  }
> 
> -static HEVCFrame *find_ref_idx(HEVCContext *s, int poc)
> +static HEVCFrame *find_ref_idx(HEVCContext *s, int poc, uint8_t use_msb)
>  {
>  int i;
> -int LtMask = (1 << s->ps.sps->log2_max_poc_lsb) - 1;
> 
> -for (i = 0; i < FF_ARRAY_ELEMS(s->DPB); i++) {
> -HEVCFrame *ref = >DPB[i];
> -if (ref->frame->buf[0] && (ref->sequence == s->seq_decode)) {
> -if ((ref->poc & LtMask) == poc)
> -return ref;
> +if (use_msb) {
> +for (i = 0; i < FF_ARRAY_ELEMS(s->DPB); i++) {
> +HEVCFrame *ref = >DPB[i];
> +if (ref->frame->buf[0] && (ref->sequence == s->seq_decode)) {
> +if (ref->poc == poc)
> +return ref;
> +}
>  }
> -}
> -
> -for (i = 0; i < FF_ARRAY_ELEMS(s->DPB); i++) {
> -HEVCFrame *ref = >DPB[i];
> -if (ref->frame->buf[0] && ref->sequence == s->seq_decode) {
> -if (ref->poc == poc || (ref->poc & LtMask) == poc)
> -return ref;
> +} else {
> +int LtMask = (1 << s->ps.sps->log2_max_poc_lsb) - 1;
> +for (i = 0; i < FF_ARRAY_ELEMS(s->DPB); i++) {
> +HEVCFrame *ref = >DPB[i];
> +if (ref->frame->buf[0] && ref->sequence == s->seq_decode) {
> +if ((ref->poc & LtMask) == poc)
> +return ref;
> +}
>  }
>  }
> 
> @@ -427,9 +429,9 @@ static HEVCFrame
> *generate_missing_ref(HEVCContext *s, int poc)
> 
>  /* add a reference with the given poc to the list and mark it as used in DPB
> */
>  static int add_candidate_ref(HEVCContext *s, RefPicList *list,
> - int poc, int ref_flag)
> + int poc, int ref_flag, uint8_t use_msb)
>  {
> -HEVCFrame *ref = find_ref_idx(s, poc);
> +HEVCFrame *ref = find_ref_idx(s, poc, use_msb);
> 
>  if (ref == s->ref || list->nb_refs >= HEVC_MAX_REFS)
>  return AVERROR_INVALIDDATA;
> @@ -485,7 +487,7 @@ int ff_hevc_frame_rps(HEVCContext *s)
>  else
>  list = ST_CURR_AFT;
> 
> -ret = add_candidate_ref(s, [list], poc,
> HEVC_FRAME_FLAG_SHORT_REF);
> +ret = add_candidate_ref(s, [list], poc,
> HEVC_FRAME_FLAG_SHORT_REF, 1);
>  if (ret < 0)
>  goto fail;
>  }
> @@ -495,7 +497,7 @@ int ff_hevc_frame_rps(HEVCContext *s)
>  int poc  = long_rps->poc[i];
&g

Re: [FFmpeg-devel] [PATCH] lavc/vaapi_encode: Async the encoding and output procedure of encoder

[Fu, Linjie]

> Mark Thompson:
> Sent: Monday, November 18, 2019 07:14
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH] lavc/vaapi_encode: Async the
> encoding and output procedure of encoder
> 
> On 07/11/2019 16:32, Linjie Fu wrote:
> > Currently, vaapi encodes a pic if all its references are ready,
> > and then outputs it immediately by calling vaapi_encode_output.
> >
> > However, while working on output procedure, hardware is be able to
> > cope with encoding tasks in the meantime to have the better performance.
> >
> > So a more efficient way is to send all the pics with available refs to
> > hardware to allow encoding while output.
> >
> > It's what vaapi originally did before the regression, and the performance
> > could be improved for ~20%.
> >
> > CMD:
> > ffmpeg -hwaccel vaapi -vaapi_device /dev/dri/renderD128
> > -hwaccel_output_format vaapi -i
> bbb_sunflower_1080p_30fps_normal.mp4
> > -c:v h264_vaapi -f h264 -y /dev/null
> >
> > Source:
> > https://download.blender.org/demo/movies/BBB/
> >
> > Before:
> > ~164 fps
> > After:
> > ~198 fps
> >
> > Fix #7706.
> >
> > Signed-off-by: Linjie Fu 
> > ---
> 
> The sync in the same receive call here is required for correctness because of
> the how VAAPI syncs to the input surface (consider the interactions of a split
> followed by two encoders using the same surfaces).  I didn't realise that for 
> a
> long time, hence the error existing in earlier versions.

Indeed, you're right 1:N encoding procedure suffered from this. And the proper
fix is to provide a new API to sync the coded buffer which is independent for 
each
encoding task, instead of syncing the shared input surface. (See comments below)

> 
> Relatedly, this change would significantly increase latency on Intel platforms
> because vaEndPicture() is mostly synchronous there, and you're now calling
> it multiple times before returning anything.

As the the latency, how about adding an option like "-async 0/1" to specify the
user requirement:
For live streaming, user may use -async 0 to minimize the latency;
For video on demand, user may use -async 1 to maximize the performance.

> 
> More generally, though, the API definition here is just really stupid.  If 
> you're
> interested it would be much better to fix this in the API - sync-to-output of
> some kind would make far more sense, as would some kind of event-based
> system (extra points if it can interop via fds to normal poll() and Vulkan).
> 
We have a proposal[1] to libva to introduce the new function to make encoder
synchronization by output bitstream., and looking forward to your comments.

- Linjie

[1] 

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH, v2 1/2] lavc/qsvdec: add decode support for HEVC 4:2:2 8-bit and 10-bit

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of
> Max Dmitrichenko
> Sent: Friday, May 15, 2020 16:19
> To: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Cc: Zhong Li 
> Subject: Re: [FFmpeg-devel] [PATCH, v2 1/2] lavc/qsvdec: add decode
> support for HEVC 4:2:2 8-bit and 10-bit
> 
> On Fri, May 15, 2020 at 9:06 AM Fu, Linjie  wrote:
> 
> > > From: ffmpeg-devel  On Behalf Of
> > > Artem Galin
> > > Sent: Tuesday, April 28, 2020 00:26
> > > To: FFmpeg development discussions and patches  > > de...@ffmpeg.org>
> > > Cc: Zhong Li 
> > > Subject: Re: [FFmpeg-devel] [PATCH, v2 1/2] lavc/qsvdec: add decode
> > > support for HEVC 4:2:2 8-bit and 10-bit
> > >
> > > On Wed, 15 Apr 2020 at 05:02, Fu, Linjie  wrote:
> > >
> > > > > From: Zhong Li 
> > > > > Sent: Wednesday, April 15, 2020 09:58
> > > > > To: FFmpeg development discussions and patches  > > > > de...@ffmpeg.org>
> > > > > Cc: Fu, Linjie 
> > > > > Subject: Re: [FFmpeg-devel] [PATCH, v2 1/2] lavc/qsvdec: add decode
> > > > > support for HEVC 4:2:2 8-bit and 10-bit
> > > > >
> > > > > Linjie Fu  于2020年2月26日周三 下午4:43
> 写道：
> > > > > >
> > > > > > Enables HEVC Range Extension decoding support (Linux) for 4:2:2
> > 8/10
> > > > bit
> > > > > > on ICL+ (gen11 +) platform.
> > > > > >
> > > > > > Signed-off-by: Linjie Fu 
> > > > > > ---
> > > > > > [v2]: restrict to support on Linux.
> > > > > >
> > > > > >  libavcodec/qsv.c  | 16 
> > > > > >  libavutil/hwcontext_qsv.c | 24 
> > > > > >  2 files changed, 40 insertions(+)
> > > > > >
> > > > > > diff --git a/libavcodec/qsv.c b/libavcodec/qsv.c
> > > > > > index db98c75..171a8d6 100644
> > > > > > --- a/libavcodec/qsv.c
> > > > > > +++ b/libavcodec/qsv.c
> > > > > > @@ -195,6 +195,12 @@ enum AVPixelFormat
> > > ff_qsv_map_fourcc(uint32_t
> > > > > fourcc)
> > > > > >  case MFX_FOURCC_NV12: return AV_PIX_FMT_NV12;
> > > > > >  case MFX_FOURCC_P010: return AV_PIX_FMT_P010;
> > > > > >  case MFX_FOURCC_P8:   return AV_PIX_FMT_PAL8;
> > > > > > +#if CONFIG_VAAPI
> > > >
> > > LGTM. CONFIG_VAAPI is not needed here because crash does not
> related to
> > > these changes.
> > > Full support MFX_FOURCC_YUY2 is WIP for Windows.
> > >
> > > > > > +case MFX_FOURCC_YUY2: return AV_PIX_FMT_YUYV422;
> > > > >
> > > > > There is no VAAPI structures here, so should not use CONFIG_VAAPI
> to
> > > > > disable them.
> > > > >
> > > >
> > > > This is pointed out in [1] that D3D code doesn't support YUYV format,
> > and
> > > > indeed
> > > > It leads to unexpected crash in windows.(instead of working or
> > reporting
> > > > unsupported
> > > > On ICL- platform)
> > > >
> > > > Hence this patch restricted to add support on linux only.
> > > >
> > > > And I admit the best solution should be get this fully supported on
> > both
> > > > linux and windows.
> > > > (I believe Max and Artem is helping on windows side)
> > > >
> > > > Thanks for the review,
> > > > Linjie
> > > >
> > > > [1]
> > > > https://patchwork.ffmpeg.org/project/ffmpeg/patch/1582596080-
> 1035-1-
> > > git-send-email-linjie...@intel.com/
> >
> > Synced with Zhong, will keep the restriction for now and apply this set
> > soon. (if no objections)
> >
> >
> it make sense, as patch is clearly Linux focused
> 
Thanks, applied.

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 6/6] avcodec: move mpeg4 profiles to profiles.h

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of
> Marton Balint
> Sent: Tuesday, May 12, 2020 03:35
> To: ffmpeg-devel@ffmpeg.org
> Cc: Marton Balint 
> Subject: [FFmpeg-devel] [PATCH 6/6] avcodec: move mpeg4 profiles to
> profiles.h
> 
> Signed-off-by: Marton Balint 
> ---
>  doc/codecs.texi| 17 ++---
>  libavcodec/mpeg4videoenc.c |  2 ++
>  libavcodec/options_table.h |  4 
>  libavcodec/profiles.h  |  6 ++
>  libavcodec/v4l2_m2m_enc.c  | 34 ++
>  5 files changed, 32 insertions(+), 31 deletions(-)
> 
> diff --git a/doc/codecs.texi b/doc/codecs.texi
> index c08229ba7e..ece8d50edd 100644
> --- a/doc/codecs.texi
> +++ b/doc/codecs.texi
> @@ -837,21 +837,8 @@ Set number of macroblock rows at the bottom which
> are skipped.
> 
>  @item profile @var{integer} (@emph{encoding,audio,video})
> 
> -Possible values:
> -@table @samp
> -@item unknown
> -
> -@item mpeg4_sp
> -
> -@item mpeg4_core
> -
> -@item mpeg4_main
> -
> -@item mpeg4_asp
> -
> -@end table
> -
> -Encoder specific profiles are documented in the relevant encoder
> documentation.
> +Set encoder codec profile. Default value is @samp{unknown}. Encoder
> specific
> +profiles are documented in the relevant encoder documentation.
> 
>  @item level @var{integer} (@emph{encoding,audio,video})
> 
> diff --git a/libavcodec/mpeg4videoenc.c b/libavcodec/mpeg4videoenc.c
> index 2cd5a8c015..2e0b119d7f 100644
> --- a/libavcodec/mpeg4videoenc.c
> +++ b/libavcodec/mpeg4videoenc.c
> @@ -27,6 +27,7 @@
>  #include "mpegvideo.h"
>  #include "h263.h"
>  #include "mpeg4video.h"
> +#include "profiles.h"
> 
>  /* The uni_DCtab_* tables below contain unified bits+length tables to
> encode DC
>   * differences in MPEG-4. Unified in the sense that the specification 
> specifies
> @@ -1376,6 +1377,7 @@ static const AVOption options[] = {
>  { "data_partitioning", "Use data partitioning.",
> OFFSET(data_partitioning), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, VE },
>  { "alternate_scan","Enable alternate scantable.",
> OFFSET(alternate_scan),AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, VE },
>  FF_MPV_COMMON_OPTS
> +FF_MPEG4_PROFILE_OPTS
>  { NULL },
>  };
> 
> diff --git a/libavcodec/options_table.h b/libavcodec/options_table.h
> index 4c7dca696b..6db8facff6 100644
> --- a/libavcodec/options_table.h
> +++ b/libavcodec/options_table.h
> @@ -262,10 +262,6 @@ static const AVOption avcodec_options[] = {
>  {"skip_bottom", "number of macroblock rows at the bottom which are
> skipped", OFFSET(skip_bottom), AV_OPT_TYPE_INT, {.i64 = DEFAULT },
> INT_MIN, INT_MAX, V|D},
>  {"profile", NULL, OFFSET(profile), AV_OPT_TYPE_INT, {.i64 =
> FF_PROFILE_UNKNOWN }, INT_MIN, INT_MAX, V|A|E|CC, "avctx.profile"},
>  {"unknown", NULL, 0, AV_OPT_TYPE_CONST, {.i64 =
> FF_PROFILE_UNKNOWN }, INT_MIN, INT_MAX, V|A|E, "avctx.profile"},
> -{"mpeg4_sp",   NULL, 0, AV_OPT_TYPE_CONST, {.i64 =
> FF_PROFILE_MPEG4_SIMPLE }, INT_MIN, INT_MAX, V|E, "avctx.profile"},
> -{"mpeg4_core", NULL, 0, AV_OPT_TYPE_CONST, {.i64 =
> FF_PROFILE_MPEG4_CORE }, INT_MIN, INT_MAX, V|E, "avctx.profile"},
> -{"mpeg4_main", NULL, 0, AV_OPT_TYPE_CONST, {.i64 =
> FF_PROFILE_MPEG4_MAIN }, INT_MIN, INT_MAX, V|E, "avctx.profile"},
> -{"mpeg4_asp",  NULL, 0, AV_OPT_TYPE_CONST, {.i64 =
> FF_PROFILE_MPEG4_ADVANCED_SIMPLE }, INT_MIN, INT_MAX, V|E,
> "avctx.profile"},
>  {"main10",  NULL, 0, AV_OPT_TYPE_CONST, {.i64 =
> FF_PROFILE_HEVC_MAIN_10 }, INT_MIN, INT_MAX, V|E, "avctx.profile"},
>  {"level", NULL, OFFSET(level), AV_OPT_TYPE_INT, {.i64 =
> FF_LEVEL_UNKNOWN }, INT_MIN, INT_MAX, V|A|E, "level"},
>  {"unknown", NULL, 0, AV_OPT_TYPE_CONST, {.i64 = FF_LEVEL_UNKNOWN },
> INT_MIN, INT_MAX, V|A|E, "level"},
> diff --git a/libavcodec/profiles.h b/libavcodec/profiles.h
> index d950971875..e414ea77a7 100644
> --- a/libavcodec/profiles.h
> +++ b/libavcodec/profiles.h
> @@ -37,6 +37,12 @@
>  FF_AVCTX_PROFILE_OPTION("mpeg2_aac_low", NULL, AUDIO,
> FF_PROFILE_MPEG2_AAC_LOW)\
>  FF_AVCTX_PROFILE_OPTION("mpeg2_aac_he",  NULL, AUDIO,
> FF_PROFILE_MPEG2_AAC_HE)\
> 
> +#define FF_MPEG4_PROFILE_OPTS \
> +FF_AVCTX_PROFILE_OPTION("mpeg4_sp",  NULL, VIDEO,
> FF_PROFILE_MPEG4_SIMPLE)\
> +FF_AVCTX_PROFILE_OPTION("mpeg4_core",NULL, VIDEO,
> FF_PROFILE_MPEG4_CORE)\
> +FF_AVCTX_PROFILE_OPTION("mpeg4_main",NULL, VIDEO,
> FF_PROFILE_MPEG4_MAIN)\
> +FF_AVCTX_PROFILE_OPTION("mpeg4_asp", NULL, VIDEO,
> FF_PROFILE_MPEG4_ADVANCED_SIMPLE)\
> +
Since it's now searchable according to the AVClass, we may not need
the prefix anymore. (for now it's necessary for the ABI)

Hence would it be better to add some deprecated flags for them and
remove the prefix in future  when avcodec version increase?
(something like FF_API_NEXT)

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject 

Re: [FFmpeg-devel] [PATCH 1/6] avutil/opt: add AV_OPT_FLAG_CHILD_CONSTS

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of
> Marton Balint
> Sent: Tuesday, May 12, 2020 03:35
> To: ffmpeg-devel@ffmpeg.org
> Cc: Marton Balint 
> Subject: [FFmpeg-devel] [PATCH 1/6] avutil/opt: add
> AV_OPT_FLAG_CHILD_CONSTS
> 
> This will be used for AVCodecContext->profile. By specifying constants in the
> encoders we won't have to use the common AVCodecContext options table
> and
> different encoders can use the same profile name even with different values.
> 
> Signed-off-by: Marton Balint 
> ---
>  doc/APIchanges  | 3 +++
>  libavutil/opt.c | 3 ++-
>  libavutil/opt.h | 1 +
>  libavutil/version.h | 2 +-
>  4 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/doc/APIchanges b/doc/APIchanges
> index 75cfdb08b0..235888c174 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -15,6 +15,9 @@ libavutil: 2017-10-21
> 
>  API changes, most recent first:
> 
> +2020-05-xx - xx - lavu 56.44.101 - opt.h
> +  Add AV_OPT_FLAG_CHILD_CONSTS.
> +
>  2020-05-10 - xx - lavu 56.44.100 - hwcontext_vulkan.h
>Add enabled_inst_extensions, num_enabled_inst_extensions,
> enabled_dev_extensions
>and num_enabled_dev_extensions fields to AVVulkanDeviceContext
> diff --git a/libavutil/opt.c b/libavutil/opt.c
> index b792dec01c..423313bce2 100644
> --- a/libavutil/opt.c
> +++ b/libavutil/opt.c
> @@ -256,11 +256,12 @@ static int set_string_number(void *obj, void
> *target_obj, const AVOption *o, con
>  }
> 
>  {
> -const AVOption *o_named = av_opt_find(target_obj, i ? buf : val, 
> o-
> >unit, 0, 0);
>  int res;
>  int ci = 0;
>  double const_values[64];
>  const char * const_names[64];
> +int search_flags = (o->flags & AV_OPT_FLAG_CHILD_CONSTS) ?
> AV_OPT_SEARCH_CHILDREN : 0;
> +const AVOption *o_named = av_opt_find(target_obj, i ? buf : val, 
> o-
> >unit, 0, search_flags);
>  if (o_named && o_named->type == AV_OPT_TYPE_CONST)
>  d = DEFAULT_NUMVAL(o_named);
>  else {
> diff --git a/libavutil/opt.h b/libavutil/opt.h
> index 1969c984dd..e46119572a 100644
> --- a/libavutil/opt.h
> +++ b/libavutil/opt.h
> @@ -291,6 +291,7 @@ typedef struct AVOption {
>  #define AV_OPT_FLAG_RUNTIME_PARAM   (1<<15) ///< a generic
> parameter which can be set by the user at runtime
>  #define AV_OPT_FLAG_FILTERING_PARAM (1<<16) ///< a generic
> parameter which can be set by the user for filtering
>  #define AV_OPT_FLAG_DEPRECATED  (1<<17) ///< set if option is
> deprecated, users should refer to AVOption.help text for more information
> +#define AV_OPT_FLAG_CHILD_CONSTS(1<<18) ///< set if option
> constants can also reside in child objects
>  //FIXME think about enc-audio, ... style flags
> 
>  /**
> diff --git a/libavutil/version.h b/libavutil/version.h
> index 48d8a38c42..c4946c1c7e 100644
> --- a/libavutil/version.h
> +++ b/libavutil/version.h
> @@ -80,7 +80,7 @@
> 
>  #define LIBAVUTIL_VERSION_MAJOR  56
>  #define LIBAVUTIL_VERSION_MINOR  44
> -#define LIBAVUTIL_VERSION_MICRO 100
> +#define LIBAVUTIL_VERSION_MICRO 101
> 
>  #define LIBAVUTIL_VERSION_INT
> AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
> LIBAVUTIL_VERSION_MINOR, \
> --
> 2.16.4

I'm with this idea to specify avctx->profile with candidate options
in child encoder.

Also verified that this would benefit encoders who share the standard
profile options and use avtcx->profile directly. For others we may need
to implement some corresponding changes to transfer from private->profile
to avctx->profile. (But we may keep it for now, and change step by step)

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [RFC] encoder profile validation

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of
> Marton Balint
> Sent: Saturday, May 16, 2020 21:52
> To: ffmpeg-devel@ffmpeg.org
> Subject: [FFmpeg-devel] [RFC] encoder profile validation
> 
> Hi,
> 
> As you may know, a recent patchset enabled AVCodecContext->profile
> constants to reside in encoders.
> 
> In order to make a full transition to avctx->profile even in existing
> encoders which might use a private profile setting, we have to make sure
> only supported avctx->profile values are passed to encoders.
> 
> The fact that avctx->profile is not validated is already an issue, and
> assertions/segmentation faults can already happen in existing encoders
> (e.g.: aac, mpeg) if unsupported values are passed.
> 
> AVCodec have a .profiles attribute which supposed to contain the list of
> supported profiles. However this is problematic because
> - AVCodecContext->profile is not validated against this list
> - not all encoders define the list
> - even if there is a list it might be defined as NULL_IF_CONFIG_SMALL
> - some encoders support more or less than its currently defined list
> - AVCodec->profiles contains AVProfiles which supposed to have a textual
>representation of each profile, which can cause profile name
>duplications/inconsistencies against libavcodec/profiles.c if the list
>is different to the one in codecs profile list.
> 
> So I'd rather not user AVCodec->profiles for this validation. I am

The validation would help from my perspective. Meanwhile got some questions:

IIRC, AVCodec->profiles probably didn't cover the HW specific profiles for 
encoders
Like nvenc/qsv/amfenc. Are you intending to add them in this profile list  and 
keep
Them in common or did the map/verify internally in specific hw encoder?

> thinking about two possible solutions:
> 
> 1) Add a new AVCodec->supported_profiles attribute which is a simple
> integer list and validate against this list
> 
> or
> 
> 2) Introdce a new OPT type, AV_OPT_TYPE_ENUM which validates against
> the
> values of its named constants. This has the benefit that the supported
> profiles only appear once in the AVClass option list, but it also
> means that encoders might not share the supported profiles list via a
> #define in libavcodec/profile.h, because they might support different
> profiles.
> 
> I like the second approach better, but let me know what you think.
> 
Regardless of the validation itself, as to " make a full transition to 
avctx->profile", 
do we still need to add a prefix like "-profile:v h264_main " in your designs?
(I'd prefer a more natural usage)

- Linjie


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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 4/4] vaapi_encode_h265: Enable 4:2:2 support

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of
> Mark Thompson
> Sent: Sunday, March 8, 2020 00:15
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH 4/4] vaapi_encode_h265: Enable 4:2:2
> support
> 
> On 05/03/2020 02:49, Fu, Linjie wrote:
> > 2. recon surface of Y210 or 444 (AYUV and Y410 in media-driver) depends
> on the surface hint [3] in
> > libva and corresponding code in media-driver to resize the recon surface
> which is not upstreamed
> > yet.
> 
> What is the reasoning for forcing the user to pass new extra attributes with
> this rather than handling it transparently so that it works like all other
> encoders?
> 
> In some places in Mesa surfaces are reallocated with different properties
> when they are used for a purpose they don't currently support, which avoids
> weird constraints being exported to the user (e.g. see
> <https://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/state_trackers/v
> a/surface.c#n1000>).  Since reconstructed picture surfaces are pretty unlikely
> to be used for anything else (just being copied out for debugging maybe?), it
> seems like an answer like that would be simpler in this case too.  (Though
> perhaps I'm missing something weird about the Intel hardware which makes
> this case uglier.)
> 

Implemented the surface reallocation inside media driver in [1], merged the 
query
support in [2],  verified that it works for both AYUV(or XYUV)/Y410, yuyv422.

And for Y210, it seems to be better to implement render target support in
vaapi_encoder in this patch as well:
{ "YUV422_10", VA_RT_FORMAT_YUV422_10,10, 3, 1, 0 },

Hence patch LGTM with or without above modifications, thx.

- Linjie

[1] < https://github.com/intel/media-driver/pull/915>
[2] < https://github.com/intel/media-driver/pull/855>

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH, v2 1/2] lavc/qsvdec: add decode support for HEVC 4:2:2 8-bit and 10-bit

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of
> Artem Galin
> Sent: Tuesday, April 28, 2020 00:26
> To: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Cc: Zhong Li 
> Subject: Re: [FFmpeg-devel] [PATCH, v2 1/2] lavc/qsvdec: add decode
> support for HEVC 4:2:2 8-bit and 10-bit
> 
> On Wed, 15 Apr 2020 at 05:02, Fu, Linjie  wrote:
> 
> > > From: Zhong Li 
> > > Sent: Wednesday, April 15, 2020 09:58
> > > To: FFmpeg development discussions and patches  > > de...@ffmpeg.org>
> > > Cc: Fu, Linjie 
> > > Subject: Re: [FFmpeg-devel] [PATCH, v2 1/2] lavc/qsvdec: add decode
> > > support for HEVC 4:2:2 8-bit and 10-bit
> > >
> > > Linjie Fu  于2020年2月26日周三 下午4:43写道：
> > > >
> > > > Enables HEVC Range Extension decoding support (Linux) for 4:2:2 8/10
> > bit
> > > > on ICL+ (gen11 +) platform.
> > > >
> > > > Signed-off-by: Linjie Fu 
> > > > ---
> > > > [v2]: restrict to support on Linux.
> > > >
> > > >  libavcodec/qsv.c  | 16 
> > > >  libavutil/hwcontext_qsv.c | 24 
> > > >  2 files changed, 40 insertions(+)
> > > >
> > > > diff --git a/libavcodec/qsv.c b/libavcodec/qsv.c
> > > > index db98c75..171a8d6 100644
> > > > --- a/libavcodec/qsv.c
> > > > +++ b/libavcodec/qsv.c
> > > > @@ -195,6 +195,12 @@ enum AVPixelFormat
> ff_qsv_map_fourcc(uint32_t
> > > fourcc)
> > > >  case MFX_FOURCC_NV12: return AV_PIX_FMT_NV12;
> > > >  case MFX_FOURCC_P010: return AV_PIX_FMT_P010;
> > > >  case MFX_FOURCC_P8:   return AV_PIX_FMT_PAL8;
> > > > +#if CONFIG_VAAPI
> >
> LGTM. CONFIG_VAAPI is not needed here because crash does not related to
> these changes.
> Full support MFX_FOURCC_YUY2 is WIP for Windows.
> 
> > > > +case MFX_FOURCC_YUY2: return AV_PIX_FMT_YUYV422;
> > >
> > > There is no VAAPI structures here, so should not use CONFIG_VAAPI to
> > > disable them.
> > >
> >
> > This is pointed out in [1] that D3D code doesn't support YUYV format, and
> > indeed
> > It leads to unexpected crash in windows.(instead of working or reporting
> > unsupported
> > On ICL- platform)
> >
> > Hence this patch restricted to add support on linux only.
> >
> > And I admit the best solution should be get this fully supported on both
> > linux and windows.
> > (I believe Max and Artem is helping on windows side)
> >
> > Thanks for the review,
> > Linjie
> >
> > [1]
> > https://patchwork.ffmpeg.org/project/ffmpeg/patch/1582596080-1035-1-
> git-send-email-linjie...@intel.com/

Synced with Zhong, will keep the restriction for now and apply this set soon. 
(if no objections)

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v5 1/3] lavc/libopenh264enc: Rewrite profile handling

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of Fu,
> Linjie
> Sent: Tuesday, May 12, 2020 21:58
> To: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH v5 1/3] lavc/libopenh264enc: Rewrite
> profile handling
> 
> > From: ffmpeg-devel  On Behalf Of
> > Martin Storsjö
> > Sent: Tuesday, May 12, 2020 17:36
> > To: FFmpeg development discussions and patches  > de...@ffmpeg.org>
> > Cc: Fu, Linjie 
> > Subject: Re: [FFmpeg-devel] [PATCH v5 1/3] lavc/libopenh264enc: Rewrite
> > profile handling
> >
> > On Wed, 6 May 2020, Linjie Fu wrote:
> >
> > > Support the profiles "constrained_baseline" and "high" for libopenh264
> > > version >= 1.8, support "constrained_baseline" and "main" for earlier
> > > version.
> > >
> > > If option not supported with current version, convert to constrained
> > > baseline with a warning for users.
> > >
> > > Signed-off-by: Linjie Fu 
> > > ---
> > > libavcodec/libopenh264enc.c | 44
> > +
> > > 1 file changed, 40 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c
> > > index 3bb3cceb64..12c71ca0e9 100644
> > > --- a/libavcodec/libopenh264enc.c
> > > +++ b/libavcodec/libopenh264enc.c
> > > @@ -44,7 +44,7 @@ typedef struct SVCContext {
> > > ISVCEncoder *encoder;
> > > int slice_mode;
> > > int loopfilter;
> > > -char *profile;
> > > +int profile;
> > > int max_nal_size;
> > > int skip_frames;
> > > int skipped;
> > > @@ -75,7 +75,12 @@ static const AVOption options[] = {
> > > #endif
> > > #endif
> > > { "loopfilter", "enable loop filter", OFFSET(loopfilter),
> AV_OPT_TYPE_INT,
> > { .i64 = 1 }, 0, 1, VE },
> > > -{ "profile", "set profile restrictions", OFFSET(profile),
> > AV_OPT_TYPE_STRING, { .str = NULL }, 0, 0, VE },
> > > +{ "profile", "set profile restrictions", OFFSET(profile),
> AV_OPT_TYPE_INT,
> > { .i64 = FF_PROFILE_UNKNOWN }, FF_PROFILE_UNKNOWN, 0x, VE,
> > "profile" },
> > > +#define PROFILE(name, value)  name, NULL, 0, AV_OPT_TYPE_CONST,
> > { .i64 = value }, 0, 0, VE, "profile"
> > > +{ PROFILE("constrained_baseline",
> > FF_PROFILE_H264_CONSTRAINED_BASELINE) },
> > > +{ PROFILE("main", FF_PROFILE_H264_MAIN) },
> > > +{ PROFILE("high", FF_PROFILE_H264_HIGH) },
> > > +#undef PROFILE
> > > { "max_nal_size", "set maximum NAL size in bytes",
> > OFFSET(max_nal_size), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, VE },
> > > { "allow_skip_frames", "allow skipping frames to hit the target 
> > > bitrate",
> > OFFSET(skip_frames), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, VE },
> > > { "cabac", "Enable cabac", OFFSET(cabac), AV_OPT_TYPE_INT, { .i64 = 0 
> > > },
> > 0, 1, VE },
> > > @@ -176,10 +181,41 @@ FF_ENABLE_DEPRECATION_WARNINGS
> > > param.iLoopFilterDisableIdc  = !s->loopfilter;
> > > param.iEntropyCodingModeFlag = 0;
> > > param.iMultipleThreadIdc = avctx->thread_count;
> > > -if (s->profile && !strcmp(s->profile, "main"))
> > > +
> > > +if (s->profile == FF_PROFILE_UNKNOWN)
> > > +s->profile = !s->cabac ?
> FF_PROFILE_H264_CONSTRAINED_BASELINE :
> > > +#if OPENH264_VER_AT_LEAST(1, 8)
> > > + FF_PROFILE_H264_HIGH;
> > > +#else
> > > + FF_PROFILE_H264_MAIN;
> > > +#endif
> > > +
> > > +switch (s->profile) {
> > > +#if OPENH264_VER_AT_LEAST(1, 8)
> > > +case FF_PROFILE_H264_HIGH:
> > > param.iEntropyCodingModeFlag = 1;
> > > -else if (!s->profile && s->cabac)
> > > +av_log(avctx, AV_LOG_VERBOSE, "Using CABAC, "
> > > +"select EProfileIdc PRO_HIGH in libopenh264.\n");
> > > +break;
> > > +#else
> > > +case FF_PROFILE_H264_MAIN:
> > > param.iEntropyCodingModeFlag = 1;
> > > +av_log(avctx, AV_LOG_VERBOSE, "Usin

Re: [FFmpeg-devel] [PATCH v5 1/3] lavc/libopenh264enc: Rewrite profile handling

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of
> Martin Storsjö
> Sent: Tuesday, May 12, 2020 17:36
> To: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Cc: Fu, Linjie 
> Subject: Re: [FFmpeg-devel] [PATCH v5 1/3] lavc/libopenh264enc: Rewrite
> profile handling
> 
> On Wed, 6 May 2020, Linjie Fu wrote:
> 
> > Support the profiles "constrained_baseline" and "high" for libopenh264
> > version >= 1.8, support "constrained_baseline" and "main" for earlier
> > version.
> >
> > If option not supported with current version, convert to constrained
> > baseline with a warning for users.
> >
> > Signed-off-by: Linjie Fu 
> > ---
> > libavcodec/libopenh264enc.c | 44
> +
> > 1 file changed, 40 insertions(+), 4 deletions(-)
> >
> > diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c
> > index 3bb3cceb64..12c71ca0e9 100644
> > --- a/libavcodec/libopenh264enc.c
> > +++ b/libavcodec/libopenh264enc.c
> > @@ -44,7 +44,7 @@ typedef struct SVCContext {
> > ISVCEncoder *encoder;
> > int slice_mode;
> > int loopfilter;
> > -char *profile;
> > +int profile;
> > int max_nal_size;
> > int skip_frames;
> > int skipped;
> > @@ -75,7 +75,12 @@ static const AVOption options[] = {
> > #endif
> > #endif
> > { "loopfilter", "enable loop filter", OFFSET(loopfilter), 
> > AV_OPT_TYPE_INT,
> { .i64 = 1 }, 0, 1, VE },
> > -{ "profile", "set profile restrictions", OFFSET(profile),
> AV_OPT_TYPE_STRING, { .str = NULL }, 0, 0, VE },
> > +{ "profile", "set profile restrictions", OFFSET(profile), 
> > AV_OPT_TYPE_INT,
> { .i64 = FF_PROFILE_UNKNOWN }, FF_PROFILE_UNKNOWN, 0x, VE,
> "profile" },
> > +#define PROFILE(name, value)  name, NULL, 0, AV_OPT_TYPE_CONST,
> { .i64 = value }, 0, 0, VE, "profile"
> > +{ PROFILE("constrained_baseline",
> FF_PROFILE_H264_CONSTRAINED_BASELINE) },
> > +{ PROFILE("main", FF_PROFILE_H264_MAIN) },
> > +{ PROFILE("high", FF_PROFILE_H264_HIGH) },
> > +#undef PROFILE
> > { "max_nal_size", "set maximum NAL size in bytes",
> OFFSET(max_nal_size), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, VE },
> > { "allow_skip_frames", "allow skipping frames to hit the target 
> > bitrate",
> OFFSET(skip_frames), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, VE },
> > { "cabac", "Enable cabac", OFFSET(cabac), AV_OPT_TYPE_INT, { .i64 = 0 },
> 0, 1, VE },
> > @@ -176,10 +181,41 @@ FF_ENABLE_DEPRECATION_WARNINGS
> > param.iLoopFilterDisableIdc  = !s->loopfilter;
> > param.iEntropyCodingModeFlag = 0;
> > param.iMultipleThreadIdc = avctx->thread_count;
> > -if (s->profile && !strcmp(s->profile, "main"))
> > +
> > +if (s->profile == FF_PROFILE_UNKNOWN)
> > +s->profile = !s->cabac ? FF_PROFILE_H264_CONSTRAINED_BASELINE :
> > +#if OPENH264_VER_AT_LEAST(1, 8)
> > + FF_PROFILE_H264_HIGH;
> > +#else
> > + FF_PROFILE_H264_MAIN;
> > +#endif
> > +
> > +switch (s->profile) {
> > +#if OPENH264_VER_AT_LEAST(1, 8)
> > +case FF_PROFILE_H264_HIGH:
> > param.iEntropyCodingModeFlag = 1;
> > -else if (!s->profile && s->cabac)
> > +av_log(avctx, AV_LOG_VERBOSE, "Using CABAC, "
> > +"select EProfileIdc PRO_HIGH in libopenh264.\n");
> > +break;
> > +#else
> > +case FF_PROFILE_H264_MAIN:
> > param.iEntropyCodingModeFlag = 1;
> > +av_log(avctx, AV_LOG_VERBOSE, "Using CABAC, "
> > +"select EProfileIdc PRO_MAIN in libopenh264.\n");
> > +break;
> > +#endif
> > +case FF_PROFILE_H264_CONSTRAINED_BASELINE:
> > +case FF_PROFILE_UNKNOWN:
> > +param.iEntropyCodingModeFlag = 0;
> > +av_log(avctx, AV_LOG_VERBOSE, "Using CAVLC, "
> > +   "select EProfileIdc PRO_BASELINE in libopenh264.\n");
> > +break;
> > +default:
> > +param.iEntropyCodingModeFlag = 0;
> > +av_log(avctx, AV_LOG_WARNING, "Unsupported profile, "
> > +   "select EProfileIdc PRO_BASELINE in libopenh264.\n");
> > +break;
> > +}
> >
> > param.sSpatialLayers[0].iVideoWidth = param.iPicWidth;
> > param.sSpatialLayers[0].iVideoHeight= param.iPicHeight;
> > --
> > 2.17.1
> 
> Thanks, this looks ok to me.
> 
> // Martin
> 
Thanks for the review.

- Linjie

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH, RFC] lavc/options_table: Add basic candidate h264 profiles

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of
> lance.lmw...@gmail.com
> Sent: Wednesday, May 6, 2020 22:57
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH, RFC] lavc/options_table: Add basic
> candidate h264 profiles
> 
> On Wed, May 06, 2020 at 06:13:10PM +0800, myp...@gmail.com wrote:
> > On Wed, May 6, 2020 at 6:04 PM myp...@gmail.com
>  wrote:
> > >
> > > On Wed, May 6, 2020 at 5:40 PM Martin Storsjö 
> wrote:
> > > >
> > > > On Wed, 6 May 2020, Linjie Fu wrote:
> > > >
> > > > > Allows specifying avctx->profile with string type profile for
> > > > > h264 encoders.
> > > > >
> > > > > Private field/option "profile" may be abled to be removed for basic
> > > > > h264 profiles, directly for encoders like libopenh264/h264_vaapi,
> > > > > or with an map to hardware profile like h264_qsv.
> > > > >
> > > > > Signed-off-by: Linjie Fu 
> > > > > ---
> > > > > One concern is some encoders have options for specific profiles,
> > > > > like "high444p" for nvenc_h264 and "constrained_high" for h264_amf,
> > > > > hence they may not be able to remove the private option easily.
> > > > >
> > > > > Please help to comment.
> > > > >
> > > > > libavcodec/options_table.h | 4 
> > > > > 1 file changed, 4 insertions(+)
> > > >
> > > > This change in itself looks sensible to me (but I'd like for someone 
> > > > else
> > > > to comment as well). Even if it might not be able to get rid of the
> > > > private option for all encoders, it should at least simplify the easiest
> > > > cases.
> > > >
> > > > // Martin
> > > I think we have a discussion about this case,
> > > https://patchwork.ffmpeg.org/project/ffmpeg/patch/fa14da65-9e1a-
> c74b-b3cb-46fc5f3ea...@gmail.com/
> >
> > I agree with Hendrik Leppkes, and I think "main" more natural than
> > "h264_main"  for 264 codec profile from a user viewpoint, both of
> > command line tools or  application program interface
> 
> So what's the conclusion? I have submit a patch to add mpeg2 profile
>  52 and Marton prefer to use global profile with mpeg2_. By Hendrik's
> comments, I think it's more reasonable to use private profile.

I'd also prefer an option without prefix which seems more natural, especially
for the codecs that already have implemented it as a private option. Otherwise
it seems to be a sudden break for users to use "h264_main"/"mepg2_main"
instead of "main" unless we declared some deprecated flags and get rid of
the private options later.

Please help to comment.

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH, RFC] lavc/options_table: Add basic candidate h264 profiles

[Fu, Linjie]

> From: myp...@gmail.com 
> Sent: Wednesday, May 6, 2020 18:13
> To: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Cc: Fu, Linjie 
> Subject: Re: [FFmpeg-devel] [PATCH, RFC] lavc/options_table: Add basic
> candidate h264 profiles
> 
> On Wed, May 6, 2020 at 6:04 PM myp...@gmail.com 
> wrote:
> >
> > On Wed, May 6, 2020 at 5:40 PM Martin Storsjö  wrote:
> > >
> > > On Wed, 6 May 2020, Linjie Fu wrote:
> > >
> > > > Allows specifying avctx->profile with string type profile for
> > > > h264 encoders.
> > > >
> > > > Private field/option "profile" may be abled to be removed for basic
> > > > h264 profiles, directly for encoders like libopenh264/h264_vaapi,
> > > > or with an map to hardware profile like h264_qsv.
> > > >
> > > > Signed-off-by: Linjie Fu 
> > > > ---
> > > > One concern is some encoders have options for specific profiles,
> > > > like "high444p" for nvenc_h264 and "constrained_high" for h264_amf,
> > > > hence they may not be able to remove the private option easily.
> > > >
> > > > Please help to comment.
> > > >
> > > > libavcodec/options_table.h | 4 
> > > > 1 file changed, 4 insertions(+)
> > >
> > > This change in itself looks sensible to me (but I'd like for someone else
> > > to comment as well). Even if it might not be able to get rid of the
> > > private option for all encoders, it should at least simplify the easiest
> > > cases.
> > >
> > > // Martin
> > I think we have a discussion about this case,
> > https://patchwork.ffmpeg.org/project/ffmpeg/patch/fa14da65-9e1a-
> c74b-b3cb-46fc5f3ea...@gmail.com/

Ahh, thanks for this input since it seems to be well-discussed.

> I agree with Hendrik Leppkes, and I think "main" more natural than
> "h264_main"  for 264 codec profile from a user viewpoint, both of
> command line tools or  application program interface

Indeed, being natural makes sense.
(otherwise we would have used numeric values instead)

- Linjie



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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v3] libavcodec/libx264: fix reference frame computation based on level

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of
> Josh Brewster
> Sent: Tuesday, May 5, 2020 23:52
> To: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH v3] libavcodec/libx264: fix reference
> frame computation based on level
> 
> > > From: ffmpeg-devel ffmpeg-devel-boun...@ffmpeg.org On Behalf Of
> > > Josh de Kock
> > > Sent: Tuesday, April 28, 2020 23:47
> > > To: ffmpeg-devel@ffmpeg.org
> > > Subject: Re: [FFmpeg-devel] [PATCH v3] libavcodec/libx264: fix reference
> > > frame computation based on level
> > > On 26/04/2020 12:46, Josh Brewster wrote:
> > >
> > > > Hi, is there anything else I need to do to have this merged?
> > >
> > > From a precursory look at what x264 and we're doing here your patch is
> > > correct. It doesn't break from a quick test, and looks OK to me. Would
> > > rather someone else has a look at it too but I will again in a couple
> > > days if no one does.
> >
> > Should be ok IMHO, thx.
> >
> > -   Linjie
> 
> Thanks for the feedback, I'll wait for it to be merged then.
> 

FYI, already merged several days ago with the help of Josh in:
https://git.ffmpeg.org/gitweb/ffmpeg.git/commit/79f001675a2bae16e243f30a3e7de9da6aeb3c2d

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] MAINTAINERS: Add myself to libopenh264enc

[Fu, Linjie]

> From: Fu, Linjie 
> Sent: Thursday, April 30, 2020 09:13
> To: ffmpeg-devel@ffmpeg.org
> Cc: Fu, Linjie 
> Subject: [PATCH] MAINTAINERS: Add myself to libopenh264enc
> 
> Reviewed-by: Martin Storsjö 
> Signed-off-by: Linjie Fu 
> ---
>  MAINTAINERS | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 06956f8..96654f1 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -194,6 +194,7 @@ Codecs:
>libdavs2.cHuiwen Ren
>libgsm.c  Michel Bardiaux
>libkvazaar.c  Arttu Ylä-Outinen
> +  libopenh264enc.c  Martin Storsjo, Linjie Fu
>libopenjpeg.c Jaikrishnan Menon
>libopenjpegenc.c  Michael Bradshaw
>libtheoraenc.cDavid Conrad
> --
> 2.7.4
Ping, pls help to comment.

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] lavc/vaapi_encode: add FF_CODEC_CAP_INIT_CLEANUP caps for encoders

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of
> Timo Rothenpieler
> Sent: Monday, May 4, 2020 13:20
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH] lavc/vaapi_encode: add
> FF_CODEC_CAP_INIT_CLEANUP caps for encoders
> 
> On 31.03.2020 17:34, Linjie Fu wrote:
> > ff_vaapi_encode_close() is not enough to free the resources like cbs
> > if initialization failure happens after codec->configure (except for
> > vp8/vp9).
> >
> > We need to call avctx->codec->close() to deallocate, otherwise memory
> > leak happens.
> >
> > Add FF_CODEC_CAP_INIT_CLEANUP for vaapi encoders and deallocate the
> > resources at free_and_end inside avcodec_open2().
> >
> > Signed-off-by: Linjie Fu 
> 
> Not my area of code, but the patch looks straight forward enough to my eyes.

Thanks for review, and hope this could be merged soon.

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] lavc/vaapi_encode: add FF_CODEC_CAP_INIT_CLEANUP caps for encoders

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of Fu,
> Linjie
> Sent: Thursday, April 23, 2020 22:59
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH] lavc/vaapi_encode: add
> FF_CODEC_CAP_INIT_CLEANUP caps for encoders
> 
> > From: Fu, Linjie 
> > Sent: Tuesday, March 31, 2020 23:34
> > To: ffmpeg-devel@ffmpeg.org
> > Cc: Fu, Linjie 
> > Subject: [PATCH] lavc/vaapi_encode: add FF_CODEC_CAP_INIT_CLEANUP
> > caps for encoders
> >
> > ff_vaapi_encode_close() is not enough to free the resources like cbs
> > if initialization failure happens after codec->configure (except for
> > vp8/vp9).
> >
> > We need to call avctx->codec->close() to deallocate, otherwise memory
> > leak happens.
> >
> > Add FF_CODEC_CAP_INIT_CLEANUP for vaapi encoders and deallocate the
> > resources at free_and_end inside avcodec_open2().
> >
> > Signed-off-by: Linjie Fu 
> > ---
> >  libavcodec/vaapi_encode.c   | 1 -
> >  libavcodec/vaapi_encode_h264.c  | 1 +
> >  libavcodec/vaapi_encode_h265.c  | 1 +
> >  libavcodec/vaapi_encode_mjpeg.c | 1 +
> >  libavcodec/vaapi_encode_mpeg2.c | 1 +
> >  libavcodec/vaapi_encode_vp8.c   | 1 +
> >  libavcodec/vaapi_encode_vp9.c   | 1 +
> >  7 files changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/libavcodec/vaapi_encode.c b/libavcodec/vaapi_encode.c
> > index 8ff720e..d371033 100644
> > --- a/libavcodec/vaapi_encode.c
> > +++ b/libavcodec/vaapi_encode.c
> > @@ -2361,7 +2361,6 @@ av_cold int
> ff_vaapi_encode_init(AVCodecContext
> > *avctx)
> >  return 0;
> >
> >  fail:
> > -ff_vaapi_encode_close(avctx);
> >  return err;
> >  }
> >
> > diff --git a/libavcodec/vaapi_encode_h264.c
> > b/libavcodec/vaapi_encode_h264.c
> > index f4965d8..6a86905 100644
> > --- a/libavcodec/vaapi_encode_h264.c
> > +++ b/libavcodec/vaapi_encode_h264.c
> > @@ -1356,6 +1356,7 @@ AVCodec ff_h264_vaapi_encoder = {
> >  .close  = _encode_h264_close,
> >  .priv_class = _encode_h264_class,
> >  .capabilities   = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_HARDWARE,
> > +.caps_internal  = FF_CODEC_CAP_INIT_CLEANUP,
> >  .defaults   = vaapi_encode_h264_defaults,
> >  .pix_fmts = (const enum AVPixelFormat[]) {
> >  AV_PIX_FMT_VAAPI,
> > diff --git a/libavcodec/vaapi_encode_h265.c
> > b/libavcodec/vaapi_encode_h265.c
> > index 97dc5a7..4c24f7c 100644
> > --- a/libavcodec/vaapi_encode_h265.c
> > +++ b/libavcodec/vaapi_encode_h265.c
> > @@ -1292,6 +1292,7 @@ AVCodec ff_hevc_vaapi_encoder = {
> >  .close  = _encode_h265_close,
> >  .priv_class = _encode_h265_class,
> >  .capabilities   = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_HARDWARE,
> > +.caps_internal  = FF_CODEC_CAP_INIT_CLEANUP,
> >  .defaults   = vaapi_encode_h265_defaults,
> >  .pix_fmts = (const enum AVPixelFormat[]) {
> >  AV_PIX_FMT_VAAPI,
> > diff --git a/libavcodec/vaapi_encode_mjpeg.c
> > b/libavcodec/vaapi_encode_mjpeg.c
> > index bd029cc..c469e70 100644
> > --- a/libavcodec/vaapi_encode_mjpeg.c
> > +++ b/libavcodec/vaapi_encode_mjpeg.c
> > @@ -565,6 +565,7 @@ AVCodec ff_mjpeg_vaapi_encoder = {
> >  .priv_class = _encode_mjpeg_class,
> >  .capabilities   = AV_CODEC_CAP_HARDWARE |
> >AV_CODEC_CAP_INTRA_ONLY,
> > +.caps_internal  = FF_CODEC_CAP_INIT_CLEANUP,
> >  .defaults   = vaapi_encode_mjpeg_defaults,
> >  .pix_fmts = (const enum AVPixelFormat[]) {
> >  AV_PIX_FMT_VAAPI,
> > diff --git a/libavcodec/vaapi_encode_mpeg2.c
> > b/libavcodec/vaapi_encode_mpeg2.c
> > index bac9ea1..55f9289 100644
> > --- a/libavcodec/vaapi_encode_mpeg2.c
> > +++ b/libavcodec/vaapi_encode_mpeg2.c
> > @@ -702,6 +702,7 @@ AVCodec ff_mpeg2_vaapi_encoder = {
> >  .close  = _encode_mpeg2_close,
> >  .priv_class = _encode_mpeg2_class,
> >  .capabilities   = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_HARDWARE,
> > +.caps_internal  = FF_CODEC_CAP_INIT_CLEANUP,
> >  .defaults   = vaapi_encode_mpeg2_defaults,
> >  .pix_fmts = (const enum AVPixelFormat[]) {
> >  AV_PIX_FMT_VAAPI,
> > diff --git a/libavcodec/vaapi_encode_vp8.c
> > b/libavcodec/vaapi_encode_vp8.c
> > index 6e7bf9d..d8fb031 100644
> > --- a/libavcodec/vaapi_encode_vp8.c
> > +++ b/libavcodec/vaapi_encode_vp8.c
> > @@ -257,6 +257,7 @@ AVCodec ff_vp8_vaapi_encoder = {
> >  .close  = _vaapi_encode_clos

Re: [FFmpeg-devel] [PATCH v4 8/9] lavc/libopenh264enc: allow specifying the profile through AVCodecContext

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of
> Martin Storsjö
> Sent: Tuesday, April 28, 2020 04:13
> To: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Cc: Fu, Linjie 
> Subject: Re: [FFmpeg-devel] [PATCH v4 8/9] lavc/libopenh264enc: allow
> specifying the profile through AVCodecContext
> 
> On Wed, 15 Apr 2020, Linjie Fu wrote:
> 
> > Signed-off-by: Linjie Fu 
> > ---
> > libavcodec/libopenh264enc.c | 16 
> > 1 file changed, 16 insertions(+)
> >
> > diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c
> > index 0fe8cf4..4d337d2 100644
> > --- a/libavcodec/libopenh264enc.c
> > +++ b/libavcodec/libopenh264enc.c
> > @@ -113,6 +113,22 @@ static av_cold int
> svc_encode_init_profile(AVCodecContext *avctx, SEncParamExt *
> > {
> > SVCContext *s = avctx->priv_data;
> >
> > +/* Allow specifying the libopenh264 profile through AVCodecContext.
> */
> > +if (FF_PROFILE_UNKNOWN == s->profile &&
> > +FF_PROFILE_UNKNOWN != avctx->profile)
> > +switch (avctx->profile) {
> > +case FF_PROFILE_H264_CONSTRAINED_BASELINE:
> > +s->profile = FF_PROFILE_H264_CONSTRAINED_BASELINE;
> > +break;
> > +case FF_PROFILE_H264_HIGH:
> > +s->profile = FF_PROFILE_H264_HIGH;
> > +break;
> > +default:
> > +av_log(avctx, AV_LOG_WARNING,
> > +   "Unsupported avctx->profile: %d.\n", avctx->profile);
> > +break;
> > +}
> > +
> > if (s->profile == FF_PROFILE_UNKNOWN)
> > s->profile = s->cabac ? FF_PROFILE_H264_HIGH :
> > FF_PROFILE_H264_CONSTRAINED_BASELINE;
> 
> With this in place, why even do the previous commit, why not just go for
> only using avctx->profile? Although as far as I can see, that field
> doesn't seem to have string options for sepcifying H264 profiles, so it
> can only be set via code or by specifying a numberic value - is that
> right?

Yes, code/numberic value works in this way, however maybe not straight
forward enough for user.

> Wouldn't it just be best to add the H264 profiles to options_table.h to
> keep allowing it to be set via a string, but remove the internal
> s->profile field here, as patch 7/9 already breaks handling of the profile
> field by stopping recognizing "main" and only recognizing "high" anyway.

Most sw codecs like libx264/libx265 and hw codecs like h264_vaapi/h264_nvenc
have the logic of :
1. derive the settings from avctx->profile which is determined in 
options_table.h;
2. If not, check the private option for specific encoder;

And specifically for libopenh264enc,  we allow one more logic:
3. determine the profile by s->cabac;

I admit it'll be good if settled this down to parse all possible profiles for 
each codec
(maybe according to libavcodec/profiles.c), hence we may remove 
similar/redundant
logics in specific encoder(x264/h264_vaapi/h264_nvenc).

Please help to comment whether it makes sense for you all, since this would 
impact
many codecs.

- Linjie



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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v4 7/9] lavc/libopenh264enc: add profile high option support

[Fu, Linjie]

> From: Martin Storsjö 
> Sent: Tuesday, April 28, 2020 04:09
> To: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Cc: Fu, Linjie 
> Subject: Re: [FFmpeg-devel] [PATCH v4 7/9] lavc/libopenh264enc: add profile
> high option support
> 
> On Wed, 15 Apr 2020, Linjie Fu wrote:
> 
> > Add support for PRO_HIGH/PRO_BASELINE in SVC Encoding extention
> mode,
> > which determined by iEntropyCodingModeFlag in ParamTranscode().
> >
> >
> <https://github.com/cisco/openh264/blob/master/codec/encoder/core/inc/
> param_svc.h#L394>
> >
> > Signed-off-by: Linjie Fu 
> > ---
> > libavcodec/libopenh264enc.c | 32 ++--
> > 1 file changed, 26 insertions(+), 6 deletions(-)
> >
> > diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c
> > index a7c389e..0fe8cf4 100644
> > --- a/libavcodec/libopenh264enc.c
> > +++ b/libavcodec/libopenh264enc.c
> > @@ -42,7 +42,7 @@ typedef struct SVCContext {
> > ISVCEncoder *encoder;
> > int slice_mode;
> > int loopfilter;
> > -char *profile;
> > +int profile;
> > int max_nal_size;
> > int skip_frames;
> > int skipped;
> > @@ -73,7 +73,11 @@ static const AVOption options[] = {
> > #endif
> > #endif
> > { "loopfilter", "enable loop filter", OFFSET(loopfilter), 
> > AV_OPT_TYPE_INT,
> { .i64 = 1 }, 0, 1, VE },
> > -{ "profile", "set profile restrictions", OFFSET(profile),
> AV_OPT_TYPE_STRING, { .str = NULL }, 0, 0, VE },
> > +{ "profile", "Set profile restrictions", OFFSET(profile), 
> > AV_OPT_TYPE_INT,
> { .i64 = FF_PROFILE_UNKNOWN }, FF_PROFILE_UNKNOWN, 0x, VE,
> "profile" },
> > +#define PROFILE(name, value)  name, NULL, 0, AV_OPT_TYPE_CONST,
> { .i64 = value }, 0, 0, VE, "profile"
> > +{ PROFILE("constrained_baseline",
> FF_PROFILE_H264_CONSTRAINED_BASELINE) },
> > +{ PROFILE("high", FF_PROFILE_H264_HIGH) },
> > +#undef PROFILE
> > { "max_nal_size", "set maximum NAL size in bytes",
> OFFSET(max_nal_size), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, VE },
> > { "allow_skip_frames", "allow skipping frames to hit the target 
> > bitrate",
> OFFSET(skip_frames), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, VE },
> > { "cabac", "Enable cabac", OFFSET(cabac), AV_OPT_TYPE_INT, { .i64 = 0 },
> 0, 1, VE },
> > @@ -109,12 +113,28 @@ static av_cold int
> svc_encode_init_profile(AVCodecContext *avctx, SEncParamExt *
> > {
> > SVCContext *s = avctx->priv_data;
> >
> > -if (s->profile && !strcmp(s->profile, "main"))
> > -param->iEntropyCodingModeFlag = 1; //< 0:CAVLC  1:CABAC
> > -else if (!s->profile && s->cabac)
> > +if (s->profile == FF_PROFILE_UNKNOWN)
> > +s->profile = s->cabac ? FF_PROFILE_H264_HIGH :
> > +FF_PROFILE_H264_CONSTRAINED_BASELINE;
> > +
> > +switch (s->profile) {
> > +case FF_PROFILE_H264_HIGH:
> > param->iEntropyCodingModeFlag = 1;
> > -else
> > +av_log(avctx, AV_LOG_VERBOSE, "Using CABAC, "
> > +   "select EProfileIdc PRO_HIGH in libopenh264.\n");
> > +break;
> > +case FF_PROFILE_H264_CONSTRAINED_BASELINE:
> > +case FF_PROFILE_UNKNOWN:
> > +param->iEntropyCodingModeFlag = 0;
> > +av_log(avctx, AV_LOG_VERBOSE, "Using CAVLC, "
> > +   "select EProfileIdc PRO_BASELINE in libopenh264.\n");
> > +break;
> > +default:
> > param->iEntropyCodingModeFlag = 0;
> > +av_log(avctx, AV_LOG_WARNING, "Unsupported profile, "
> > +   "select EProfileIdc PRO_BASELINE in libopenh264.\n");
> > +break;
> > +}
> >
> > return 0;
> > }
> > --
> > 2.7.4
> 
> This is fine I guess - apparently it changed in OpenH264 1.8, before that,
> setting iEntropyCodingModeFlag = 1 got main profile, not high.

Yes, main profile was somehow removed in [1] v1.7, while high profile
was introduced in [2] v1.8. 

> The commit message feels a bit misleading for what the commit really does
> though. The commit message says "add ... support", which sounds like it
> would be a pure addition, while it is a bit of a rewrite, and actually
> removes support for "-pro

Re: [FFmpeg-devel] [PATCH v6 1/5] lavc/libopenh264enc: Add qmin/qmax support

[Fu, Linjie]

> From: Martin Storsjö 
> Sent: Wednesday, April 29, 2020 16:52
> To: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Cc: Fu, Linjie 
> Subject: Re: [FFmpeg-devel] [PATCH v6 1/5] lavc/libopenh264enc: Add
> qmin/qmax support
> 
> On Wed, 29 Apr 2020, Linjie Fu wrote:
> 
> > Clip iMinQp/iMaxQp to (1, 51) for user specified qp range.
> >
> > If not set, leave iMinQp/iMaxQp untouched and use the values (0, 51)
> > initialized in FillDefault(), and the QP range would be adjusted to the
> > defaults inside libopenh264 library according to the iUsageType, (12, 42)
> > for iUsageType == CAMERA_VIDEO_REAL_TIME which is default.
> >
> >
> <https://github.com/cisco/openh264/blob/master/codec/encoder/core/src/
> encoder_ext.cpp#L375>
> >
> > Signed-off-by: Linjie Fu 
> > ---
> > libavcodec/libopenh264enc.c | 11 +++
> > 1 file changed, 11 insertions(+)
> 
> LGTM
> 
Thanks for your review for the whole set, hope it would be merge soon: )

- Linjie

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] tests/api/api-h264-slice-test: remove unused bool header

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of
> Carl Eugen Hoyos
> Sent: Thursday, April 2, 2020 03:23
> To: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH] tests/api/api-h264-slice-test: remove
> unused bool header
> 
> Am Mi., 1. Apr. 2020 um 06:58 Uhr schrieb Linjie Fu :
> >
> > Signed-off-by: Linjie Fu 
> > ---
> >  tests/api/api-h264-slice-test.c | 1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/tests/api/api-h264-slice-test.c 
> > b/tests/api/api-h264-slice-test.c
> > index dee93b8..b7aa405 100644
> > --- a/tests/api/api-h264-slice-test.c
> > +++ b/tests/api/api-h264-slice-test.c
> > @@ -24,7 +24,6 @@
> >
> >  #include "config.h"
> >
> > -#include 
> 
> Good idea.
> 
> Carl Eugen

Ping for this, thx.

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH, v4] lavc/vp9: fix reference frame dimensions check for SINGLE_REFERENCE mode

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of Fu,
> Linjie
> Sent: Friday, March 20, 2020 09:49
> To: Ronald S. Bultje ; FFmpeg development
> discussions and patches 
> Subject: Re: [FFmpeg-devel] [PATCH, v4] lavc/vp9: fix reference frame
> dimensions check for SINGLE_REFERENCE mode
> 
> > From: Ronald S. Bultje 
> > Sent: Thursday, March 19, 2020 20:10
> > To: FFmpeg development discussions and patches  de...@ffmpeg.org>
> > Cc: Fu, Linjie 
> > Subject: Re: [FFmpeg-devel] [PATCH, v4] lavc/vp9: fix reference frame
> dimensions check for SINGLE_REFERENCE mode
> >
> > Hi,
> >
> > On Tue, Mar 17, 2020 at 10:59 AM Linjie Fu
> mailto:linjie...@intel.com>> wrote:
> > With the description in frame size with refs semantics (SPEC 7.2.5),
> > it is a requirement of bitstream conformance that for at least one
> > reference frame has the valid dimensions.
> >
> > Modify the check to make sure the decoder works well in
> SINGLE_REFERENCE
> > mode that not all reference frames have valid dimensions.
> >
> > Check and error out if invalid reference frame is used in inter_recon.
> >
> > One of the failure case is a 480x272 inter frame (SINGLE_REFERENCE mode)
> > with following reference pool:
> >
> > 0.  960x544LASTvalid
> > 1. 1920x1088 GOLDEN  invalid, but not used in single reference mode
> > 2. 1920x1088 ALTREF  invalid, but not used in single reference mode
> > 3~7  ... Unused
> >
> > Identical logic in libvpx:
> >
> <https://github.com/webmproject/libvpx/blob/master/vp9/decoder/vp9_d
> ecodeframe.c#L736>
> >
> > Signed-off-by: Linjie Fu mailto:linjie...@intel.com>>
> > ---
> > [v3]: replace assert with check/return, tested in both multi frame/slice
> mode
> > [v4]: clear error_info to make decoding still work for other frames in this
> stream
> >
> > libavcodec/vp9.c  | 20 ++--
> > libavcodec/vp9dec.h   |  5 +
> > libavcodec/vp9recon.c | 10 ++
> > 3 files changed, 33 insertions(+), 2 deletions(-)
> >
> > LGTM, thanks for the revisions. (We have been discussing this on IRC.)
> 
> Thanks for the review and valuable suggestions.
> 
Ping for merge, thx.

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] ffmpeg: Don't require a known device to pass a frames context to an encoder

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of
> Mark Thompson
> Sent: Wednesday, April 29, 2020 06:57
> To: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Subject: [FFmpeg-devel] [PATCH] ffmpeg: Don't require a known device to
> pass a frames context to an encoder
> 
> The previous version here did not handle passing a frames context when
> ffmpeg itself did not know about the device it came from (for example,
> because it was created by device derivation inside a filter graph), which
> would break encoders requiring that input.  Fix that by checking for HW
> frames and device context methods independently, and prefer to use a
> frames context method if possible.  At the same time, revert the encoding
> additions to the device matching function because the additional
> complexity was not relevant to decoding.
> ---
>  fftools/ffmpeg_hw.c | 75 +
>  1 file changed, 42 insertions(+), 33 deletions(-)
> diff --git a/fftools/ffmpeg_hw.c b/fftools/ffmpeg_hw.c
> index c5c8aa97ef..fc4a5d31d6 100644
> --- a/fftools/ffmpeg_hw.c
> +++ b/fftools/ffmpeg_hw.c
> @@ -19,6 +19,7 @@
>  #include 
> 
>  #include "libavutil/avstring.h"
> +#include "libavutil/pixdesc.h"
>  #include "libavfilter/buffersink.h"
> 
>  #include "ffmpeg.h"
> @@ -282,10 +283,7 @@ void hw_device_free_all(void)
>  nb_hw_devices = 0;
>  }
> 
> -static HWDevice *hw_device_match_by_codec(const AVCodec *codec,
> -  enum AVPixelFormat format,
> -  int possible_methods,
> -  int *matched_methods)
> +static HWDevice *hw_device_match_by_codec(const AVCodec *codec)
>  {
>  const AVCodecHWConfig *config;
>  HWDevice *dev;
> @@ -294,18 +292,11 @@ static HWDevice
> *hw_device_match_by_codec(const AVCodec *codec,
>  config = avcodec_get_hw_config(codec, i);
>  if (!config)
>  return NULL;
> -if (format != AV_PIX_FMT_NONE &&
> -config->pix_fmt != AV_PIX_FMT_NONE &&
> -config->pix_fmt != format)
> -continue;
> -if (!(config->methods & possible_methods))
> +if (!(config->methods &
> AV_CODEC_HW_CONFIG_METHOD_HW_DEVICE_CTX))
>  continue;
>  dev = hw_device_get_by_type(config->device_type);
> -if (dev) {
> -if (matched_methods)
> -*matched_methods = config->methods & possible_methods;
> +if (dev)
>  return dev;
> -}
>  }
>  }
> 
> @@ -351,9 +342,7 @@ int hw_device_setup_for_decode(InputStream *ist)
>  if (!dev)
>  err = hw_device_init_from_type(type, NULL, );
>  } else {
> -dev = hw_device_match_by_codec(ist->dec, AV_PIX_FMT_NONE,
> -   
> AV_CODEC_HW_CONFIG_METHOD_HW_DEVICE_CTX,
> -   NULL);
> +dev = hw_device_match_by_codec(ist->dec);
>  if (!dev) {
>  // No device for this codec, but not using generic hwaccel
>  // and therefore may well not need one - ignore.
> @@ -429,37 +418,57 @@ int hw_device_setup_for_decode(InputStream
> *ist)
> 
>  int hw_device_setup_for_encode(OutputStream *ost)
>  {
> -HWDevice *dev;
> -AVBufferRef *frames_ref;
> -int methods = AV_CODEC_HW_CONFIG_METHOD_HW_DEVICE_CTX;
> -int matched_methods;
> +const AVCodecHWConfig *config;
> +HWDevice *dev = NULL;
> +AVBufferRef *frames_ref = NULL;
> +int i;
> 
>  if (ost->filter) {
>  frames_ref = av_buffersink_get_hw_frames_ctx(ost->filter->filter);
>  if (frames_ref &&
>  ((AVHWFramesContext*)frames_ref->data)->format ==
> -ost->enc_ctx->pix_fmt)
> -methods |= AV_CODEC_HW_CONFIG_METHOD_HW_FRAMES_CTX;
> +ost->enc_ctx->pix_fmt) {
> +// Matching format, will try to use hw_frames_ctx.
> +} else {
> +frames_ref = NULL;
> +}
>  }
> 
> -dev = hw_device_match_by_codec(ost->enc, ost->enc_ctx->pix_fmt,
> -   methods, _methods);
> -if (dev) {
> -if (matched_methods &
> AV_CODEC_HW_CONFIG_METHOD_HW_DEVICE_CTX) {
> -ost->enc_ctx->hw_device_ctx = av_buffer_ref(dev->device_ref);
> -if (!ost->enc_ctx->hw_device_ctx)
> -return AVERROR(ENOMEM);
> -}
> -if (matched_methods &
> AV_CODEC_HW_CONFIG_METHOD_HW_FRAMES_CTX) {
> +for (i = 0;; i++) {
> +config = avcodec_get_hw_config(ost->enc, i);
> +if (!config)
> +break;
> +
> +if (frames_ref &&
> +config->methods &
> AV_CODEC_HW_CONFIG_METHOD_HW_FRAMES_CTX &&
> +(config->pix_fmt == AV_PIX_FMT_NONE ||
> + config->pix_fmt == ost->enc_ctx->pix_fmt)) {
> +av_log(ost->enc_ctx, AV_LOG_VERBOSE, "Using input "
> +   

Re: [FFmpeg-devel] [PATCH v5 3/5] lavc/libopenh264enc: add bit rate control select support

[Fu, Linjie]

> From: Martin Storsjö 
> Sent: Wednesday, April 29, 2020 03:18
> To: Fu, Linjie 
> Cc: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Subject: RE: [FFmpeg-devel] [PATCH v5 3/5] lavc/libopenh264enc: add bit
> rate control select support
> 
> On Tue, 28 Apr 2020, Fu, Linjie wrote:
> 
> >> From: Martin Storsjö 
> >> Sent: Tuesday, April 28, 2020 18:31
> >> To: FFmpeg development discussions and patches  >> de...@ffmpeg.org>
> >> Cc: Fu, Linjie 
> >> Subject: Re: [FFmpeg-devel] [PATCH v5 3/5] lavc/libopenh264enc: add bit
> >> rate control select support
> >>
> >> On Tue, 28 Apr 2020, Linjie Fu wrote:
> >>
> >> We don't need checks for 1.2 - the initial version of openh264 that we
> >> support is 1.3, so we only need checks for 1.4 and newer.
> >
> > Ahh, didn't noticed this until checked the commit message of
> > 8a3d9ca603f4d15ecaa9ca379cbaab4ecaec8ce4.
> >
> > IMHO it seems to be better if we add this version check in the configure as
> well.
> > (Did a quick verification with version modified in pkgconfig to 1.1.0,
> configure is
> > still good for libopenh264 )
> 
> We don't need an explicit version check for 1.3 in configure - we require
> that WelsGetCodecVersion can be found in the configure check, and that
> function was added in 1.3 (explicitly for the purpose so that we can check
> that the version that we have linked is the same as we compiled against).
> 

Checked and confirmed that you are right, WelsGetCodecVersion is enough
for  1.3.0 version check, thanks for elaborations.

> >
> >>> +#if OPENH264_VER_AT_LEAST(1, 4)
> >>> +{ "timestamp", "bit rate control based on timestamp",
> >> 0, AV_OPT_TYPE_CONST, { .i64 = RC_TIMESTAMP_MODE },   0, 0, VE,
> >> "rc_mode" },
> >>> +#endif
> >>> +
> >>> { NULL }
> >>> };
> >>
> >> This commit seems to lack something that actually sets the rc_mode value
> >> in the parameters struct though - wasn't that part of the previous version
> >> of the patch?
> >>
> > Did you mean setting rc_mode through parameters like bit_rate/qp?
> > This patch enables explicitly setting the rc_mode as part of that logic.
> 
> No, I didn't mean that.
> 
> The previous version of this patch had the following change:
> 
> -param.iRCMode= RC_QUALITY_MODE;
> +param.iRCMode= s->rc_mode;
> 
> Now I don't find that part any longer in this patch - and without that
> change, the rest of the commit is pretty pointless, no?

it's somehow missed while rebasing the set, thanks.

- Linjie

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v3] libavcodec/libx264: fix reference frame computation based on level

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of
> Josh de Kock
> Sent: Tuesday, April 28, 2020 23:47
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v3] libavcodec/libx264: fix reference
> frame computation based on level
> 
> On 26/04/2020 12:46, Josh Brewster wrote:
>  I only made sure that the level was positive because its initial
>  value was -1.
> 
> > else if (x4->params.i_level_idc >= 0) {
> > Let me know if I need to reject 0 too. It seemed like premature
> optimization
> > as the level simply wouldn't be present in x264_levels.
> >>>
> >>> I'd say yes, level_idc = 0 is possible but invalid by PARSE_X264_OPT(),
> which seems
> >>> make no sense to calculate refs from x264_levels[] table.
> >>>
> >>> -   Linjie
> >>
> >> Changed to > 0, thanks.
> >
> > Hi, is there anything else I need to do to have this merged?
> 
>  From a precursory look at what x264 and we're doing here your patch is
> correct. It doesn't break from a quick test, and looks OK to me. Would
> rather someone else has a look at it too but I will again in a couple
> days if no one does.
> 
Should be ok IMHO, thx.

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v5 1/5] lavc/libopenh264enc: Add qmin/qmax support

[Fu, Linjie]

> From: Martin Storsjö 
> Sent: Tuesday, April 28, 2020 18:28
> To: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Cc: Fu, Linjie 
> Subject: Re: [FFmpeg-devel] [PATCH v5 1/5] lavc/libopenh264enc: Add
> qmin/qmax support
> 
> On Tue, 28 Apr 2020, Linjie Fu wrote:
> 
> > Clip iMinQp/iMaxQp to (1, 51) if user specified qp range
> > explicitly since QP = 0 is not well supported currently
> > in libopenh264, otherwise leave iMinQp/iMaxQp untouched
> > and use the values initialized in FillDefault().
> 
> I'd suggest changing the commit message. It's not that "QP = 0 is not well
> supported". Setting QP=0 means "use the defaults", and that's an intended
> usecase. It's unfortunate that the library logs a warning message in this
> case though.
> 
> Can you make a PR to openh264 to change the verbosity level of that
> message, from warning to info?

PR filed in:
https://github.com/cisco/openh264/pull/3278

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v5 2/5] lavc/libopenh264enc: add default gop size and bit rate

[Fu, Linjie]

> From: Martin Storsjö 
> Sent: Tuesday, April 28, 2020 18:29
> To: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Cc: Fu, Linjie 
> Subject: Re: [FFmpeg-devel] [PATCH v5 2/5] lavc/libopenh264enc: add
> default gop size and bit rate
> 
> On Tue, 28 Apr 2020, Linjie Fu wrote:
> 
> > It would be 200kbps bitrate with gop size = 12 by default
> > which generated too many IDR frames in rather low bit rate.
> > The quality would be poor.
> >
> > Set these default values to -1 to check whether it's specified
> > by user explicitly.
> >
> > Use 2Mbps bitrate as nvenc sugguested, and leave gop size
> > untouched in libopenh264.
> >
> > Signed-off-by: Linjie Fu 
> > ---
> > libavcodec/libopenh264enc.c | 9 +++--
> > 1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c
> > index 1b6b53a..0951412 100644
> > --- a/libavcodec/libopenh264enc.c
> > +++ b/libavcodec/libopenh264enc.c
> > @@ -37,6 +37,8 @@
> > #define SM_SIZELIMITED_SLICE SM_DYN_SLICE
> > #endif
> >
> > +#define TARGET_BITRATE_DEFAULT 2*1000*1000
> > +
> > typedef struct SVCContext {
> > const AVClass *av_class;
> > ISVCEncoder *encoder;
> > @@ -132,7 +134,7 @@ FF_ENABLE_DEPRECATION_WARNINGS
> > param.fMaxFrameRate  = 1/av_q2d(avctx->time_base);
> > param.iPicWidth  = avctx->width;
> > param.iPicHeight = avctx->height;
> > -param.iTargetBitrate = avctx->bit_rate;
> > +param.iTargetBitrate = avctx->bit_rate > 0 ? 
> > avctx->bit_rate :
> TARGET_BITRATE_DEFAULT;
> > param.iMaxBitrate= FFMAX(avctx->rc_max_rate, avctx-
> >bit_rate);
> > param.iRCMode= RC_QUALITY_MODE;
> > // QP = 0 is not well supported, so clip to (1, 51)
> > @@ -148,7 +150,8 @@ FF_ENABLE_DEPRECATION_WARNINGS
> > param.bEnableFrameSkip   = s->skip_frames;
> > param.bEnableLongTermReference   = 0;
> > param.iLtrMarkPeriod = 30;
> > -param.uiIntraPeriod  = avctx->gop_size;
> > +if (avctx->gop_size)
> > +param.uiIntraPeriod  = avctx->gop_size;
> 
> This should be gop_size > 0, or maybe >= 0. Otherwise we'll end up setting
> the default -1.
> 
Ops, fixed and thanks for pointing this out.

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v5 3/5] lavc/libopenh264enc: add bit rate control select support

[Fu, Linjie]

> From: Martin Storsjö 
> Sent: Tuesday, April 28, 2020 18:31
> To: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Cc: Fu, Linjie 
> Subject: Re: [FFmpeg-devel] [PATCH v5 3/5] lavc/libopenh264enc: add bit
> rate control select support
> 
> On Tue, 28 Apr 2020, Linjie Fu wrote:
> 
> > RC_BITRATE_MODE:
> >set BITS_EXCEEDED to iCurrentBitsLevel and allows QP adjust
> >in RcCalculatePictureQp().
> >
> > RC_BUFFERBASED_MODE:
> >use buffer status to adjust the video quality, introduced in
> >release 1.2.
> >
> > RC_TIMESTAMP_MODE:
> >bit rate control based on timestamp, introduced in release 1.4.
> >
> > Default to use RC_QUALITY_MODE.
> >
> > Signed-off-by: Linjie Fu 
> > ---
> > libavcodec/libopenh264enc.c | 15 +++
> > 1 file changed, 15 insertions(+)
> >
> > diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c
> > index 0951412..93d3de2 100644
> > --- a/libavcodec/libopenh264enc.c
> > +++ b/libavcodec/libopenh264enc.c
> > @@ -49,6 +49,9 @@ typedef struct SVCContext {
> > int skip_frames;
> > int skipped;
> > int cabac;
> > +
> > +// rate control mode
> > +int rc_mode;
> > } SVCContext;
> >
> > #define OFFSET(x) offsetof(SVCContext, x)
> > @@ -73,6 +76,18 @@ static const AVOption options[] = {
> > { "max_nal_size", "set maximum NAL size in bytes",
> OFFSET(max_nal_size), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, VE },
> > { "allow_skip_frames", "allow skipping frames to hit the target 
> > bitrate",
> OFFSET(skip_frames), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, VE },
> > { "cabac", "Enable cabac", OFFSET(cabac), AV_OPT_TYPE_INT, { .i64 = 0 },
> 0, 1, VE },
> > +
> > +{ "rc_mode", "Select rate control mode", OFFSET(rc_mode),
> AV_OPT_TYPE_INT, { .i64 = RC_QUALITY_MODE }, RC_OFF_MODE,
> RC_TIMESTAMP_MODE, VE, "rc_mode" },
> > +{ "off",   "bit rate control off", 
> > 0,
> AV_OPT_TYPE_CONST, { .i64 = RC_OFF_MODE }, 0, 0, VE, "rc_mode" },
> > +{ "quality",   "quality mode", 
> > 0,
> AV_OPT_TYPE_CONST, { .i64 = RC_QUALITY_MODE }, 0, 0, VE, "rc_mode" },
> > +{ "bitrate",   "bitrate mode", 
> > 0,
> AV_OPT_TYPE_CONST, { .i64 = RC_BITRATE_MODE }, 0, 0, VE, "rc_mode" },
> > +#if OPENH264_VER_AT_LEAST(1, 2)
> > +{ "buffer","using buffer status to adjust the video quality 
> > (no bitrate
> control)", 0, AV_OPT_TYPE_CONST, { .i64 = RC_BUFFERBASED_MODE }, 0, 0,
> VE, "rc_mode" },
> > +#endif
> 
> We don't need checks for 1.2 - the initial version of openh264 that we
> support is 1.3, so we only need checks for 1.4 and newer.

Ahh, didn't noticed this until checked the commit message of
8a3d9ca603f4d15ecaa9ca379cbaab4ecaec8ce4.

IMHO it seems to be better if we add this version check in the configure as 
well.
(Did a quick verification with version modified in pkgconfig to 1.1.0, 
configure is
still good for libopenh264 )

Will send another fix for this if no against.
 
> > +#if OPENH264_VER_AT_LEAST(1, 4)
> > +{ "timestamp", "bit rate control based on timestamp",
> 0, AV_OPT_TYPE_CONST, { .i64 = RC_TIMESTAMP_MODE },   0, 0, VE,
> "rc_mode" },
> > +#endif
> > +
> > { NULL }
> > };
> 
> This commit seems to lack something that actually sets the rc_mode value
> in the parameters struct though - wasn't that part of the previous version
> of the patch?
> 
Did you mean setting rc_mode through parameters like bit_rate/qp?
This patch enables explicitly setting the rc_mode as part of that logic. 

As to full enhancement, it's WIP and I'm still considering some parameters
like -qp to specify exact QP. (libopenh264 seems to only accept iMax/MinQp
in RC_QUALITY_MODE) For specific QP settings like -qp 26, we have to select
RC_OFF_MODE and specify QP by iDLayerQp in sSpatialLayers, which seems
a little bit confusing. (setting -qp 26 which leads to a non-quality mode)

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v4 2/9] lavc/libopenh264enc: add default gop size and bit rate

[Fu, Linjie]

> From: Martin Storsjö 
> Sent: Tuesday, April 28, 2020 16:08
> To: Fu, Linjie 
> Cc: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Subject: RE: [FFmpeg-devel] [PATCH v4 2/9] lavc/libopenh264enc: add
> default gop size and bit rate
> 
> On Tue, 28 Apr 2020, Fu, Linjie wrote:
> 
> >> From: Martin Storsjö 
> >> Sent: Tuesday, April 28, 2020 14:28
> >> To: Fu, Linjie 
> >> Cc: FFmpeg development discussions and patches  >> de...@ffmpeg.org>
> >> Subject: RE: [FFmpeg-devel] [PATCH v4 2/9] lavc/libopenh264enc: add
> >> default gop size and bit rate
> >>
> >> On Tue, 28 Apr 2020, Fu, Linjie wrote:
> >>
> >>>> From: Martin Storsjö 
> >>>> Sent: Tuesday, April 28, 2020 03:28
> >>>>> static const AVCodecDefault svc_enc_defaults[] = {
> >>>>> +{ "b", "0" },
> >>>>> +{ "g", "120"   },
> >>>>> { "qmin",  "-1"},
> >>>>
> >>>> Why do you hardcode a value for g here, but put the default bitrate
> value
> >>>> in the code above? Wouldn't it be clearer to have both defaults here at
> >>>> the same place?
> >>>>
> >>> A default value in svc_enc_defaults[] would help to distinguish between
> >>> "the user specified the bitrate to be " vs. "the user did not specify
> >> anything
> >>> about the target bitrate", as Anton has suggested in [1].
> >>>
> >>> Considering about your suggestions in patch 1/9, IMO it seems to be
> more
> >> reasonable
> >>> to keep the uiIntraPeriod untouched. The libopenh264 library would fill
> the
> >> default
> >>> value of uiIntraPeriod to 0, and as a consequence the gop size would be
> >> rather large.
> >>>
> >>> Updated the default "g" to "-1", same as libx264 did.
> >>> (Note that it's not acceptable for bitrate, since bitrate = 0 as default 
> >>> in
> >> library is not
> >>> valid)
> >>
> >> If we have the option of not setting the bitrate on the encoder, then yes,
> >> it's better to avoid setting one in svc_enc_defaults. But in this case, if
> >> no bitrate was chosen by the user, we end up enforcing a default value
> >> anyway - just that the default is not written in the global defaults table
> >> (libavcodec/options_table.h) and not in svc_enc_defaults, but instead in
> >> code.
> >>
> >> If we actually need to set a bitrate, it's IMO better to put this default
> >
> > Indeed we have to set the bitrate.
> >
> >> in the table, especially if we have other defaults like gop size there.
> >>
> >
> > In previous discussion in [1], we seems to come to an alignment that this
> would
> > help to determine whether it's specified by user, and hence allow
> libopenh264enc
> > to select the RC_MODE through settings like avctx->bit_rate, max/min/avg
> bitrates
> > if rc_mode is not set explicitly.
> >
> > VAAPI encoder has the similar logic in [2].
> >
> > I'm okay with either solution, if above logic is not going to be 
> > implemented.
> 
> Right, so if logic that selects rc mode based on those settings will be
> implemented, then yes it makes sense to keep the default at -1 and fall
> back to a default bitrate within code. In that case I'd kind of prefer to
> have the default bitrate specified in a define high up in the file,
> instead of buried in init logic though.
> 
> // Martin

Updated.
Separated the patch set and resend, thanks for the suggestions.

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v4 6/9] lavc/libopenh264enc: separate svc_encode_init() into several functions

[Fu, Linjie]

> From: Martin Storsjö 
> Sent: Tuesday, April 28, 2020 04:02
> To: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Cc: Fu, Linjie 
> Subject: Re: [FFmpeg-devel] [PATCH v4 6/9] lavc/libopenh264enc: separate
> svc_encode_init() into several functions
> 
> On Wed, 15 Apr 2020, Linjie Fu wrote:
> 
> > Separate the initialization procedure into different functions.
> >
> > Make it more readable and easier to be extended.
> >
> > Signed-off-by: Linjie Fu 
> > ---
> > libavcodec/libopenh264enc.c | 283 +++-
> 
> > 1 file changed, 174 insertions(+), 109 deletions(-)
> 
> I honestly don't see the point here. Yes, the init function is long, but
> you're moving code around and making it even 65 lines longer in total...
> 
> In which way is this easier to extend than the previous form?

This patch simply adds some wrappers for the existed code, hence it's true
it'll be longer.

IMO a hierarchical function structure seems to be more clear and readable,
hence easier for developer and user. 

However I don't have much strong intention for this. If it's not a good idea,
I'll separate this patch set and get the functional patches merged firstly, thx.

- Linjie



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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v4 3/9] lavc/libopenh264enc: add bit rate control select support

[Fu, Linjie]

> From: Martin Storsjö 
> Sent: Tuesday, April 28, 2020 03:35
> To: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Cc: Fu, Linjie 
> Subject: Re: [FFmpeg-devel] [PATCH v4 3/9] lavc/libopenh264enc: add bit
> rate control select support
> 
> On Wed, 15 Apr 2020, Linjie Fu wrote:
> 
> > RC_BITRATE_MODE:
> >set BITS_EXCEEDED to iCurrentBitsLevel and allows QP adjust
> >in RcCalculatePictureQp().
> >
> > RC_BUFFERBASED_MODE:
> >use buffer status to adjust the video quality.
> >
> > RC_TIMESTAMP_MODE:
> >bit rate control based on timestamp.
> >
> > Default to use RC_QUALITY_MODE.
> >
> > Signed-off-by: Linjie Fu 
> > ---
> > libavcodec/libopenh264enc.c | 12 +++-
> > 1 file changed, 11 insertions(+), 1 deletion(-)
> 
> This looks ok, but I don't think all of these have been available since
> the beginning. We do support building with a few older versions of the
> library, so I think at least RC_TIMESTAMP_MODE appeared later. The lowest
> version that was supported originally was OpenH264 1.3, so for things that
> have appeared later, please add ifdefs (or consistently bump the minimum
> version somewhere and remove redundant checks for lower versions).
> 

Double checked this in the release history:
RC_BUFFERBASED_MODE was introduced since release 1.2;
RC_TIMESTAMP_MODE was introduced since release 1.4.

Updated.

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v4 2/9] lavc/libopenh264enc: add default gop size and bit rate

[Fu, Linjie]

> From: Martin Storsjö 
> Sent: Tuesday, April 28, 2020 14:28
> To: Fu, Linjie 
> Cc: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Subject: RE: [FFmpeg-devel] [PATCH v4 2/9] lavc/libopenh264enc: add
> default gop size and bit rate
> 
> On Tue, 28 Apr 2020, Fu, Linjie wrote:
> 
> >> From: Martin Storsjö 
> >> Sent: Tuesday, April 28, 2020 03:28
> >>> static const AVCodecDefault svc_enc_defaults[] = {
> >>> +{ "b", "0" },
> >>> +{ "g", "120"   },
> >>> { "qmin",  "-1"},
> >>
> >> Why do you hardcode a value for g here, but put the default bitrate value
> >> in the code above? Wouldn't it be clearer to have both defaults here at
> >> the same place?
> >>
> > A default value in svc_enc_defaults[] would help to distinguish between
> > "the user specified the bitrate to be " vs. "the user did not specify
> anything
> > about the target bitrate", as Anton has suggested in [1].
> >
> > Considering about your suggestions in patch 1/9, IMO it seems to be more
> reasonable
> > to keep the uiIntraPeriod untouched. The libopenh264 library would fill the
> default
> > value of uiIntraPeriod to 0, and as a consequence the gop size would be
> rather large.
> >
> > Updated the default "g" to "-1", same as libx264 did.
> > (Note that it's not acceptable for bitrate, since bitrate = 0 as default in
> library is not
> > valid)
> 
> If we have the option of not setting the bitrate on the encoder, then yes,
> it's better to avoid setting one in svc_enc_defaults. But in this case, if
> no bitrate was chosen by the user, we end up enforcing a default value
> anyway - just that the default is not written in the global defaults table
> (libavcodec/options_table.h) and not in svc_enc_defaults, but instead in
> code.
> 
> If we actually need to set a bitrate, it's IMO better to put this default

Indeed we have to set the bitrate.

> in the table, especially if we have other defaults like gop size there.
> 

In previous discussion in [1], we seems to come to an alignment that this would
help to determine whether it's specified by user, and hence allow libopenh264enc
to select the RC_MODE through settings like avctx->bit_rate, max/min/avg 
bitrates
if rc_mode is not set explicitly.

VAAPI encoder has the similar logic in [2].

I'm okay with either solution, if above logic is not going to be implemented.

[1] https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2020-April/260215.html
[2] https://github.com/FFmpeg/FFmpeg/blob/master/libavcodec/vaapi_encode.c#L1542

- Linjie


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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v4 1/9] lavc/libopenh264enc: Add default qmin/qmax support

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of
> Martin Storsjö
> Sent: Tuesday, April 28, 2020 14:31
> To: Fu, Linjie 
> Cc: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH v4 1/9] lavc/libopenh264enc: Add
> default qmin/qmax support
> 
> On Tue, 28 Apr 2020, Fu, Linjie wrote:
> 
> >> From: Martin Storsjö 
> >> Sent: Tuesday, April 28, 2020 14:19
> >> To: Fu, Linjie 
> >> Cc: FFmpeg development discussions and patches  >> de...@ffmpeg.org>
> >> Subject: RE: [FFmpeg-devel] [PATCH v4 1/9] lavc/libopenh264enc: Add
> >> default qmin/qmax support
> >>
> >> On Tue, 28 Apr 2020, Fu, Linjie wrote:
> >>
> >>>> From: Martin Storsjö 
> >>>> Sent: Tuesday, April 28, 2020 03:27
> >>>>
> >>>> If qmax/qmin < 0, i.e. wasn't specified by the user, wouldn't it be
> better
> >>>> to not touch param.iMax/MinQp at all (and use the default value of the
> >>>> library, which may change between versions), instead of overriding it
> with
> >>>> a value hardcoded here?
> >>>>
> >>> Okay, this seems more natural if the recommended QP range varies
> >> between
> >>> versions, though one of my original purposes is to avoid the warning in
> >> default
> >>> situation for changing the QP inside libopenh264 library.
> >>
> >> Well in general I'd want to avoid hardcoding opinionated defaults within
> >> our own wrapper - I'd like it to behave as close to what upstream
> >> intended, so that whatever issues we see with defaults, are the same
> >> issues that everyone else sees as well, so any fixes to those defaults
> >> upstream also end up for us - so we don't get stuck on whatever we
> thought
> >> was a good default at some point.
> >
> > Agree, this makes more sense.
> >
> >> What warnings about changing QP are you referring to?
> > Warning:Change QP Range from(0,51) to (12,42)
> >
> https://github.com/cisco/openh264/blob/master/codec/encoder/core/src/e
> ncoder_ext.cpp#L375
> >
> > The main reason is that "0" is not supported well, which is the default
> > value of iMinQp.
> 
> Ah, right.
> 
> In that case, setting some other default might make sense indeed.
> How/where does OpenH264 set suitable values for this, in order not to get
> the warnings everywhere? Are the values e.g. 12-42 hardcoded everywhere
> in
> every caller?
> 
IIRC, it depends on the iUsageType, and hardcoded in the libopenh264 library 
function
ParamValidation() in [1]:
For SCREEN_CONTENT_REAL_TIME,
it's (MIN_SCREEN_QP, MAX_SCREEN_QP), e.g. (26, 35);
For other usage like CAMERA_VIDEO_REAL_TIME by default,
It's (GOM_MIN_QP_MODE, MAX_LOW_BR_QP), e.g. (12, 42)

- Linjie

[1] 
https://github.com/cisco/openh264/blob/master/codec/encoder/core/src/encoder_ext.cpp#L379
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v4 1/9] lavc/libopenh264enc: Add default qmin/qmax support

[Fu, Linjie]

> From: Martin Storsjö 
> Sent: Tuesday, April 28, 2020 14:19
> To: Fu, Linjie 
> Cc: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Subject: RE: [FFmpeg-devel] [PATCH v4 1/9] lavc/libopenh264enc: Add
> default qmin/qmax support
> 
> On Tue, 28 Apr 2020, Fu, Linjie wrote:
> 
> >> From: Martin Storsjö 
> >> Sent: Tuesday, April 28, 2020 03:27
> >>
> >> If qmax/qmin < 0, i.e. wasn't specified by the user, wouldn't it be better
> >> to not touch param.iMax/MinQp at all (and use the default value of the
> >> library, which may change between versions), instead of overriding it with
> >> a value hardcoded here?
> >>
> > Okay, this seems more natural if the recommended QP range varies
> between
> > versions, though one of my original purposes is to avoid the warning in
> default
> > situation for changing the QP inside libopenh264 library.
> 
> Well in general I'd want to avoid hardcoding opinionated defaults within
> our own wrapper - I'd like it to behave as close to what upstream
> intended, so that whatever issues we see with defaults, are the same
> issues that everyone else sees as well, so any fixes to those defaults
> upstream also end up for us - so we don't get stuck on whatever we thought
> was a good default at some point.

Agree, this makes more sense.
 
> What warnings about changing QP are you referring to?
Warning:Change QP Range from(0,51) to (12,42) 
https://github.com/cisco/openh264/blob/master/codec/encoder/core/src/encoder_ext.cpp#L375
 

The main reason is that "0" is not supported well, which is the default value 
of iMinQp.

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v4 2/9] lavc/libopenh264enc: add default gop size and bit rate

[Fu, Linjie]

> From: Martin Storsjö 
> Sent: Tuesday, April 28, 2020 03:28
> > static const AVCodecDefault svc_enc_defaults[] = {
> > +{ "b", "0" },
> > +{ "g", "120"   },
> > { "qmin",  "-1"},
> 
> Why do you hardcode a value for g here, but put the default bitrate value
> in the code above? Wouldn't it be clearer to have both defaults here at
> the same place?
> 
A default value in svc_enc_defaults[] would help to distinguish between
"the user specified the bitrate to be " vs. "the user did not specify 
anything
about the target bitrate", as Anton has suggested in [1].

Considering about your suggestions in patch 1/9, IMO it seems to be more 
reasonable
to keep the uiIntraPeriod untouched. The libopenh264 library would fill the 
default
value of uiIntraPeriod to 0, and as a consequence the gop size would be rather 
large.

Updated the default "g" to "-1", same as libx264 did.
(Note that it's not acceptable for bitrate, since bitrate = 0 as default in 
library is not
valid)

- Linjie

[1] https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2020-April/260320.html

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v4 1/9] lavc/libopenh264enc: Add default qmin/qmax support

[Fu, Linjie]

> From: Martin Storsjö 
> Sent: Tuesday, April 28, 2020 03:27
> 
> If qmax/qmin < 0, i.e. wasn't specified by the user, wouldn't it be better
> to not touch param.iMax/MinQp at all (and use the default value of the
> library, which may change between versions), instead of overriding it with
> a value hardcoded here?
> 
Okay, this seems more natural if the recommended QP range varies between
versions, though one of my original purposes is to avoid the warning in default
situation for changing the QP inside libopenh264 library.

Updated locally as suggested, and thanks for the review for the whole set.

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v4 0/9] Enhancement for libopenh264 encoder

[Fu, Linjie]

Hi,

> From: Fu, Linjie 
> Sent: Wednesday, April 15, 2020 12:54
> To: ffmpeg-devel@ffmpeg.org
> Cc: Fu, Linjie 
> Subject: [PATCH v4 0/9] Enhancement for libopenh264 encoder
> 
> Docs would be provided later.
> 
> Linjie Fu (9):
>   lavc/libopenh264enc: Add default qmin/qmax support
>   lavc/libopenh264enc: add default gop size and bit rate
>   lavc/libopenh264enc: add bit rate control select support
>   lavc/libopenh264enc: prompt slice number changing inside libopenh264
>   lavc/libopenh264enc: set slice_mode option to deprecated
> [v4]: move FF_API_OPENH264_SLICE_MODE to libavcodec/version.h
>   lavc/libopenh264enc: separate svc_encode_init() into several functions
>   lavc/libopenh264enc: add profile high option support
>   lavc/libopenh264enc: allow specifying the profile through
> AVCodecContext
>   lavc/libopenh264enc: Add coder option to replace cabac
> [v4]: move FF_API_OPENH264_CABAC to libavcodec/version.h
> 
>  libavcodec/libopenh264enc.c | 365 ++--
> 
>  libavcodec/version.h|   6 +
>  2 files changed, 259 insertions(+), 112 deletions(-)
> 
> --
> 2.7.4
Ping for this set.

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] lavc/vaapi_encode: add FF_CODEC_CAP_INIT_CLEANUP caps for encoders

[Fu, Linjie]

> From: Fu, Linjie 
> Sent: Tuesday, March 31, 2020 23:34
> To: ffmpeg-devel@ffmpeg.org
> Cc: Fu, Linjie 
> Subject: [PATCH] lavc/vaapi_encode: add FF_CODEC_CAP_INIT_CLEANUP
> caps for encoders
> 
> ff_vaapi_encode_close() is not enough to free the resources like cbs
> if initialization failure happens after codec->configure (except for
> vp8/vp9).
> 
> We need to call avctx->codec->close() to deallocate, otherwise memory
> leak happens.
> 
> Add FF_CODEC_CAP_INIT_CLEANUP for vaapi encoders and deallocate the
> resources at free_and_end inside avcodec_open2().
> 
> Signed-off-by: Linjie Fu 
> ---
>  libavcodec/vaapi_encode.c   | 1 -
>  libavcodec/vaapi_encode_h264.c  | 1 +
>  libavcodec/vaapi_encode_h265.c  | 1 +
>  libavcodec/vaapi_encode_mjpeg.c | 1 +
>  libavcodec/vaapi_encode_mpeg2.c | 1 +
>  libavcodec/vaapi_encode_vp8.c   | 1 +
>  libavcodec/vaapi_encode_vp9.c   | 1 +
>  7 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/libavcodec/vaapi_encode.c b/libavcodec/vaapi_encode.c
> index 8ff720e..d371033 100644
> --- a/libavcodec/vaapi_encode.c
> +++ b/libavcodec/vaapi_encode.c
> @@ -2361,7 +2361,6 @@ av_cold int ff_vaapi_encode_init(AVCodecContext
> *avctx)
>  return 0;
> 
>  fail:
> -ff_vaapi_encode_close(avctx);
>  return err;
>  }
> 
> diff --git a/libavcodec/vaapi_encode_h264.c
> b/libavcodec/vaapi_encode_h264.c
> index f4965d8..6a86905 100644
> --- a/libavcodec/vaapi_encode_h264.c
> +++ b/libavcodec/vaapi_encode_h264.c
> @@ -1356,6 +1356,7 @@ AVCodec ff_h264_vaapi_encoder = {
>  .close  = _encode_h264_close,
>  .priv_class = _encode_h264_class,
>  .capabilities   = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_HARDWARE,
> +.caps_internal  = FF_CODEC_CAP_INIT_CLEANUP,
>  .defaults   = vaapi_encode_h264_defaults,
>  .pix_fmts = (const enum AVPixelFormat[]) {
>  AV_PIX_FMT_VAAPI,
> diff --git a/libavcodec/vaapi_encode_h265.c
> b/libavcodec/vaapi_encode_h265.c
> index 97dc5a7..4c24f7c 100644
> --- a/libavcodec/vaapi_encode_h265.c
> +++ b/libavcodec/vaapi_encode_h265.c
> @@ -1292,6 +1292,7 @@ AVCodec ff_hevc_vaapi_encoder = {
>  .close  = _encode_h265_close,
>  .priv_class = _encode_h265_class,
>  .capabilities   = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_HARDWARE,
> +.caps_internal  = FF_CODEC_CAP_INIT_CLEANUP,
>  .defaults   = vaapi_encode_h265_defaults,
>  .pix_fmts = (const enum AVPixelFormat[]) {
>  AV_PIX_FMT_VAAPI,
> diff --git a/libavcodec/vaapi_encode_mjpeg.c
> b/libavcodec/vaapi_encode_mjpeg.c
> index bd029cc..c469e70 100644
> --- a/libavcodec/vaapi_encode_mjpeg.c
> +++ b/libavcodec/vaapi_encode_mjpeg.c
> @@ -565,6 +565,7 @@ AVCodec ff_mjpeg_vaapi_encoder = {
>  .priv_class = _encode_mjpeg_class,
>  .capabilities   = AV_CODEC_CAP_HARDWARE |
>AV_CODEC_CAP_INTRA_ONLY,
> +.caps_internal  = FF_CODEC_CAP_INIT_CLEANUP,
>  .defaults   = vaapi_encode_mjpeg_defaults,
>  .pix_fmts = (const enum AVPixelFormat[]) {
>  AV_PIX_FMT_VAAPI,
> diff --git a/libavcodec/vaapi_encode_mpeg2.c
> b/libavcodec/vaapi_encode_mpeg2.c
> index bac9ea1..55f9289 100644
> --- a/libavcodec/vaapi_encode_mpeg2.c
> +++ b/libavcodec/vaapi_encode_mpeg2.c
> @@ -702,6 +702,7 @@ AVCodec ff_mpeg2_vaapi_encoder = {
>  .close  = _encode_mpeg2_close,
>  .priv_class = _encode_mpeg2_class,
>  .capabilities   = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_HARDWARE,
> +.caps_internal  = FF_CODEC_CAP_INIT_CLEANUP,
>  .defaults   = vaapi_encode_mpeg2_defaults,
>  .pix_fmts = (const enum AVPixelFormat[]) {
>  AV_PIX_FMT_VAAPI,
> diff --git a/libavcodec/vaapi_encode_vp8.c
> b/libavcodec/vaapi_encode_vp8.c
> index 6e7bf9d..d8fb031 100644
> --- a/libavcodec/vaapi_encode_vp8.c
> +++ b/libavcodec/vaapi_encode_vp8.c
> @@ -257,6 +257,7 @@ AVCodec ff_vp8_vaapi_encoder = {
>  .close  = _vaapi_encode_close,
>  .priv_class = _encode_vp8_class,
>  .capabilities   = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_HARDWARE,
> +.caps_internal  = FF_CODEC_CAP_INIT_CLEANUP,
>  .defaults   = vaapi_encode_vp8_defaults,
>  .pix_fmts = (const enum AVPixelFormat[]) {
>  AV_PIX_FMT_VAAPI,
> diff --git a/libavcodec/vaapi_encode_vp9.c
> b/libavcodec/vaapi_encode_vp9.c
> index d7f415d..ea19470 100644
> --- a/libavcodec/vaapi_encode_vp9.c
> +++ b/libavcodec/vaapi_encode_vp9.c
> @@ -291,6 +291,7 @@ AVCodec ff_vp9_vaapi_encoder = {
>  .close  = _vaapi_encode_close,
>  .priv_class = _encode_vp9_class,
>  .capabilities   = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_HARDWARE,
&g

Re: [FFmpeg-devel] [PATCH] lavfi/vf_vpp_qsv: fix the infinite loop while framerate lower than input

[Fu, Linjie]

> From: Zhong Li 
> Sent: Tuesday, April 21, 2020 17:10
> To: Fu, Linjie 
> Cc: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH] lavfi/vf_vpp_qsv: fix the infinite loop
> while framerate lower than input
> 
> > > I can't see the benefit to use MSDK framerate conversion function. Is
> > > it a good idea to drop it and use ffmpeg native fps filter instead?
> >
> > The implementation of FRC inside MSDK is quite straight-forward or simple
> > currently since it just drops or repeats frames, hence I think using native
> fps
> > filter is a good idea for decoding + FRC or FRC + encoding.
> >
> > However, for a pure hardware transcoding pipeline, there may be some
> > performance issues if inserting a software filter, extra memory copy would
> > be introduced in hwdownload/hwupload between system memory and
> video
> > memory, which would impact a lot for large resolutions.
> IIRC, it is not necessary to insert hwdownload/upload to use fps filter. No ？
Ahh, you are right, the native fps filter would be fair enough for this case.

Thanks for the elaborations.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] lavfi/vf_vpp_qsv: fix the infinite loop while framerate lower than input

[Fu, Linjie]

> From: Zhong Li 
> Sent: Sunday, April 19, 2020 23:00
> To: Fu, Linjie 
> Cc: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH] lavfi/vf_vpp_qsv: fix the infinite loop
> while framerate lower than input
> 
> Fu, Linjie  于2020年2月29日周六 下午5:35写道：
> >
> > > -Original Message-
> > > From: Zhong Li 
> > > Sent: Saturday, February 29, 2020 13:14
> > > To: FFmpeg development discussions and patches  > > de...@ffmpeg.org>
> > > Cc: Fu, Linjie 
> > > Subject: Re: [FFmpeg-devel] [PATCH] lavfi/vf_vpp_qsv: fix the infinite
> loop
> > > while framerate lower than input
> > >
> > > Linjie Fu  于2020年2月28日周五 下午11:34写
> 道：
> > > >
> > > > There are frame droppings in frc while converting into a lower
> framerate,
> > > > and MSDK returns ERROR_MORE_DATA which should be ignored.
> > >
> > > Should be fixed in MSDK instead of working around in FFmpeg?
> >
> > MSDK made decision regarding frame rate conversion. If it's the framerate
> down case,
> > FRC would skip frame without producing an output [1], and request a new
> input frame.
> 
> I can't see the benefit to use MSDK framerate conversion function. Is
> it a good idea to drop it and use ffmpeg native fps filter instead?

The implementation of FRC inside MSDK is quite straight-forward or simple
currently since it just drops or repeats frames, hence I think using native fps
filter is a good idea for decoding + FRC or FRC + encoding.

However, for a pure hardware transcoding pipeline, there may be some
performance issues if inserting a software filter, extra memory copy would
be introduced in hwdownload/hwupload between system memory and video
memory, which would impact a lot for large resolutions.

Took a simple tests for 4K 10bit HEVC transcoding:

# inserting a fps filter in the transcoding: 20 fps
$ ffmpeg -hwaccel qsv -c:v hevc_qsv -i ../Exodus_UHD_HDR_Exodus_draft.mp4 -vf 
"hwdownload,format=p010le,fps=fps=60,hwupload=extra_hw_frames=40" -c:v hevc_qsv 
out.mp4

# using msdk framerate conversion: 33 fps
$ ffmpeg -hwaccel qsv -c:v hevc_qsv -i ../Exodus_UHD_HDR_Exodus_draft.mp4 -vf 
"vpp_qsv=framerate=60" -c:v hevc_qsv out.mp4

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v2] libavcodec/libx264: fix reference frame computation based on level

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of
> Josh Brewster
> Hi Linjie, thanks for the feedback. I have changed the patch to use the right
> parameter. I only made sure that the level was positive because its initial
> value was -1.
> > else if (x4->params.i_level_idc >= 0) {
> Let me know if I need to reject 0 too. It seemed like premature optimization
> as the level simply wouldn't be present in x264_levels.
I'd say yes, level_idc = 0 is possible but invalid by PARSE_X264_OPT(), which  
seems
make no sense to calculate refs from x264_levels[] table.

- Linjie


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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] libavcodec/libx264: fix reference frame computation based on level

[Fu, Linjie]

Hi,

> From: ffmpeg-devel  On Behalf Of
> josh.brews...@protonmail.com
> Sent: Friday, April 17, 2020 07:05
> To: ffmpeg-devel@ffmpeg.org
> Subject: [FFmpeg-devel] [PATCH] libavcodec/libx264: fix reference frame
> computation based on level
> 
> Hell, can someone please review this patch? It fixes a wrong reference frame
> computation problem when using parameters such as "-level 31" instead of
> "-level 3.1".
>
> diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
> index a08fe0ce76..1149f2d668 100644
> --- a/libavcodec/libx264.c
> +++ b/libavcodec/libx264.c
> @@ -701,11 +701,14 @@ FF_ENABLE_DEPRECATION_WARNINGS
> 
>  if (!strcmp(x4->level, "1b")) {
>  level_id = 9;
> -} else if (strlen(x4->level) <= 3){
> +} else if (av_strtod(x4->level, NULL) < 7){
>  level_id = av_strtod(x4->level, ) * 10 + 0.5;
>  if (*tail)
>  level_id = -1;
>  }
> +else {
Wrong coding style for "else", should be "} else {"
> +level_id = av_strtod(x4->level, NULL);
> +}
>  if (level_id <= 0)
>  av_log(avctx, AV_LOG_WARNING, "Failed to parse level\n");

This part of code of parsing level_id seems redundant, since 
PARSE_X264_OPT("level", level) has
been called  and did  the same thing inside libx264, which converts x4->level 
to x4->params.i_level_idc
correctly (equals 31), regardless of "-level 3.1 or level 31".

Hence it would be better to use x4->params.i_level_idc directly.

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v3 5/9] lavc/libopenh264enc: set slice_mode option to deprecated

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of
> Anton Khirnov
> Sent: Tuesday, April 14, 2020 19:11
> To: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH v3 5/9] lavc/libopenh264enc: set
> slice_mode option to deprecated
> 
> Quoting Linjie Fu (2020-04-11 12:03:35)
> > "slice mode" seems to be unnecessary since it could be determined by
> > -slices/max_nal_size.
> >
> > default:SM_FIXEDSLCNUM_SLICE mode with cpu-number slices.
> > -slices N:  SM_FIXEDSLCNUM_SLICE mode with N slices.
> > -max_nal_size:  SM_SIZELIMITED_SLICE mode with limited size slices.
> >
> > Add OPENH264_API_SLICE_MODE to remove this option after
> > LIBAVCODEC_VERSION_MAJOR = 59.
> >
> > Signed-off-by: Linjie Fu 
> > ---
> >  libavcodec/libopenh264enc.c | 15 ---
> >  1 file changed, 12 insertions(+), 3 deletions(-)
> >
> > diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c
> > index e556f03..3a642f0 100644
> > --- a/libavcodec/libopenh264enc.c
> > +++ b/libavcodec/libopenh264enc.c
> > @@ -33,6 +33,10 @@
> >  #include "internal.h"
> >  #include "libopenh264.h"
> >
> > +#ifndef OPENH264_API_SLICE_MODE
> > +#define OPENH264_API_SLICE_MODE
> (LIBAVCODEC_VERSION_MAJOR < 59)
> > +#endif
> 
> This should be moved to libavcodec/version.h with an FF_ prefix, so that
> it's not overlooked when we bump the version.

Moved the macro to version.h and resend the whole patch set as V4.
https://patchwork.ffmpeg.org/project/ffmpeg/patch/1586926427-20170-6-git-send-email-linjie...@intel.com/
https://patchwork.ffmpeg.org/project/ffmpeg/patch/1586926427-20170-10-git-send-email-linjie...@intel.com/

 
> Otherwise looks ok

Thanks for the review.

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH, v2 1/2] lavc/qsvdec: add decode support for HEVC 4:2:2 8-bit and 10-bit

[Fu, Linjie]

> From: Zhong Li 
> Sent: Wednesday, April 15, 2020 09:58
> To: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Cc: Fu, Linjie 
> Subject: Re: [FFmpeg-devel] [PATCH, v2 1/2] lavc/qsvdec: add decode
> support for HEVC 4:2:2 8-bit and 10-bit
> 
> Linjie Fu  于2020年2月26日周三 下午4:43写道：
> >
> > Enables HEVC Range Extension decoding support (Linux) for 4:2:2 8/10 bit
> > on ICL+ (gen11 +) platform.
> >
> > Signed-off-by: Linjie Fu 
> > ---
> > [v2]: restrict to support on Linux.
> >
> >  libavcodec/qsv.c  | 16 
> >  libavutil/hwcontext_qsv.c | 24 
> >  2 files changed, 40 insertions(+)
> >
> > diff --git a/libavcodec/qsv.c b/libavcodec/qsv.c
> > index db98c75..171a8d6 100644
> > --- a/libavcodec/qsv.c
> > +++ b/libavcodec/qsv.c
> > @@ -195,6 +195,12 @@ enum AVPixelFormat ff_qsv_map_fourcc(uint32_t
> fourcc)
> >  case MFX_FOURCC_NV12: return AV_PIX_FMT_NV12;
> >  case MFX_FOURCC_P010: return AV_PIX_FMT_P010;
> >  case MFX_FOURCC_P8:   return AV_PIX_FMT_PAL8;
> > +#if CONFIG_VAAPI
> > +case MFX_FOURCC_YUY2: return AV_PIX_FMT_YUYV422;
> 
> There is no VAAPI structures here, so should not use CONFIG_VAAPI to
> disable them.
> 

This is pointed out in [1] that D3D code doesn't support YUYV format, and indeed
It leads to unexpected crash in windows.(instead of working or reporting 
unsupported
On ICL- platform)

Hence this patch restricted to add support on linux only.

And I admit the best solution should be get this fully supported on both linux 
and windows.
(I believe Max and Artem is helping on windows side)

Thanks for the review,
Linjie

[1] 
https://patchwork.ffmpeg.org/project/ffmpeg/patch/1582596080-1035-1-git-send-email-linjie...@intel.com/


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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH, v2 1/2] lavc/qsvdec: add decode support for HEVC 4:2:2 8-bit and 10-bit

[Fu, Linjie]

> From: Fu, Linjie 
> Sent: Wednesday, February 26, 2020 16:39
> To: ffmpeg-devel@ffmpeg.org
> Cc: Fu, Linjie 
> Subject: [PATCH,v2 1/2] lavc/qsvdec: add decode support for HEVC 4:2:2 8-bit
> and 10-bit
> 
> Enables HEVC Range Extension decoding support (Linux) for 4:2:2 8/10 bit
> on ICL+ (gen11 +) platform.
> 
> Signed-off-by: Linjie Fu 
> ---
> [v2]: restrict to support on Linux.
> 
>  libavcodec/qsv.c  | 16 
>  libavutil/hwcontext_qsv.c | 24 
>  2 files changed, 40 insertions(+)
> 
> diff --git a/libavcodec/qsv.c b/libavcodec/qsv.c
> index db98c75..171a8d6 100644
> --- a/libavcodec/qsv.c
> +++ b/libavcodec/qsv.c
> @@ -195,6 +195,12 @@ enum AVPixelFormat ff_qsv_map_fourcc(uint32_t
> fourcc)
>  case MFX_FOURCC_NV12: return AV_PIX_FMT_NV12;
>  case MFX_FOURCC_P010: return AV_PIX_FMT_P010;
>  case MFX_FOURCC_P8:   return AV_PIX_FMT_PAL8;
> +#if CONFIG_VAAPI
> +case MFX_FOURCC_YUY2: return AV_PIX_FMT_YUYV422;
> +#if QSV_VERSION_ATLEAST(1, 27)
> +case MFX_FOURCC_Y210: return AV_PIX_FMT_Y210;
> +#endif
> +#endif
>  }
>  return AV_PIX_FMT_NONE;
>  }
> @@ -211,6 +217,16 @@ int ff_qsv_map_pixfmt(enum AVPixelFormat format,
> uint32_t *fourcc)
>  case AV_PIX_FMT_P010:
>  *fourcc = MFX_FOURCC_P010;
>  return AV_PIX_FMT_P010;
> +#if CONFIG_VAAPI
> +case AV_PIX_FMT_YUV422P:
> +*fourcc = MFX_FOURCC_YUY2;
> +return AV_PIX_FMT_YUYV422;
> +#if QSV_VERSION_ATLEAST(1, 27)
> +case AV_PIX_FMT_YUV422P10:
> +*fourcc = MFX_FOURCC_Y210;
> +return AV_PIX_FMT_Y210;
> +#endif
> +#endif
>  default:
>  return AVERROR(ENOSYS);
>  }
> diff --git a/libavutil/hwcontext_qsv.c b/libavutil/hwcontext_qsv.c
> index b1b6740..4306c6e3 100644
> --- a/libavutil/hwcontext_qsv.c
> +++ b/libavutil/hwcontext_qsv.c
> @@ -44,6 +44,10 @@
>  #include "pixdesc.h"
>  #include "time.h"
> 
> +#define QSV_VERSION_ATLEAST(MAJOR, MINOR)   \
> +(MFX_VERSION_MAJOR > (MAJOR) || \
> + MFX_VERSION_MAJOR == (MAJOR) && MFX_VERSION_MINOR >=
> (MINOR))
> +
>  typedef struct QSVDevicePriv {
>  AVBufferRef *child_device_ctx;
>  } QSVDevicePriv;
> @@ -103,6 +107,14 @@ static const struct {
>  { AV_PIX_FMT_BGRA, MFX_FOURCC_RGB4 },
>  { AV_PIX_FMT_P010, MFX_FOURCC_P010 },
>  { AV_PIX_FMT_PAL8, MFX_FOURCC_P8   },
> +#if CONFIG_VAAPI
> +{ AV_PIX_FMT_YUYV422,
> +   MFX_FOURCC_YUY2 },
> +#if QSV_VERSION_ATLEAST(1, 27)
> +{ AV_PIX_FMT_Y210,
> +   MFX_FOURCC_Y210 },
> +#endif
> +#endif
>  };
> 
>  static uint32_t qsv_fourcc_from_pix_fmt(enum AVPixelFormat pix_fmt)
> @@ -773,7 +785,19 @@ static int map_frame_to_surface(const AVFrame
> *frame, mfxFrameSurface1 *surface)
>  surface->Data.R = frame->data[0] + 2;
>  surface->Data.A = frame->data[0] + 3;
>  break;
> +#if CONFIG_VAAPI
> +case AV_PIX_FMT_YUYV422:
> +surface->Data.Y = frame->data[0];
> +surface->Data.U = frame->data[0] + 1;
> +surface->Data.V = frame->data[0] + 3;
> +break;
> 
> +case AV_PIX_FMT_Y210:
> +surface->Data.Y16 = (mfxU16 *)frame->data[0];
> +surface->Data.U16 = (mfxU16 *)frame->data[0] + 1;
> +surface->Data.V16 = (mfxU16 *)frame->data[0] + 3;
> +break;
> +#endif
>  default:
>  return MFX_ERR_UNSUPPORTED;
>  }
> --
Ping for the patch set for 422 8/10 bit:
https://patchwork.ffmpeg.org/project/ffmpeg/patch/1582706369-32704-1-git-send-email-linjie...@intel.com/
https://patchwork.ffmpeg.org/project/ffmpeg/patch/1582706402-439-1-git-send-email-linjie...@intel.com/


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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] lavc/vaapi_encode_h265: add low_delay_b option for HEVC

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of
> Mark Thompson
> Sent: Monday, April 13, 2020 20:20
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH] lavc/vaapi_encode_h265: add
> low_delay_b option for HEVC
> 
> On 13/04/2020 05:32, Linjie Fu wrote:
> > Low delay B-frame is supported on ICL+ platform.
> >
> > For low power encoding, low_delay_b should be enabled by default.
> >
> > Low delay B:
> >  Video-Coding-HEVC-288.html>
> >
> > There is an on-going work in libva and media-driver to add querys
> > support for low delay b, would add it once it's ready:
> > https://github.com/intel/libva/pull/220
> > https://github.com/intel/libva/pull/364
> > https://github.com/intel/media-driver/issues/721
> >
> > Signed-off-by: Linjie Fu 
> > ---
> >  doc/encoders.texi  |  8 
> >  libavcodec/vaapi_encode_h265.c | 19 +--
> >  2 files changed, 25 insertions(+), 2 deletions(-)
> 
> My understanding was that the only reason for this stuff existing was to work
> around broken hardware which didn't support P frames.  Is this no longer
> true?
> 
> >
> > diff --git a/doc/encoders.texi b/doc/encoders.texi
> > index e23b6b3..b0812be 100644
> > --- a/doc/encoders.texi
> > +++ b/doc/encoders.texi
> > @@ -3089,6 +3089,14 @@ Some combination of the following values:
> >  Include HDR metadata if the input frames have it
> >  (@emph{mastering_display_colour_volume} and
> @emph{content_light_level}
> >  messages).
> > +
> > +@item low_delay_b
> > +Use low delay B-frames instead of P frames. Reordering of pictures is
> > +not allowed. The first picture is encoded as an I picture and subsequent
> > +pictures are encoded as B pictures. Moreover, since past B pictures are
> > +used for prediction, a low coding delay but with higher coding efficiency
> > +(because of bi-prediction) is achieved.
> 
> This sounds like a marketing blurb.
> 
> Not for the manual, but can you explain in detail what might actually make
> this better, with actual numbers if possible?  Intuitively the coding 
> efficiency
> will be worse, because a number of the B-picture syntax elements are just
> redundant but still have to be coded (picking between two options which are
> actually identical).  The gain of allowing biprediction between two identical
> pictures doesn't seem like a useful feature.
> 
The story is, this is kind of the hardware limitation for VDENC as we've 
discussed
on IRC.

As to the performance/efficiency, I'm curious too and would take some 
experiments.

This patch is kind of insufficient, updated a new version, to distinguish 
low_delay_b
and reference B frames, thx.

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] vaapi_decode: Improve logging around codec/profile selection

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of
> Mark Thompson
> Sent: Monday, April 13, 2020 21:14
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH] vaapi_decode: Improve logging around
> codec/profile selection
> 
> On 12/04/2020 18:19, Fu, Linjie wrote:
> >> From: ffmpeg-devel  On Behalf Of
> >> Mark Thompson
> >> Sent: Sunday, April 12, 2020 21:00
> >> To: ffmpeg-devel@ffmpeg.org
> >> Subject: [FFmpeg-devel] [PATCH] vaapi_decode: Improve logging around
> >> codec/profile selection
> >>
> >> ---
> >> On 12/04/2020 13:14, Mark Thompson wrote:
> >>> ...  This does rather suggest that the error messages in that file should 
> >>> be
> >> clearer, though - it would be nice if it could distinguish between "this
> codec
> >> isn't supported by libavcodec at all", "this codec might work but hasn't
> built
> >> into this libavcodec" and "this codec is supported by libavcodec but not by
> >> your hardware".
> >>
> >> Something like this?
> >>
> >>
> >>  libavcodec/vaapi_decode.c | 39
> +++
> >> 
> >>  1 file changed, 31 insertions(+), 8 deletions(-)
> >>
> >> --
> > Generally makes sense, however there is one concern if I got it correctly:
> >
> > If a codec is not supported by VAAPI in current libavcodec build,
> ff_get_format()
> > would not select VAAPI as the HW acceleration.
> > Instead, it would fallback to the native software decoding path, and won't
> trigger
> > the (!found_codec) logic.
> >
> > ./configure --enable-vaapi --disable-hwaccel=hevc_vaapi
> >
> > $ ffmpeg -v debug -hwaccel vaapi -
> i ./hevc_rext_decode/Main_422_10_A_RExt_Sony_2.bin -f null -
> >
> > Log:
> > [hevc @ 0x5592f7b0fec0] Format yuv422p10le chosen by get_format().
> >
> > VAAPI error return would not be triggered.
> 
> Yeah, it's not covering that case because of how get_format() works - the
> caller can't choose the VAAPI format, so we never get here.  If the caller
> really wanted VAAPI then they need to deal with it externally (for ffmpeg,
> you want something like the proposed -decode_format option:
> <https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2018-
> November/236205.html>).
> 
> The case where the "no codec" message is triggered is when you can pick it
> (the hwaccel is enabled) but the build still doesn't have the codec in the 
> table
> (for example if it were built against older libva headers).
> 
Yep, got the point and looks reasonable, thanks for elaboration.

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] lavc/vaapi_encode_h265: fix max_transform_hierarchy_depth_inter/intra

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of
> Mark Thompson
> Sent: Monday, April 13, 2020 20:20
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH] lavc/vaapi_encode_h265: fix
> max_transform_hierarchy_depth_inter/intra
> 
> On 13/04/2020 05:32, Linjie Fu wrote:
> > Set the max_transform_hierarchy_depth_inter/intra to 2 by default
> > based on the Programmer's Reference Manuals (PRM) in [1].
> >
> > Intel Encoder only supports 2 levels of quad-tree. That is:
> > - max_transform_hierarchy_depth_inter/intra <= 2.
> >
> > [1]  kbl-vol10-hevc.pdf>
> >
> > Signed-off-by: Linjie Fu 
> > ---
> > Fixed value for intel platform, makes more sense on TGL+ platform.
> > (If conflict with other driver capability, we may add query support
> >  later)
> >  libavcodec/vaapi_encode_h265.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/libavcodec/vaapi_encode_h265.c
> b/libavcodec/vaapi_encode_h265.c
> > index cd48545..d6cb82a 100644
> > --- a/libavcodec/vaapi_encode_h265.c
> > +++ b/libavcodec/vaapi_encode_h265.c
> > @@ -445,8 +445,9 @@ static int
> vaapi_encode_h265_init_sequence_params(AVCodecContext *avctx)
> >  sps->log2_min_luma_transform_block_size_minus2   = 0;
> >  sps->log2_diff_max_min_luma_transform_block_size = 3;
> >  // Full transform hierarchy allowed (2-5).
> > -sps->max_transform_hierarchy_depth_inter = 3;
> > -sps->max_transform_hierarchy_depth_intra = 3;
> > +// Default to 2 based on Programmer's Reference Manuals of Intel
> graphics
> > +sps->max_transform_hierarchy_depth_inter = 2;
> > +sps->max_transform_hierarchy_depth_intra = 2;
> >  // AMP works.
> >  sps->amp_enabled_flag = 1;
> >  // SAO and temporal MVP do not work.
> >
> 
> I don't much like the idea of changing this based on a value in a Kaby Lake
> document given that the current value hasn't had any problems on Kaby Lake.
> Can you explain the benefits of changing this?  

It fixes the encoding issue for HEVC on gen12+ platform.
We didn't notice this either, until it triggers gpu hang for encoding on Tiger 
Lake (gen12).

> Can you confirm that it continues to work on all the other currently-working 
> platforms?
> 
Yes, we set up CI and runs full-round tests for conformance and catch 
regression on different
platforms(CFL/CML/ICL/KBL/SKL).

Identical fix has been merged in gstreamer-vaapi, no regression is detected on 
other platforms.
https://gitlab.freedesktop.org/gstreamer/gstreamer-vaapi/-/merge_requests/282/diffs?commit_id=17d82e14e78af901f1cd7f2344e173ad6ae6a8a6

- Linjie


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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] lavc/vaapi_encode_h265: add low_delay_b option for HEVC

[Fu, Linjie]

> From: myp...@gmail.com 
> Sent: Monday, April 13, 2020 15:43
> To: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Cc: Fu, Linjie 
> Subject: Re: [FFmpeg-devel] [PATCH] lavc/vaapi_encode_h265: add
> low_delay_b option for HEVC
> 
> On Mon, Apr 13, 2020 at 12:39 PM Linjie Fu  wrote:
> >
> > Low delay B-frame is supported on ICL+ platform.
> >
> > For low power encoding, low_delay_b should be enabled by default.
> >
> > Low delay B:
> > <http://what-when-how.com/Tutorial/topic-397pct9eq3/High-Efficiency-
> Video-Coding-HEVC-288.html>
> >
> > There is an on-going work in libva and media-driver to add querys
> > support for low delay b, would add it once it's ready:
> > https://github.com/intel/libva/pull/220
> > https://github.com/intel/libva/pull/364
> > https://github.com/intel/media-driver/issues/721
> >
> > Signed-off-by: Linjie Fu 
> > ---
> >  doc/encoders.texi  |  8 
> >  libavcodec/vaapi_encode_h265.c | 19 +--
> >  2 files changed, 25 insertions(+), 2 deletions(-)
> >
> > diff --git a/doc/encoders.texi b/doc/encoders.texi
> > index e23b6b3..b0812be 100644
> > --- a/doc/encoders.texi
> > +++ b/doc/encoders.texi
> > @@ -3089,6 +3089,14 @@ Some combination of the following values:
> >  Include HDR metadata if the input frames have it
> >  (@emph{mastering_display_colour_volume} and
> @emph{content_light_level}
> >  messages).
> > +
> > +@item low_delay_b
> > +Use low delay B-frames instead of P frames. Reordering of pictures is
> > +not allowed. The first picture is encoded as an I picture and subsequent
> > +pictures are encoded as B pictures. Moreover, since past B pictures are
> > +used for prediction, a low coding delay but with higher coding efficiency
> > +(because of bi-prediction) is achieved.
> > +
> >  @end table
> >
> >  @end table
> > diff --git a/libavcodec/vaapi_encode_h265.c
> b/libavcodec/vaapi_encode_h265.c
> > index 97dc5a7..cd48545 100644
> > --- a/libavcodec/vaapi_encode_h265.c
> > +++ b/libavcodec/vaapi_encode_h265.c
> > @@ -62,6 +62,7 @@ typedef struct VAAPIEncodeH265Context {
> >  int tier;
> >  int level;
> >  int sei;
> > +int low_delay_b;
> >
> >  // Derived settings.
> >  int fixed_qp_idr;
> > @@ -894,6 +895,9 @@ static int
> vaapi_encode_h265_init_slice_params(AVCodecContext *avctx,
> >
> >  sh->slice_type = hpic->slice_type;
> >
> > +if (sh->slice_type == HEVC_SLICE_P && priv->low_delay_b)
> > +sh->slice_type = HEVC_SLICE_B;
> Add a trace or info message in this, it's will help to reduce the
> sudden surprises, 

Yes, considered this and the trace/info message for low_delay_b change
was added in vaapi_encode_h265_init() as a AV_LOG_VERBOSE Information,
so I didn't prompt a debug information for each slice.

> the other question is, if enable the  low_delay_b in
>  ICL-  platform, what is the result of this?

Possibly leads to encoding failure.
And that's why a query support is necessary for this option/feature on ICL-.


While replying this mail, I noticed this patch is insufficient. 
Will update a new one, sorry for the noise.

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] vaapi_decode: Improve logging around codec/profile selection

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of
> Mark Thompson
> Sent: Sunday, April 12, 2020 21:00
> To: ffmpeg-devel@ffmpeg.org
> Subject: [FFmpeg-devel] [PATCH] vaapi_decode: Improve logging around
> codec/profile selection
> 
> ---
> On 12/04/2020 13:14, Mark Thompson wrote:
> > ...  This does rather suggest that the error messages in that file should be
> clearer, though - it would be nice if it could distinguish between "this codec
> isn't supported by libavcodec at all", "this codec might work but hasn't built
> into this libavcodec" and "this codec is supported by libavcodec but not by
> your hardware".
> 
> Something like this?
> 
> 
>  libavcodec/vaapi_decode.c | 39 +++
> 
>  1 file changed, 31 insertions(+), 8 deletions(-)
> 
> diff --git a/libavcodec/vaapi_decode.c b/libavcodec/vaapi_decode.c
> index 54a0ecb47a..a191850e36 100644
> --- a/libavcodec/vaapi_decode.c
> +++ b/libavcodec/vaapi_decode.c
> @@ -429,6 +429,7 @@ static int
> vaapi_decode_make_config(AVCodecContext *avctx,
>  const AVCodecDescriptor *codec_desc;
>  VAProfile *profile_list = NULL, matched_va_profile, va_profile;
>  int profile_count, exact_match, matched_ff_profile, codec_profile;
> +int found_codec, found_profile;
> 
>  AVHWDeviceContext*device = (AVHWDeviceContext*)device_ref-
> >data;
>  AVVAAPIDeviceContext *hwctx = device->hwctx;
> @@ -457,15 +458,19 @@ static int
> vaapi_decode_make_config(AVCodecContext *avctx,
>  }
> 
>  matched_va_profile = VAProfileNone;
> +found_codec = found_profile = 0;
>  exact_match = 0;
> 
>  for (i = 0; i < FF_ARRAY_ELEMS(vaapi_profile_map); i++) {
>  int profile_match = 0;
>  if (avctx->codec_id != vaapi_profile_map[i].codec_id)
>  continue;
> +found_codec = 1;
>  if (avctx->profile == vaapi_profile_map[i].codec_profile ||
> -vaapi_profile_map[i].codec_profile == FF_PROFILE_UNKNOWN)
> +vaapi_profile_map[i].codec_profile == FF_PROFILE_UNKNOWN) {
>  profile_match = 1;
> +found_profile = 1;
> +}
> 
>  va_profile = vaapi_profile_map[i].profile_parser ?
>   vaapi_profile_map[i].profile_parser(avctx) :
> @@ -487,24 +492,42 @@ static int
> vaapi_decode_make_config(AVCodecContext *avctx,
>  }
>  av_freep(_list);
> 
> -if (matched_va_profile == VAProfileNone) {
> -av_log(avctx, AV_LOG_ERROR, "No support for codec %s "
> -   "profile %d.\n", codec_desc->name, avctx->profile);
> +if (!found_codec) {
> +av_log(avctx, AV_LOG_ERROR, "This libavcodec build does not "
> +   "support VAAPI decoding of codec %s.\n",
> +   codec_desc->name);
> +err = AVERROR(ENOSYS);
> +goto fail;
> +}
> +if (!found_profile && !(avctx->hwaccel_flags &
> +AV_HWACCEL_FLAG_ALLOW_PROFILE_MISMATCH)) {
> +// We allow this case with profile-mismatch enabled to support
> +// things like trying to decode H.264 extended profile.
> +av_log(avctx, AV_LOG_ERROR, "This libavcodec build does not "
> +   "support VAAPI decoding of codec %s profile %d.\n",
> +   codec_desc->name, avctx->profile);
>  err = AVERROR(ENOSYS);
>  goto fail;
>  }
> +if (matched_va_profile == VAProfileNone) {
> +av_log(avctx, AV_LOG_ERROR, "This VAAPI driver does not "
> +   "support decoding of codec %s.\n",
> +   codec_desc->name);
> +err = AVERROR(EINVAL);
> +goto fail;
> +}
>  if (!exact_match) {
>  if (avctx->hwaccel_flags &
>  AV_HWACCEL_FLAG_ALLOW_PROFILE_MISMATCH) {
> -av_log(avctx, AV_LOG_VERBOSE, "Codec %s profile %d not "
> -   "supported for hardware decode.\n",
> +av_log(avctx, AV_LOG_WARNING, "This VAAPI driver does not "
> +   "support decoding of codec %s profile %d.\n",
> codec_desc->name, avctx->profile);
>  av_log(avctx, AV_LOG_WARNING, "Using possibly-"
> "incompatible profile %d instead.\n",
> matched_ff_profile);
>  } else {
> -av_log(avctx, AV_LOG_VERBOSE, "Codec %s profile %d not "
> -   "supported for hardware decode.\n",
> +av_log(avctx, AV_LOG_ERROR, "This VAAPI driver does not "
> +   "support decoding of codec %s profile %d.\n",
> codec_desc->name, avctx->profile);
>  err = AVERROR(EINVAL);
>  goto fail;
> --
Generally makes sense, however there is one concern if I got it correctly:

If a codec is not supported by VAAPI in current libavcodec build, 
ff_get_format()
would not select VAAPI as the HW acceleration. 
Instead, it would fallback to the native software decoding path, and won't 
trigger
the (!found_codec) logic.

./configure 

Re: [FFmpeg-devel] [PATCH] lavc/vaapi_decode: fix the build failure when hevc_vaapi is disabled

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of
> Mark Thompson
> Sent: Sunday, April 12, 2020 20:14
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH] lavc/vaapi_decode: fix the build failure
> when hevc_vaapi is disabled
> 
> On 12/04/2020 12:55, Andreas Rheinhardt wrote:
> > Linjie Fu:
> >> Verified with ./configure --enable-vaapi --disable-hwaccel=hevc_vaapi
> >>
> >> Failure reported in:
> >> http://fate.ffmpeg.org/report.cgi?time=20200401135031=x86_64-
> archlinux-gcc-random
> >>
> >> Signed-off-by: Linjie Fu 
> >> ---
> >>  libavcodec/vaapi_decode.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/libavcodec/vaapi_decode.c b/libavcodec/vaapi_decode.c
> >> index 54a0ecb..06916cc 100644
> >> --- a/libavcodec/vaapi_decode.c
> >> +++ b/libavcodec/vaapi_decode.c
> >> @@ -383,6 +383,7 @@ static const struct {
> >> H264ConstrainedBaseline),
> >>  MAP(H264,H264_MAIN,   H264Main),
> >>  MAP(H264,H264_HIGH,   H264High),
> >> +#if CONFIG_HEVC_VAAPI_HWACCEL
> >>  #if VA_CHECK_VERSION(0, 37, 0)
> >>  MAP(HEVC,HEVC_MAIN,   HEVCMain),
> >>  MAP(HEVC,HEVC_MAIN_10,HEVCMain10  ),
> >> @@ -393,6 +394,7 @@ static const struct {
> >>  MAP(HEVC,HEVC_REXT,   None,
> >>   ff_vaapi_parse_hevc_rext_profile ),
> >>  #endif
> >> +#endif
> >>  MAP(MJPEG,   MJPEG_HUFFMAN_BASELINE_DCT,
> >>JPEGBaseline),
> >>  MAP(WMV3,VC1_SIMPLE,  VC1Simple   ),
> >>
> > Any more comments? If not, I'll apply this soon (i.e. in a few hours).
> 
> I'd put it around the RExt part only, to be more consistent with other codecs.
Ok, and  also combined with VA_CHECK_VERSION as Carl has suggested, thx.

- Linjie


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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 3/3] lavc/vaapi_encode_h265: add h265 tile encoding support

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of Fu,
> Linjie
> Sent: Saturday, March 28, 2020 23:02
> To: FFmpeg development discussions and patches  de...@ffmpeg.org>; myp...@gmail.com
> Subject: Re: [FFmpeg-devel] [PATCH 3/3] lavc/vaapi_encode_h265: add h265
> tile encoding support
> 
> > From: ffmpeg-devel  On Behalf Of
> Fu,
> > Linjie
> > Sent: Tuesday, March 24, 2020 10:15
> > To: myp...@gmail.com; FFmpeg development discussions and patches
> > 
> > Subject: Re: [FFmpeg-devel] [PATCH 3/3] lavc/vaapi_encode_h265: add
> h265
> > tile encoding support
> >
> > > From: myp...@gmail.com 
> > > Sent: Monday, March 23, 2020 22:44
> > > To: FFmpeg development discussions and patches  > > de...@ffmpeg.org>
> > > Cc: Fu, Linjie 
> > > Subject: Re: [FFmpeg-devel] [PATCH 3/3] lavc/vaapi_encode_h265: add
> > h265
> > > tile encoding support
> > >
> > > On Mon, Mar 23, 2020 at 10:30 PM Linjie Fu  wrote:
> > > >
> > > > Default to enable uniform_spacing_flag. Guess level by the tile
> > > > rows/cols. Supported for ICL+ platforms.
> > > >
> > > > Also add documentations.
> > > >
> > > > To encode with 4 rows 2 columns:
> > > > ffmpeg ... -c:v hevc_vaapi -tile_rows 4 -tile_cols 2 ...
> > > >
> > > > Signed-off-by: Linjie Fu 
> > > > ---
> > > >  doc/encoders.texi  |  8 
> > > >  libavcodec/vaapi_encode_h265.c | 36
> > > +++-
> > > >  2 files changed, 43 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/doc/encoders.texi b/doc/encoders.texi
> > > > index e23b6b3..2dc5cd4 100644
> > > > --- a/doc/encoders.texi
> > > > +++ b/doc/encoders.texi
> > > > @@ -3091,6 +3091,14 @@ Include HDR metadata if the input frames
> have
> > it
> > > >  messages).
> > > >  @end table
> > > >
> > > > +@item tile_rows
> > > > +Selects how many rows of tiles to encode with. For example, 4 tile
> rows
> > > would
> > > > +be requested by setting the tile_rows option to 4.
> > > > +
> > > > +@item tile_cols
> > > > +Selects how many columns of tiles to encode with. For example, 5 tile
> > > columns
> > > > +would be requested by setting the tile_cols option to 5.
> > > > +
> > > >  @end table
> > > >
> > > >  @item mjpeg_vaapi
> > > > diff --git a/libavcodec/vaapi_encode_h265.c
> > > b/libavcodec/vaapi_encode_h265.c
> > > > index 97dc5a7..457e3d9 100644
> > > > --- a/libavcodec/vaapi_encode_h265.c
> > > > +++ b/libavcodec/vaapi_encode_h265.c
> > > > @@ -63,6 +63,9 @@ typedef struct VAAPIEncodeH265Context {
> > > >  int level;
> > > >  int sei;
> > > >
> > > > +int trows;
> > > > +int tcols;
> > > > +
> > > >  // Derived settings.
> > > >  int fixed_qp_idr;
> > > >  int fixed_qp_p;
> > > > @@ -345,7 +348,7 @@ static int
> > > vaapi_encode_h265_init_sequence_params(AVCodecContext *avctx)
> > > >
> > > >  level = ff_h265_guess_level(ptl, avctx->bit_rate,
> > > >  ctx->surface_width, 
> > > > ctx->surface_height,
> > > > -ctx->nb_slices, 1, 1,
> > > > +ctx->nb_slices, ctx->tile_rows, 
> > > > ctx->tile_cols,
> > > >  (ctx->b_per_p > 0) + 1);
> > > >  if (level) {
> > > >  av_log(avctx, AV_LOG_VERBOSE, "Using level %s.\n", level-
> > >name);
> > > > @@ -558,6 +561,20 @@ static int
> > > vaapi_encode_h265_init_sequence_params(AVCodecContext *avctx)
> > > >
> > > >  pps->pps_loop_filter_across_slices_enabled_flag = 1;
> > > >
> > > > +if (ctx->tile_rows && ctx->tile_cols) {
> > > > +pps->tiles_enabled_flag = 1;
> > > > +pps->uniform_spacing_flag   = 1;
> > > > +
> > > > +pps->num_tile_rows_minus1   = ctx->tile_rows - 1;
> > > > +pps->num_tile_columns_minus1= ctx->tile_cols - 1;
> > > > +
> > > > +

Re: [FFmpeg-devel] [PATCH 04/10] lavc/libopenh264enc: add bit rate control select support

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of
> Anton Khirnov
> Sent: Friday, April 10, 2020 18:20
> To: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 04/10] lavc/libopenh264enc: add bit
> rate control select support
> 
> Quoting Linjie Fu (2020-04-06 13:14:47)
> > RC_BITRATE_MODE:
> > set BITS_EXCEEDED to iCurrentBitsLevel and allows QP adjust
> > in RcCalculatePictureQp().
> >
> > RC_BUFFERBASED_MODE:
> > use buffer status to adjust the video quality.
> >
> > RC_TIMESTAMP_MODE:
> > bit rate control based on timestamp.
> >
> > Default to use RC_QUALITY_MODE.
> >
> > Signed-off-by: Linjie Fu 
> > ---
> >  libavcodec/libopenh264enc.c | 12 +++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> >
> 
> Doesn't this functionality depend on what variables the user specified
> explicitly (like qscale, max/min/avg bitrates, buffer sizes etc.).
> Wouldn't it be better to choose the default RC type based on those, like
> vaapi does it?
> 

Judging rc_mode by specific variables makes great sense.

And I'm planning to add this in later patches, because libopenh264enc currently
lacks the supports for avctx->global_quality, avctx->flags & 
AV_CODEC_FLAG_QSCALE,
or a specific qp. They should be added firstly.

This patch is the first step for user to determined it by explicit rc_mode, 
like vaapi does.

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 05/10] lavc/libopenh264enc: prompt slice number changing according to cpus

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of
> Anton Khirnov
> Sent: Saturday, April 11, 2020 16:38
> To: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 05/10] lavc/libopenh264enc: prompt
> slice number changing according to cpus
> 
> Quoting Fu, Linjie (2020-04-10 15:49:30)
> > > From: ffmpeg-devel  On Behalf Of
> > > Anton Khirnov
> > > Sent: Friday, April 10, 2020 18:23
> > > To: FFmpeg development discussions and patches  > > de...@ffmpeg.org>
> > > Subject: Re: [FFmpeg-devel] [PATCH 05/10] lavc/libopenh264enc: prompt
> > > slice number changing according to cpus
> > >
> > > Quoting Linjie Fu (2020-04-06 13:14:48)
> > > > Libopenh264enc would set the slice according to the number of cpu
> cores
> > > > if uiSliceNum equals to 0 (auto) in SM_FIXEDSLCNUM_SLICE mode.
> > > >
> > > > Prompt a warning for user to catch this.
> > > >
> > > > Signed-off-by: Linjie Fu 
> > > > ---
> > > >  libavcodec/libopenh264enc.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c
> > > > index dab8244..01a85fb 100644
> > > > --- a/libavcodec/libopenh264enc.c
> > > > +++ b/libavcodec/libopenh264enc.c
> > > > @@ -237,6 +237,9 @@ FF_ENABLE_DEPRECATION_WARNINGS
> > > >  param.sSpatialLayers[0].sSliceCfg.uiSliceMode   = s-
> >slice_mode;
> > > >  param.sSpatialLayers[0].sSliceCfg.sSliceArgument.uiSliceNum =
> avctx-
> > > >slices;
> > > >  #endif
> > > > +if (avctx->slices == 0 && s->slice_mode == SM_FIXEDSLCNUM_SLICE)
> > > > +av_log(avctx, AV_LOG_WARNING, "Auto slice number, "
> > > > +   "default to use the number of CPU cores: %d\n",
> av_cpu_count());
> > >
> > > Generally makes sense, but I'd avoid the call to av_cpu_count() since we
> > > don't know what method precisely will libopenh264 use to set the slice
> > > count. So IMO just say something like "slice count will be set
> > > automatically".
> >
> > There is a statement in the API definition[1] for uiSliceNum which says:
> > when uiSliceNum=0 means auto design it with cpu core number.
> > So IMHO it seems clear enough if I got it correctly.
> 
> My concern is that different methods of computing the core count might
> return different results in certain cases. E.g. if the code is running
> under some resource management system that restricts it to a subset of
> total cores on the system. So we should not assume that av_cpu_count()
> will always necessarily return the same number that libopenh264 uses.

Indeed, thanks for the elaborations.

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 03/10] lavc/libopenh264enc: add default gop size and bit rate

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of
> Anton Khirnov
> Sent: Saturday, April 11, 2020 16:43
> To: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 03/10] lavc/libopenh264enc: add
> default gop size and bit rate
> 
> Quoting Fu, Linjie (2020-04-10 15:33:04)
> > > From: ffmpeg-devel  On Behalf Of
> > > Anton Khirnov
> > > Sent: Friday, April 10, 2020 18:14
> > > To: FFmpeg development discussions and patches  > > de...@ffmpeg.org>
> > > Subject: Re: [FFmpeg-devel] [PATCH 03/10] lavc/libopenh264enc: add
> > > default gop size and bit rate
> > >
> > > Quoting Linjie Fu (2020-04-06 13:14:46)
> > > > Signed-off-by: Linjie Fu 
> > > > ---
> > > >  libavcodec/libopenh264enc.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c
> > > > index c7ae5b1..3ff5be7 100644
> > > > --- a/libavcodec/libopenh264enc.c
> > > > +++ b/libavcodec/libopenh264enc.c
> > > > @@ -335,6 +335,8 @@ static int svc_encode_frame(AVCodecContext
> > > *avctx, AVPacket *avpkt,
> > > >  }
> > > >
> > > >  static const AVCodecDefault svc_enc_defaults[] = {
> > > > +{ "b", "2M"},
> > > > +{ "g", "120"   },
> > >
> > > Why these values specifically?
> >
> > According to the default settings in nvenc (b = 2M) and vaapi encoder for
> h264 (g = 120).
> >
> > >What happens if we leave them unset?
> >
> > It would be 200kbps bitrate with gop size = 12.
> >
> > There would be too much IDR frames with in small bitrate, which leads to
> > a poor quality.
> >
> >  (I should have added such statements in the commit message)
> 
> I mean we could override the defaults to "-1" like is done for e.g.
> x264, then we can distinguish between "the user specified the bitrate to
> be " vs. "the user did not specify anything about the target
> bitrate".

Indeed, I noticed this while attempting to select rc_mode by avctx->bit_rate.
It makes sense to distinguish them, and I've changed it.

> And if the user did not specify the bitrate we could leave libopenh264
> to choose bitrate on its own or use some other rate limiting mechanism
> like constant QP or something CRF-like (don't know if it has that).

I'm afraid it doesn't have one if we leave the bitrate unset:
[OpenH264] this = 0x0x55a61f80, Error: Invalid bitrate settings in total 
configure, bitrate= 0

so I'll assign a default target bitrate to 2M.

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 06/10] lavc/libopenh264enc: set slice_mode option to deprecated

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of
> Anton Khirnov
> Sent: Friday, April 10, 2020 18:28
> To: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 06/10] lavc/libopenh264enc: set
> slice_mode option to deprecated
> 
> Quoting Linjie Fu (2020-04-06 13:14:49)
> > "slice mode" seems to be unnecessary since it could be determined by
> > -slices/max_nal_size.
> >
> > default:SM_FIXEDSLCNUM_SLICE mode with cpu-number slices.
> > -slices N:  SM_FIXEDSLCNUM_SLICE mode with N slices.
> > -max_nal_size:  SM_SIZELIMITED_SLICE mode with limited size slices.
> >
> > This could be removed later.
> >
> > Signed-off-by: Linjie Fu 
> > ---
> 
> I'd say also schedule it for removal with new FF_API_ macros, otherwise
> deprecated things tend to stay around for way too long.
> 
Good idea, added OPENH264_API_SLICE_MODE macros to remove this
Options after LIBAVCODEC_VERSION_MAJOR = 59.

Thanks for the suggestion.

- Linjie

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 08/10] lavc/libopenh264enc: add profile high option support

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of
> Anton Khirnov
> Sent: Friday, April 10, 2020 18:57
> To: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 08/10] lavc/libopenh264enc: add profile
> high option support
> 
> Quoting Linjie Fu (2020-04-06 13:14:51)
> > Add support for PRO_HIGH/PRO_BASELINE in SVC Encoding extention
> mode,
> > which determined by iEntropyCodingModeFlag in ParamTranscode().
> >
> >
>  param_svc.h#L394>
> >
> > Signed-off-by: Linjie Fu 
> > ---
> >  libavcodec/libopenh264enc.c | 32 ++--
> >  1 file changed, 26 insertions(+), 6 deletions(-)
> >
> > diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c
> > index f02c5fd..d331cfd 100644
> > --- a/libavcodec/libopenh264enc.c
> > +++ b/libavcodec/libopenh264enc.c
> > @@ -42,7 +42,7 @@ typedef struct SVCContext {
> >  ISVCEncoder *encoder;
> >  int slice_mode; // deprecated
> >  int loopfilter;
> > -char *profile;
> > +int profile;
> 
> Wouldn't it be better to deprecate the encoder-private option and use
> only the AVCodecContext one?
> 

If I got it correctly, the general "profile" option in 
libavcodec/options_table.h
needs values of type INT like FF_PROFILE_H264_HIGH = 100;

It also works for encoders with "-profile:v 100" to encode a bitstream with 
profile
HIGH, however maybe not straight forward enough. And it seems to be one of the
reasons that many encoders have a private option with AV_OPT_TYPE_STRING type.

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 05/10] lavc/libopenh264enc: prompt slice number changing according to cpus

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of
> Anton Khirnov
> Sent: Friday, April 10, 2020 18:23
> To: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 05/10] lavc/libopenh264enc: prompt
> slice number changing according to cpus
> 
> Quoting Linjie Fu (2020-04-06 13:14:48)
> > Libopenh264enc would set the slice according to the number of cpu cores
> > if uiSliceNum equals to 0 (auto) in SM_FIXEDSLCNUM_SLICE mode.
> >
> > Prompt a warning for user to catch this.
> >
> > Signed-off-by: Linjie Fu 
> > ---
> >  libavcodec/libopenh264enc.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c
> > index dab8244..01a85fb 100644
> > --- a/libavcodec/libopenh264enc.c
> > +++ b/libavcodec/libopenh264enc.c
> > @@ -237,6 +237,9 @@ FF_ENABLE_DEPRECATION_WARNINGS
> >  param.sSpatialLayers[0].sSliceCfg.uiSliceMode   = 
> > s->slice_mode;
> >  param.sSpatialLayers[0].sSliceCfg.sSliceArgument.uiSliceNum = avctx-
> >slices;
> >  #endif
> > +if (avctx->slices == 0 && s->slice_mode == SM_FIXEDSLCNUM_SLICE)
> > +av_log(avctx, AV_LOG_WARNING, "Auto slice number, "
> > +   "default to use the number of CPU cores: %d\n", 
> > av_cpu_count());
> 
> Generally makes sense, but I'd avoid the call to av_cpu_count() since we
> don't know what method precisely will libopenh264 use to set the slice
> count. So IMO just say something like "slice count will be set
> automatically".

There is a statement in the API definition[1] for uiSliceNum which says:
when uiSliceNum=0 means auto design it with cpu core number.
So IMHO it seems clear enough if I got it correctly.

I'm good with this suggestion and will update, since the detailed slice number
would also be dumped in debug level as well (however a little bit mixed up with
all the debug info), thx.

- Linjie

[1] 
https://github.com/cisco/openh264/blob/master/codec/api/svc/codec_app_def.h#L351

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 03/10] lavc/libopenh264enc: add default gop size and bit rate

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of
> Anton Khirnov
> Sent: Friday, April 10, 2020 18:14
> To: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 03/10] lavc/libopenh264enc: add
> default gop size and bit rate
> 
> Quoting Linjie Fu (2020-04-06 13:14:46)
> > Signed-off-by: Linjie Fu 
> > ---
> >  libavcodec/libopenh264enc.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c
> > index c7ae5b1..3ff5be7 100644
> > --- a/libavcodec/libopenh264enc.c
> > +++ b/libavcodec/libopenh264enc.c
> > @@ -335,6 +335,8 @@ static int svc_encode_frame(AVCodecContext
> *avctx, AVPacket *avpkt,
> >  }
> >
> >  static const AVCodecDefault svc_enc_defaults[] = {
> > +{ "b", "2M"},
> > +{ "g", "120"   },
> 
> Why these values specifically? 

According to the default settings in nvenc (b = 2M) and vaapi encoder for h264 
(g = 120).

>What happens if we leave them unset?

It would be 200kbps bitrate with gop size = 12.

There would be too much IDR frames with in small bitrate, which leads to
a poor quality. 

 (I should have added such statements in the commit message)

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 01/10] lavc/libopenh264enc: Add default qmin/qmax support

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of
> Anton Khirnov
> Sent: Friday, April 10, 2020 18:12
> To: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 01/10] lavc/libopenh264enc: Add
> default qmin/qmax support
> 
> Quoting Linjie Fu (2020-04-06 13:14:44)
> > Set default QP range to (1, 51) instead of (2, 32).
> >
> > QP = 0 is not well supported currently in libopenh264. If iMaxQp/iMinQp
> > equals 0, the QP range would be changed unexpectedly inside libopenh264
> > with a warning:
> >
> > Warning:Change QP Range from(0,51) to (12,42)
> >
> > [1]
>  encoder_ext.cpp#L375>
> > [2] 
> >
> > Signed-off-by: Linjie Fu 
> > ---
> >  libavcodec/libopenh264enc.c | 10 ++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c
> > index ae6d17c..364926f 100644
> > --- a/libavcodec/libopenh264enc.c
> > +++ b/libavcodec/libopenh264enc.c
> > @@ -135,6 +135,9 @@ FF_ENABLE_DEPRECATION_WARNINGS
> >  param.iTargetBitrate = avctx->bit_rate;
> >  param.iMaxBitrate= FFMAX(avctx->rc_max_rate, avctx-
> >bit_rate);
> >  param.iRCMode= RC_QUALITY_MODE;
> > +// QP = 0 is not well supported, so default to (1, 51)
> > +param.iMaxQp = avctx->qmax >= 0 ? 
> > av_clip(avctx->qmax, 1,
> 51) : 51;
> > +param.iMinQp = avctx->qmin >= 0 ? 
> > av_clip(avctx->qmin, 1,
> param.iMaxQp) : 1;
> 
> Should we set them at all if they are not specified by the user?

The answer is no and that's why I set qmin/qmax to -1 by default, otherwise
they would be set to (2, 31) by default [1] in VBR mode in FFmpeg.

> Wouldn't it be better to leave the unset, as is done now?

Most encoders would prefer to get a default QP range specifically if it's not
explicitly set by user, either choosing a default value inside ffmpeg or leave 
it
unset to be determined in core library/driver, like libx264, vaapi, qsv, nvenc.

For libopenh264enc specifically, the supported range is (1, 51), qp = 0 is not 
well
Supported yet in libopenh264 library [2]. If either of qmin or qmax equals 0, 
the
qp range would be reduced to (12, 42) inside libopenh264 library.

The default QP range is supposed to be as wide as enough IMHO, so set it to (1, 
51)
and avoid the warning inside the library. (and MaxQp =51 is the recommended 
value
in the demo config file in /openh264/testbin/welsenc.cfg)

Thanks for the review.

- Linjie

[1] https://github.com/FFmpeg/FFmpeg/blob/master/libavcodec/options_table.h#L97
[2] 

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 00/10] Patch set for the enhancement of libopenh264 encoder

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of Fu,
> Linjie
> Sent: Wednesday, April 8, 2020 17:48
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH 00/10] Patch set for the enhancement
> of libopenh264 encoder
> 
> > From: Fu, Linjie 
> > Sent: Monday, April 6, 2020 19:15
> > To: ffmpeg-devel@ffmpeg.org
> > Cc: Fu, Linjie 
> > Subject: [PATCH 00/10] Patch set for the enhancement of libopenh264
> > encoder
> >
> > Docs will be provided later.
> >
> > Linjie Fu (10):
> >   lavc/libopenh264enc: Add default qmin/qmax support
> > [v2] fix the av_clip logic for iMinQp.
> >   lavc/libopenh264enc: fix the if-else coding style
> >   lavc/libopenh264enc: add default gop size and bit rate
> >   lavc/libopenh264enc: add bit rate control select support
> >   lavc/libopenh264enc: prompt slice number changing according to cpus
> >   lavc/libopenh264enc: set slice_mode option to deprecated
> >   lavc/libopenh264enc: separate svc_encode_init() into several functions
> > [v2] remove forward declarations.
> > New:
> >   lavc/libopenh264enc: add profile high option support
> >   lavc/libopenh264enc: allow specifying the profile through
> > AVCodecContext
> >   lavc/libopenh264enc: replace cabac option with coder
> >
> >  libavcodec/libopenh264enc.c | 361
> ++--
> > 
> >  1 file changed, 246 insertions(+), 115 deletions(-)
> 
> Ping for this set.
> The modifications are straight-forward, thx.
> 
Ping, there are some other patches internally waiting to be
sent out with some issues fixed inside libopenh264 core library, like:

lavc/libopenh264enc: add multi reference encoding support
https://github.com/intel-media-ci/ffmpeg/pull/188/commits/3243f8bd6ffb1ff8fe7fabd7e6bb3a01a11c

Please help to review, appreciate for any comments.

Regards,
Linjie
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v2] avcodec/nvenc: adapt to the new internal encode API

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of
> James Almer
> Sent: Friday, April 10, 2020 02:27
> To: ffmpeg-devel@ffmpeg.org
> Subject: [FFmpeg-devel] [PATCH v2] avcodec/nvenc: adapt to the new
> internal encode API
> 
> Signed-off-by: James Almer 
> ---
> Version with the flush() callback left in place. But it will need the
> changes i originally added to avcodec_flush_buffers() and then removed
> for the latest iteration of this set, in some form or another.
> 
>  libavcodec/nvenc.c  | 78 ++---
>  libavcodec/nvenc.h  |  9 ++---
>  libavcodec/nvenc_h264.c |  6 
>  libavcodec/nvenc_hevc.c |  4 ---
>  4 files changed, 36 insertions(+), 61 deletions(-)
> 
> diff --git a/libavcodec/nvenc.c b/libavcodec/nvenc.c
> index 9a96bf2bba..700a9a7a97 100644
> --- a/libavcodec/nvenc.c
> +++ b/libavcodec/nvenc.c
> @@ -30,6 +30,7 @@
>  #include "libavutil/avassert.h"
>  #include "libavutil/mem.h"
>  #include "libavutil/pixdesc.h"
> +#include "encode.h"
>  #include "internal.h"
> 
> 
> -if (output_ready(avctx, ctx->encoder_flushing)) {
> +if (!frame->buf[0]) {
> +res = ff_encode_get_frame(avctx, frame);
> +if (res < 0 && res != AVERROR_EOF)
> +return res;
> +}
> +
> +res = nvenc_send_frame(avctx, frame);
> +if (res < 0) {
> +if (res != AVERROR(EAGAIN))
> +return res;
> +} else
> +av_frame_unref(frame);

Would it be better to use av_frame_move_ref inside nvenc_upload_frame()
in nvenc_send_frame() instead of add frame reference again?

Didn't verify in nvenc, but such modification leads to some performance 
improvements
for vaapi.

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v2] avcodec/nvenc: adapt to the new internal encode API

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of
> James Almer
> Sent: Friday, April 10, 2020 02:27
> To: ffmpeg-devel@ffmpeg.org
> Subject: [FFmpeg-devel] [PATCH v2] avcodec/nvenc: adapt to the new
> internal encode API
> 
> Signed-off-by: James Almer 
> ---
> Version with the flush() callback left in place. But it will need the
> changes i originally added to avcodec_flush_buffers() and then removed
> for the latest iteration of this set, in some form or another.
> 
>  libavcodec/nvenc.c  | 78 ++---
>  libavcodec/nvenc.h  |  9 ++---
>  libavcodec/nvenc_h264.c |  6 
>  libavcodec/nvenc_hevc.c |  4 ---
>  4 files changed, 36 insertions(+), 61 deletions(-)
> 
> diff --git a/libavcodec/nvenc.c b/libavcodec/nvenc.c
> index 9a96bf2bba..700a9a7a97 100644
> --- a/libavcodec/nvenc.c
> +++ b/libavcodec/nvenc.c
> @@ -30,6 +30,7 @@
>  #include "libavutil/avassert.h"
>  #include "libavutil/mem.h"
>  #include "libavutil/pixdesc.h"
> +#include "encode.h"
>  #include "internal.h"
> 
>  #define CHECK_CU(x) FF_CUDA_CHECK_DL(avctx, dl_fn->cuda_dl, x)
> @@ -1491,6 +1492,8 @@ av_cold int
> ff_nvenc_encode_close(AVCodecContext *avctx)
>  av_freep(>surfaces);
>  ctx->nb_surfaces = 0;
> 
> +av_frame_free(>frame);
> +
>  if (ctx->nvencoder) {
>  p_nvenc->nvEncDestroyEncoder(ctx->nvencoder);
> 
> @@ -1544,6 +1547,10 @@ av_cold int
> ff_nvenc_encode_init(AVCodecContext *avctx)
>  ctx->data_pix_fmt = avctx->pix_fmt;
>  }
> 
> +ctx->frame = av_frame_alloc();
> +if (!ctx->frame)
> +return AVERROR(ENOMEM);
> +
>  if ((ret = nvenc_load_libraries(avctx)) < 0)
>  return ret;
> 
> @@ -1881,9 +1888,7 @@ static int process_output_surface(AVCodecContext
> *avctx, AVPacket *pkt, NvencSur
>  goto error;
>  }
> 
> -res = pkt->data ?
> -ff_alloc_packet2(avctx, pkt, lock_params.bitstreamSizeInBytes,
> lock_params.bitstreamSizeInBytes) :
> -av_new_packet(pkt, lock_params.bitstreamSizeInBytes);
> +res = av_new_packet(pkt, lock_params.bitstreamSizeInBytes);
> 

Is there any specific reason to drop ff_alloc_packet2?

This function calls av_new_packet() with a return check while !pkt->data,
which seems to have an identical logic with this modification, so how about:
res = ff_alloc_packet2(avctx, pkt, lock_params.bitstreamSizeInBytes, 
lock_params.bitstreamSizeInBytes)

Honestly I've got one question on how to choose between:
1. ff_alloc_packet2(avctx, pkt, lock_params.bitstreamSizeInBytes, 
lock_params.bitstreamSizeInBytes);// seems to equals av_new_packet while 
!pkt->data
2. ff_alloc_packet2(avctx, pkt, lock_params.bitstreamSizeInBytes, 0);// seems 
to be the majority usage of encoder, which uses av_fast_padded_malloc();
3. av_new_packet(pkt, lock_params.bitstreamSizeInBytes);// basic

- Linjie

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 00/10] Patch set for the enhancement of libopenh264 encoder

[Fu, Linjie]

> From: Fu, Linjie 
> Sent: Monday, April 6, 2020 19:15
> To: ffmpeg-devel@ffmpeg.org
> Cc: Fu, Linjie 
> Subject: [PATCH 00/10] Patch set for the enhancement of libopenh264
> encoder
> 
> Docs will be provided later.
> 
> Linjie Fu (10):
>   lavc/libopenh264enc: Add default qmin/qmax support
> [v2] fix the av_clip logic for iMinQp.
>   lavc/libopenh264enc: fix the if-else coding style
>   lavc/libopenh264enc: add default gop size and bit rate
>   lavc/libopenh264enc: add bit rate control select support
>   lavc/libopenh264enc: prompt slice number changing according to cpus
>   lavc/libopenh264enc: set slice_mode option to deprecated
>   lavc/libopenh264enc: separate svc_encode_init() into several functions
> [v2] remove forward declarations.
> New:
>   lavc/libopenh264enc: add profile high option support
>   lavc/libopenh264enc: allow specifying the profile through
> AVCodecContext
>   lavc/libopenh264enc: replace cabac option with coder
> 
>  libavcodec/libopenh264enc.c | 361 ++--
> 
>  1 file changed, 246 insertions(+), 115 deletions(-)

Ping for this set. 
The modifications are straight-forward, thx.

- Linjie


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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 00/10] Patch set for the enhancement of libopenh264 encoder

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of
> Anton Khirnov
> Sent: Monday, April 6, 2020 17:00
> To: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 00/10] Patch set for the enhancement
> of libopenh264 encoder
> 
> Quoting Linjie Fu (2020-04-05 12:49:38)
> > > From: "Linjie Fu" 
> > > Sent Time: 2020-04-03 23:12:20 (Friday)
> > > To: ffmpeg-devel@ffmpeg.org
> > > Cc: "Linjie Fu" 
> > > Subject: [FFmpeg-devel] [PATCH 00/10] Patch set for the enhancement
> of libopenh264 encoder
> > >
> > > Docs will be provided later.
> > >
> > > Linjie Fu (10):
> > >   lavc/libopenh264enc: Add default qmin/qmax support
> > > [v2] fix the av_clip logic for iMinQp.
> > >   lavc/libopenh264enc: fix the if-else coding style
> > >   lavc/libopenh264enc: add default gop size and bit rate
> > >   lavc/libopenh264enc: add bit rate control select support
> > >   lavc/libopenh264enc: prompt slice number changing according to cpus
> > >   lavc/libopenh264enc: set slice_mode option to deprecated
> > >   lavc/libopenh264enc: separate svc_encode_init() into several functions
> > > [v2] remove forward declarations.
> > > New:
> > >   lavc/libopenh264enc: add profile high option support
> > >   lavc/libopenh264enc: allow specifying the profile through
> AVCodecContext
> > >   lavc/libopenh264enc: replace cabac option with coder
> > >
> > >  libavcodec/libopenh264enc.c | 361
> ++--
> > >  1 file changed, 246 insertions(+), 115 deletions(-)
> > >
> > Ping for this patch set.
> 
> Would you please send your patchsets as single threads (git send-email
> does that by default unless you use --no-thread). That makes it
> significantly easier to find patches that go together.
> 
Updated, thanks.

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] avcodec/amfenc: adapt to the new internal encode API

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of
> James Almer
> Sent: Saturday, April 4, 2020 06:19
> To: ffmpeg-devel@ffmpeg.org
> Subject: [FFmpeg-devel] [PATCH] avcodec/amfenc: adapt to the new
> internal encode API
> 
> Signed-off-by: James Almer 
> ---
>  libavcodec/amfenc.c  | 45 
>  libavcodec/amfenc.h  |  2 ++
>  libavcodec/amfenc_h264.c |  1 -
>  libavcodec/amfenc_hevc.c |  1 -
>  4 files changed, 25 insertions(+), 24 deletions(-)
> 
> diff --git a/libavcodec/amfenc.c b/libavcodec/amfenc.c
> index 876fddd2b6..d318c21978 100644
> --- a/libavcodec/amfenc.c
> +++ b/libavcodec/amfenc.c
> @@ -33,6 +33,7 @@
>  #include "libavutil/time.h"
> 
>  #include "amfenc.h"
> +#include "encode.h"
>  #include "internal.h"
> 
>  #if CONFIG_D3D11VA
> @@ -112,6 +113,10 @@ static int amf_load_library(AVCodecContext *avctx)
>  AMFQueryVersion_Fn version_fun;
>  AMF_RESULT res;
> 
> +ctx->frame = av_frame_alloc();
> +if (!ctx->frame) {
> +return AVERROR(ENOMEM);
> +}
>  ctx->delayed_frame = av_frame_alloc();
>  if (!ctx->delayed_frame) {
>  return AVERROR(ENOMEM);
> @@ -401,6 +406,7 @@ int av_cold ff_amf_encode_close(AVCodecContext
> *avctx)
>  ctx->factory = NULL;
>  ctx->version = 0;
>  ctx->delayed_drain = 0;
> +av_frame_free(>frame);
>  av_frame_free(>delayed_frame);
>  av_fifo_freep(>timestamp_list);
> 
> @@ -588,17 +594,27 @@ static void
> amf_release_buffer_with_frame_ref(AMFBuffer
> *frame_ref_storage_buffe
>  frame_ref_storage_buffer->pVtbl->Release(frame_ref_storage_buffer);
>  }
> 
> -int ff_amf_send_frame(AVCodecContext *avctx, const AVFrame *frame)
> +int ff_amf_receive_packet(AVCodecContext *avctx, AVPacket *avpkt)
Would be better to remove the declaration in amfenc.h as well since the function
has been removed already?

- Linjie

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v1 2/2] avcodec/mpeg12enc: Support mpeg2 encoder profile with const options

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of
> Limin Wang
> Sent: Friday, April 3, 2020 22:55
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v1 2/2] avcodec/mpeg12enc: Support
> mpeg2 encoder profile with const options
> 
> On Fri, Apr 03, 2020 at 04:25:15PM +0200, Hendrik Leppkes wrote:
> > On Fri, Apr 3, 2020 at 3:12 PM  wrote:
> > >
> > > From: Limin Wang 
> > >
> > > make setting profile more user friendly
> > >
> >
> > Please make sure API users can still set it through avctx->profile,
> > otherwise this would be an API break.
> 
> Right, I'll update the patch to check avctx->profile != unknown, then
> overwrite
> the s->profile with it to void API break.
> 
Would it be better to determine the profile with a priority like:
1.s->profile; then
2. avctx->profile; then
3. a default profile;
in case both avctx->profile and s->profile are set?

If user set a profile explicitly (through either option in cmdline or 
av_opt_set()), 
above priority seems to be natural. (and libx264 [1] has the same logic)

- Linjie

[1] https://github.com/FFmpeg/FFmpeg/blob/master/libavcodec/libx264.c#L806

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 7/7] lavc/libopenh264enc: separate svc_encode_init() into several functions

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of Fu,
> Linjie
> Sent: Tuesday, March 31, 2020 23:59
> To: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 7/7] lavc/libopenh264enc: separate
> svc_encode_init() into several functions
> 
> > From: ffmpeg-devel  On Behalf Of
> > James Almer
> > Sent: Tuesday, March 31, 2020 23:46
> > To: ffmpeg-devel@ffmpeg.org
> > Subject: Re: [FFmpeg-devel] [PATCH 7/7] lavc/libopenh264enc: separate
> > svc_encode_init() into several functions
> >
> > On 3/31/2020 12:33 PM, Linjie Fu wrote:
> > > Separate the initialization procedure into different functions.
> > >
> > > Make it more readable and easier to be extended.
> > >
> > > Signed-off-by: Linjie Fu 
> > > ---
> > >  libavcodec/libopenh264enc.c | 302 +++---
> --
> > 
> > >  1 file changed, 186 insertions(+), 116 deletions(-)
> > >
> > > diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c
> > > index 692aba9..ab54454 100644
> > > --- a/libavcodec/libopenh264enc.c
> > > +++ b/libavcodec/libopenh264enc.c
> > > @@ -85,6 +85,11 @@ static const AVOption options[] = {
> > >  { NULL }
> > >  };
> > >
> > > +static av_cold int svc_encode_init_profile(AVCodecContext *avctx,
> > SEncParamExt *param);
> > > +static av_cold int svc_encode_init_rate_control(AVCodecContext
> *avctx,
> > SEncParamExt *param);
> > > +static av_cold int svc_encode_init_spatial_layer(AVCodecContext
> *avctx,
> > SEncParamExt *param);
> > > +static av_cold int svc_encode_init_params(AVCodecContext *avctx,
> > SEncParamExt *param);
> >
> > Why use forward declarations? Just put the functions right above
> > svc_encode_init().
> 
> It's seems to be easier/clearer for review the diffs, otherwise this would be
> mixed
> up.

Ping for the rest patches in this patch set.
Will update V2 soon with the forward declarations removed as suggested.

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] configure: fix build issue of vf_dnn_processing.c when --disable-swscale

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of
> Guo, Yejun
> Sent: Thursday, April 2, 2020 11:37
> To: ffmpeg-devel@ffmpeg.org
> Cc: Guo, Yejun 
> Subject: [FFmpeg-devel] [PATCH] configure: fix build issue of
> vf_dnn_processing.c when --disable-swscale
> 
> vf_dnn_processing.c recently changed to use swscale to trasfer data
> between AVFrame and dnn model.
> 
> Signed-off-by: Guo, Yejun 
> ---
>  configure | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/configure b/configure
> index 21827ee..56888a5 100755
> --- a/configure
> +++ b/configure
> @@ -3498,6 +3498,7 @@ derain_filter_select="dnn"
>  deshake_filter_select="pixelutils"
>  deshake_opencl_filter_deps="opencl"
>  dilation_opencl_filter_deps="opencl"
> +dnn_processing_filter_deps="swscale"
>  dnn_processing_filter_select="dnn"
>  drawtext_filter_deps="libfreetype"
>  drawtext_filter_suggest="libfontconfig libfribidi"
> --

Looks reasonable, and verified the failure is gone with this patch.

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] lavc/vaapi_decode: fix the build failure when hevc_vaapi is disabled

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of
> Carl Eugen Hoyos
> Sent: Thursday, April 2, 2020 03:22
> To: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH] lavc/vaapi_decode: fix the build failure
> when hevc_vaapi is disabled
> 
> Am Mi., 1. Apr. 2020 um 17:24 Uhr schrieb Linjie Fu :
> >
> > Verified with ./configure --enable-vaapi --disable-hwaccel=hevc_vaapi
> >
> > Failure reported in:
> > http://fate.ffmpeg.org/report.cgi?time=20200401135031=x86_64-
> archlinux-gcc-random
> >
> > Signed-off-by: Linjie Fu 
> > ---
> >  libavcodec/vaapi_decode.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/libavcodec/vaapi_decode.c b/libavcodec/vaapi_decode.c
> > index 54a0ecb..06916cc 100644
> > --- a/libavcodec/vaapi_decode.c
> > +++ b/libavcodec/vaapi_decode.c
> > @@ -383,6 +383,7 @@ static const struct {
> > H264ConstrainedBaseline),
> >  MAP(H264,H264_MAIN,   H264Main),
> >  MAP(H264,H264_HIGH,   H264High),
> > +#if CONFIG_HEVC_VAAPI_HWACCEL
> >  #if VA_CHECK_VERSION(0, 37, 0)
> 
> Can these lines be joined?
> Or am I missing something between the chunks?

The complete code contains 2 different libva version checks for compatibility
which didn't show up in the diff chunks:

#if CONFIG_HEVC_VAAPI_HWACCEL
#if VA_CHECK_VERSION(0, 37, 0)
MAP(HEVC,HEVC_MAIN,   HEVCMain),
MAP(HEVC,HEVC_MAIN_10,HEVCMain10  ),
MAP(HEVC,HEVC_MAIN_STILL_PICTURE,
  HEVCMain),
#endif
#if VA_CHECK_VERSION(1, 2, 0)
MAP(HEVC,HEVC_REXT,   None,
 ff_vaapi_parse_hevc_rext_profile ),
#endif
#endif

hence I think maybe no need for a join?

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH]lavfi/deshake_opencl: Do not use bool

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of
> Carl Eugen Hoyos
> Sent: Wednesday, April 1, 2020 04:26
> To: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Subject: [FFmpeg-devel] [PATCH]lavfi/deshake_opencl: Do not use bool
> 
> Hi!
> 
> Attached patch fixes compilation with opencl for powerpc where bool
> always had a meaning different from other architectures. Other
> work-arounds certainly exist but we usually don't like bool anyway.
> Fixes ticket #8591.
> 
> Please comment, Carl Eugen

Looks reasonable, and verified on ppc64 with make V=1 
libavfilter/vf_deshake_opencl.o ,
the compile errors of "incompatible types when assigning to type ‘__vector 
__bool int’ from type ‘int’ "
are gone after applying this patch.

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/7] lavc/libopenh264enc: Add default qmin/qmax support

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of
> James Almer
> Sent: Tuesday, March 31, 2020 23:44
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH 1/7] lavc/libopenh264enc: Add default
> qmin/qmax support
> 
> On 3/31/2020 12:31 PM, Linjie Fu wrote:
> > Set default QP range to (1, 51) instead of (2, 32).
> >
> > QP = 0 is not well supported currently in libopenh264. If iMaxQp/iMinQp
> > equals 0, the QP range would be changed unexpectedly inside libopenh264
> > with a warning:
> >
> > Warning:Change QP Range from(0,51) to (12,42)
> >
> > Signed-off-by: Linjie Fu 
> > ---
> > [1]
>  encoder_ext.cpp#L375>
> > [2] 
> >
> >  libavcodec/libopenh264enc.c | 10 ++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c
> > index ae6d17c..7cd1efe 100644
> > --- a/libavcodec/libopenh264enc.c
> > +++ b/libavcodec/libopenh264enc.c
> > @@ -135,6 +135,9 @@ FF_ENABLE_DEPRECATION_WARNINGS
> >  param.iTargetBitrate = avctx->bit_rate;
> >  param.iMaxBitrate= FFMAX(avctx->rc_max_rate, avctx-
> >bit_rate);
> >  param.iRCMode= RC_QUALITY_MODE;
> > +// QP = 0 is not well supported, so default to (1, 51)
> > +param.iMaxQp = avctx->qmax >= 0 ? 
> > av_clip(avctx->qmax, 1,
> 51) : 51;
> > +param.iMinQp = avctx->qmin >= 0 ? 
> > av_clip(avctx->qmin, 1,
> avctx->qmax) : 1;
> 
> Should be param.iMaxQp. avctx->qmax may be <= 0.
> 
Fixed locally, thanks for pointing this out and the elaborations in IRC.

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 7/7] lavc/libopenh264enc: separate svc_encode_init() into several functions

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of
> James Almer
> Sent: Tuesday, March 31, 2020 23:46
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH 7/7] lavc/libopenh264enc: separate
> svc_encode_init() into several functions
> 
> On 3/31/2020 12:33 PM, Linjie Fu wrote:
> > Separate the initialization procedure into different functions.
> >
> > Make it more readable and easier to be extended.
> >
> > Signed-off-by: Linjie Fu 
> > ---
> >  libavcodec/libopenh264enc.c | 302 +++-
> 
> >  1 file changed, 186 insertions(+), 116 deletions(-)
> >
> > diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c
> > index 692aba9..ab54454 100644
> > --- a/libavcodec/libopenh264enc.c
> > +++ b/libavcodec/libopenh264enc.c
> > @@ -85,6 +85,11 @@ static const AVOption options[] = {
> >  { NULL }
> >  };
> >
> > +static av_cold int svc_encode_init_profile(AVCodecContext *avctx,
> SEncParamExt *param);
> > +static av_cold int svc_encode_init_rate_control(AVCodecContext *avctx,
> SEncParamExt *param);
> > +static av_cold int svc_encode_init_spatial_layer(AVCodecContext *avctx,
> SEncParamExt *param);
> > +static av_cold int svc_encode_init_params(AVCodecContext *avctx,
> SEncParamExt *param);
> 
> Why use forward declarations? Just put the functions right above
> svc_encode_init().

It's seems to be easier/clearer for review the diffs, otherwise this would be 
mixed
up.

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 3/3] lavc/vaapi_encode_h265: add h265 tile encoding support

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of Fu,
> Linjie
> Sent: Tuesday, March 24, 2020 10:15
> To: myp...@gmail.com; FFmpeg development discussions and patches
> 
> Subject: Re: [FFmpeg-devel] [PATCH 3/3] lavc/vaapi_encode_h265: add h265
> tile encoding support
> 
> > From: myp...@gmail.com 
> > Sent: Monday, March 23, 2020 22:44
> > To: FFmpeg development discussions and patches  > de...@ffmpeg.org>
> > Cc: Fu, Linjie 
> > Subject: Re: [FFmpeg-devel] [PATCH 3/3] lavc/vaapi_encode_h265: add
> h265
> > tile encoding support
> >
> > On Mon, Mar 23, 2020 at 10:30 PM Linjie Fu  wrote:
> > >
> > > Default to enable uniform_spacing_flag. Guess level by the tile
> > > rows/cols. Supported for ICL+ platforms.
> > >
> > > Also add documentations.
> > >
> > > To encode with 4 rows 2 columns:
> > > ffmpeg ... -c:v hevc_vaapi -tile_rows 4 -tile_cols 2 ...
> > >
> > > Signed-off-by: Linjie Fu 
> > > ---
> > >  doc/encoders.texi  |  8 
> > >  libavcodec/vaapi_encode_h265.c | 36
> > +++-
> > >  2 files changed, 43 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/doc/encoders.texi b/doc/encoders.texi
> > > index e23b6b3..2dc5cd4 100644
> > > --- a/doc/encoders.texi
> > > +++ b/doc/encoders.texi
> > > @@ -3091,6 +3091,14 @@ Include HDR metadata if the input frames have
> it
> > >  messages).
> > >  @end table
> > >
> > > +@item tile_rows
> > > +Selects how many rows of tiles to encode with. For example, 4 tile rows
> > would
> > > +be requested by setting the tile_rows option to 4.
> > > +
> > > +@item tile_cols
> > > +Selects how many columns of tiles to encode with. For example, 5 tile
> > columns
> > > +would be requested by setting the tile_cols option to 5.
> > > +
> > >  @end table
> > >
> > >  @item mjpeg_vaapi
> > > diff --git a/libavcodec/vaapi_encode_h265.c
> > b/libavcodec/vaapi_encode_h265.c
> > > index 97dc5a7..457e3d9 100644
> > > --- a/libavcodec/vaapi_encode_h265.c
> > > +++ b/libavcodec/vaapi_encode_h265.c
> > > @@ -63,6 +63,9 @@ typedef struct VAAPIEncodeH265Context {
> > >  int level;
> > >  int sei;
> > >
> > > +int trows;
> > > +int tcols;
> > > +
> > >  // Derived settings.
> > >  int fixed_qp_idr;
> > >  int fixed_qp_p;
> > > @@ -345,7 +348,7 @@ static int
> > vaapi_encode_h265_init_sequence_params(AVCodecContext *avctx)
> > >
> > >  level = ff_h265_guess_level(ptl, avctx->bit_rate,
> > >  ctx->surface_width, 
> > > ctx->surface_height,
> > > -ctx->nb_slices, 1, 1,
> > > +ctx->nb_slices, ctx->tile_rows, 
> > > ctx->tile_cols,
> > >  (ctx->b_per_p > 0) + 1);
> > >  if (level) {
> > >  av_log(avctx, AV_LOG_VERBOSE, "Using level %s.\n", level-
> >name);
> > > @@ -558,6 +561,20 @@ static int
> > vaapi_encode_h265_init_sequence_params(AVCodecContext *avctx)
> > >
> > >  pps->pps_loop_filter_across_slices_enabled_flag = 1;
> > >
> > > +if (ctx->tile_rows && ctx->tile_cols) {
> > > +pps->tiles_enabled_flag = 1;
> > > +pps->uniform_spacing_flag   = 1;
> > > +
> > > +pps->num_tile_rows_minus1   = ctx->tile_rows - 1;
> > > +pps->num_tile_columns_minus1= ctx->tile_cols - 1;
> > > +
> > > +pps->loop_filter_across_tiles_enabled_flag = 1;
> > > +
> > > +for (i = 0; i <= pps->num_tile_rows_minus1; i++)
> > > +pps->row_height_minus1[i]   = ctx->row_height[i] - 1;
> > > +for (i = 0; i <= pps->num_tile_columns_minus1; i++)
> > > +pps->column_width_minus1[i] = ctx->col_width[i] - 1;
> > > +}
> > >
> > >  // Fill VAAPI parameter buffers.
> > >
> > > @@ -666,6 +683,13 @@ static int
> > vaapi_encode_h265_init_sequence_params(AVCodecContext *avctx)
> > >  },
> > >  };
> > >
> > > +if (pps->tiles_enabled_flag) {
> > > +for (i = 0; i <

Re: [FFmpeg-devel] [PATCH 3/3] lavc/vaapi_encode_h265: add h265 tile encoding support

[Fu, Linjie]

> From: myp...@gmail.com 
> Sent: Monday, March 23, 2020 22:44
> To: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Cc: Fu, Linjie 
> Subject: Re: [FFmpeg-devel] [PATCH 3/3] lavc/vaapi_encode_h265: add h265
> tile encoding support
> 
> On Mon, Mar 23, 2020 at 10:30 PM Linjie Fu  wrote:
> >
> > Default to enable uniform_spacing_flag. Guess level by the tile
> > rows/cols. Supported for ICL+ platforms.
> >
> > Also add documentations.
> >
> > To encode with 4 rows 2 columns:
> > ffmpeg ... -c:v hevc_vaapi -tile_rows 4 -tile_cols 2 ...
> >
> > Signed-off-by: Linjie Fu 
> > ---
> >  doc/encoders.texi  |  8 
> >  libavcodec/vaapi_encode_h265.c | 36
> +++-
> >  2 files changed, 43 insertions(+), 1 deletion(-)
> >
> > diff --git a/doc/encoders.texi b/doc/encoders.texi
> > index e23b6b3..2dc5cd4 100644
> > --- a/doc/encoders.texi
> > +++ b/doc/encoders.texi
> > @@ -3091,6 +3091,14 @@ Include HDR metadata if the input frames have it
> >  messages).
> >  @end table
> >
> > +@item tile_rows
> > +Selects how many rows of tiles to encode with. For example, 4 tile rows
> would
> > +be requested by setting the tile_rows option to 4.
> > +
> > +@item tile_cols
> > +Selects how many columns of tiles to encode with. For example, 5 tile
> columns
> > +would be requested by setting the tile_cols option to 5.
> > +
> >  @end table
> >
> >  @item mjpeg_vaapi
> > diff --git a/libavcodec/vaapi_encode_h265.c
> b/libavcodec/vaapi_encode_h265.c
> > index 97dc5a7..457e3d9 100644
> > --- a/libavcodec/vaapi_encode_h265.c
> > +++ b/libavcodec/vaapi_encode_h265.c
> > @@ -63,6 +63,9 @@ typedef struct VAAPIEncodeH265Context {
> >  int level;
> >  int sei;
> >
> > +int trows;
> > +int tcols;
> > +
> >  // Derived settings.
> >  int fixed_qp_idr;
> >  int fixed_qp_p;
> > @@ -345,7 +348,7 @@ static int
> vaapi_encode_h265_init_sequence_params(AVCodecContext *avctx)
> >
> >  level = ff_h265_guess_level(ptl, avctx->bit_rate,
> >  ctx->surface_width, 
> > ctx->surface_height,
> > -ctx->nb_slices, 1, 1,
> > +ctx->nb_slices, ctx->tile_rows, 
> > ctx->tile_cols,
> >  (ctx->b_per_p > 0) + 1);
> >  if (level) {
> >  av_log(avctx, AV_LOG_VERBOSE, "Using level %s.\n", 
> > level->name);
> > @@ -558,6 +561,20 @@ static int
> vaapi_encode_h265_init_sequence_params(AVCodecContext *avctx)
> >
> >  pps->pps_loop_filter_across_slices_enabled_flag = 1;
> >
> > +if (ctx->tile_rows && ctx->tile_cols) {
> > +pps->tiles_enabled_flag = 1;
> > +pps->uniform_spacing_flag   = 1;
> > +
> > +pps->num_tile_rows_minus1   = ctx->tile_rows - 1;
> > +pps->num_tile_columns_minus1= ctx->tile_cols - 1;
> > +
> > +pps->loop_filter_across_tiles_enabled_flag = 1;
> > +
> > +for (i = 0; i <= pps->num_tile_rows_minus1; i++)
> > +pps->row_height_minus1[i]   = ctx->row_height[i] - 1;
> > +for (i = 0; i <= pps->num_tile_columns_minus1; i++)
> > +pps->column_width_minus1[i] = ctx->col_width[i] - 1;
> > +}
> >
> >  // Fill VAAPI parameter buffers.
> >
> > @@ -666,6 +683,13 @@ static int
> vaapi_encode_h265_init_sequence_params(AVCodecContext *avctx)
> >  },
> >  };
> >
> > +if (pps->tiles_enabled_flag) {
> > +for (i = 0; i <= vpic->num_tile_rows_minus1; i++)
> > +vpic->row_height_minus1[i]   = pps->row_height_minus1[i];
> > +for (i = 0; i <= vpic->num_tile_columns_minus1; i++)
> > +vpic->column_width_minus1[i] = pps->column_width_minus1[i];
> > +}
> > +
> >  return 0;
> >  }
> >
> > @@ -1181,6 +1205,11 @@ static av_cold int
> vaapi_encode_h265_init(AVCodecContext *avctx)
> >  if (priv->qp > 0)
> >  ctx->explicit_qp = priv->qp;
> >
> > +if (priv->trows && priv->tcols) {
> > +ctx->tile_rows = priv->trows;
> > +ctx->tile_cols = priv->tcols;
> > +}
> > +
> > 

Re: [FFmpeg-devel] [PATCH 1/3] lavformat: Prepare to make avio_enum_protocols const correct

[Fu, Linjie]

> From: ffmpeg-devel  On Behalf Of
> Andreas Rheinhardt
> Sent: Wednesday, March 18, 2020 21:29
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH 1/3] lavformat: Prepare to make
> avio_enum_protocols const correct
> 
> Fu, Linjie:
> Yes, updating the callers is necessary (as I already said in the commit
> message), but easy. The typical old call is (the ... usually includes a
> loop):
> void *opaque = NULL;
> char *name;
> 
> ...
> name = avio_enum_protocols(, flag);
> ...
> 
> Changing the type of opaque to const void* will make this code
> const-correct. (The opaque now points to one of the constant pointers in
> the url_protocols array, so the user having a pointer to nonconst to
> them is wrong.)
> 
> - Andreas
> 
> PS: The reason that the callers need to update their code is that there
> is no automatic conversion from type ** to const type **, so that an
> explicit cast is required. Such an automatic conversion would be
> dangerous as it could be used to remove const from any pointer without a
> cast. See http://c-faq.com/ansi/constmismatch.html
> PPS: There would be another way to get rid of this warning: Instead of
> making the void **opaque store a pointer to a pointer to a const entry
> of the url_protocols array one make the opaque point to a value of type
> uintptr_t that stores an index in the url_protocols array. This would
> still require a cast, but it would not cast const away. But I don't
> really like this approach, as forcing the user to use pointers to const
> void makes it clearer that the user is not to meddle with the opaque.

No more concerns, this fix looks reasonable.
Thanks for the kind elaborations.

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".