| Message ID | 20230825100115.393960-1-stefasab@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] lavc/libx264: enable x4->params.analyse.b_fast_pskip if mb_info is set | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
Hi
-----Original Message-----
From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Stefano Sabatini
Sent: Friday, August 25, 2023 12:01 PM
To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Cc: Stefano Sabatini <stefasab@gmail.com>
Subject: [EXTERNAL] [FFmpeg-devel] [PATCH] lavc/libx264: enable x4->params.analyse.b_fast_pskip if mb_info is set
CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
x4->params.analyse.b_fast_pskip should only be forced in case mb_info
is set.
Fix output change introduced in 418c954e318.
---
libavcodec/libx264.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c index 1a7dc7bdd5..a2877d7f75 100644
--- a/libavcodec/libx264.c
+++ b/libavcodec/libx264.c
@@ -1190,7 +1190,9 @@ FF_ENABLE_DEPRECATION_WARNINGS
}
x4->params.analyse.b_mb_info = x4->mb_info;
- x4->params.analyse.b_fast_pskip = 1;
+ if (x4->mb_info) {
+ x4->params.analyse.b_fast_pskip = x4->mb_info;
+ }
// update AVCodecContext with x264 parameters
avctx->has_b_frames = x4->params.i_bframe ?
--
2.25.1
Sorry for the delay. I agree, this was missing in the patch.
Best
Elias
On Thu, 31 Aug 2023 at 15:12, Carotti, Elias via ffmpeg-devel < ffmpeg-devel@ffmpeg.org> wrote: > Hi > > -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Stefano > Sabatini > Sent: Friday, August 25, 2023 12:01 PM > To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org> > Cc: Stefano Sabatini <stefasab@gmail.com> > Subject: [EXTERNAL] [FFmpeg-devel] [PATCH] lavc/libx264: enable > x4->params.analyse.b_fast_pskip if mb_info is set > > CAUTION: This email originated from outside of the organization. Do not > click links or open attachments unless you can confirm the sender and know > the content is safe. > > > > x4->params.analyse.b_fast_pskip should only be forced in case mb_info > is set. > > Fix output change introduced in 418c954e318. > --- > libavcodec/libx264.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c index > 1a7dc7bdd5..a2877d7f75 100644 > --- a/libavcodec/libx264.c > +++ b/libavcodec/libx264.c > @@ -1190,7 +1190,9 @@ FF_ENABLE_DEPRECATION_WARNINGS > } > > x4->params.analyse.b_mb_info = x4->mb_info; > - x4->params.analyse.b_fast_pskip = 1; > + if (x4->mb_info) { > + x4->params.analyse.b_fast_pskip = x4->mb_info; > + } > > // update AVCodecContext with x264 parameters > avctx->has_b_frames = x4->params.i_bframe ? > -- > 2.25.1 > > > Sorry for the delay. I agree, this was missing in the patch. > Best > Elias > What does this patch actually do? Kieran
On Thu, 31 Aug 2023 at 15:30, Kieran Kunhya <kierank@obe.tv> wrote: > > > On Thu, 31 Aug 2023 at 15:12, Carotti, Elias via ffmpeg-devel < > ffmpeg-devel@ffmpeg.org> wrote: > >> Hi >> >> -----Original Message----- >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of >> Stefano Sabatini >> Sent: Friday, August 25, 2023 12:01 PM >> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org> >> Cc: Stefano Sabatini <stefasab@gmail.com> >> Subject: [EXTERNAL] [FFmpeg-devel] [PATCH] lavc/libx264: enable >> x4->params.analyse.b_fast_pskip if mb_info is set >> >> CAUTION: This email originated from outside of the organization. Do not >> click links or open attachments unless you can confirm the sender and know >> the content is safe. >> >> >> >> x4->params.analyse.b_fast_pskip should only be forced in case mb_info >> is set. >> >> Fix output change introduced in 418c954e318. >> --- >> libavcodec/libx264.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c index >> 1a7dc7bdd5..a2877d7f75 100644 >> --- a/libavcodec/libx264.c >> +++ b/libavcodec/libx264.c >> @@ -1190,7 +1190,9 @@ FF_ENABLE_DEPRECATION_WARNINGS >> } >> >> x4->params.analyse.b_mb_info = x4->mb_info; >> - x4->params.analyse.b_fast_pskip = 1; >> + if (x4->mb_info) { >> + x4->params.analyse.b_fast_pskip = x4->mb_info; >> + } >> >> // update AVCodecContext with x264 parameters >> avctx->has_b_frames = x4->params.i_bframe ? >> -- >> 2.25.1 >> >> >> Sorry for the delay. I agree, this was missing in the patch. >> Best >> Elias >> > > What does this patch actually do? > > Kieran > In particular why are you turning on fast_pskip silently based on a completely different setting? Kieran
On date Thursday 2023-08-31 15:38:09 +0100, Kieran Kunhya wrote: > On Thu, 31 Aug 2023 at 15:30, Kieran Kunhya <kierank@obe.tv> wrote: > > > > > > > On Thu, 31 Aug 2023 at 15:12, Carotti, Elias via ffmpeg-devel < > > ffmpeg-devel@ffmpeg.org> wrote: > > > >> Hi > >> > >> -----Original Message----- > >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > >> Stefano Sabatini > >> Sent: Friday, August 25, 2023 12:01 PM > >> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org> > >> Cc: Stefano Sabatini <stefasab@gmail.com> > >> Subject: [EXTERNAL] [FFmpeg-devel] [PATCH] lavc/libx264: enable > >> x4->params.analyse.b_fast_pskip if mb_info is set > >> > >> CAUTION: This email originated from outside of the organization. Do not > >> click links or open attachments unless you can confirm the sender and know > >> the content is safe. > >> > >> > >> > >> x4->params.analyse.b_fast_pskip should only be forced in case mb_info > >> is set. > >> > >> Fix output change introduced in 418c954e318. > >> --- > >> libavcodec/libx264.c | 4 +++- > >> 1 file changed, 3 insertions(+), 1 deletion(-) > >> > >> diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c index > >> 1a7dc7bdd5..a2877d7f75 100644 > >> --- a/libavcodec/libx264.c > >> +++ b/libavcodec/libx264.c > >> @@ -1190,7 +1190,9 @@ FF_ENABLE_DEPRECATION_WARNINGS > >> } > >> > >> x4->params.analyse.b_mb_info = x4->mb_info; > >> - x4->params.analyse.b_fast_pskip = 1; > >> + if (x4->mb_info) { > >> + x4->params.analyse.b_fast_pskip = x4->mb_info; > >> + } > >> > >> // update AVCodecContext with x264 parameters > >> avctx->has_b_frames = x4->params.i_bframe ? > >> -- > >> 2.25.1 > >> > >> > >> Sorry for the delay. I agree, this was missing in the patch. > >> Best > >> Elias > >> > > > > What does this patch actually do? > > > > Kieran > > > > In particular why are you turning on fast_pskip silently based on a > completely different setting? The patch is fixing the regression introduced by the unconditional setting of b_fast_pskip. Now the question is if it makes sense to set mb_info without b_fast_pskip (in both case this should be probably documented). @Elias can you comment about the mb_info/b_fast_pskip use case?
On Thu, 2023-08-31 at 19:09 +0200, Stefano Sabatini wrote: > <snip> > > > In particular why are you turning on fast_pskip silently based on a > > completely different setting? > > The patch is fixing the regression introduced by the unconditional > setting of b_fast_pskip. > > Now the question is if it makes sense to set mb_info without > b_fast_pskip (in both case this should be probably documented). > > @Elias can you comment about the mb_info/b_fast_pskip use case? Sorry again for the delay in responding. We can safely remove it altogether. It's true we don't need to set it along with mb_info. However, it doesn't do any harm, since fast_pskip is by default set to true by libx264 and only turned off either explicitly by the user, or when using the placebo preset, or when doing lossless encoding (constant QP == 0.) So, I agree, let's remove these three lines. Elias NICE SRL, viale Monte Grappa 3/5, 20124 Milano, Italia, Registro delle Imprese di Milano Monza Brianza Lodi REA n. 2096882, Capitale Sociale: 10.329,14 EUR i.v., Cod. Fisc. e P.IVA 01133050052, Societa con Socio Unico
On date Saturday 2023-09-02 09:20:08 +0000, Carotti, Elias wrote: > On Thu, 2023-08-31 at 19:09 +0200, Stefano Sabatini wrote: > > > <snip> > > > > > In particular why are you turning on fast_pskip silently based on a > > > completely different setting? > > > > The patch is fixing the regression introduced by the unconditional > > setting of b_fast_pskip. > > > > Now the question is if it makes sense to set mb_info without > > b_fast_pskip (in both case this should be probably documented). > > > > @Elias can you comment about the mb_info/b_fast_pskip use case? > > Sorry again for the delay in responding. > We can safely remove it altogether. It's true we don't need to set it > along with mb_info. > However, it doesn't do any harm, since fast_pskip is by default set to > true by libx264 and only turned off either explicitly by the user, or > when using the placebo preset, or when doing lossless encoding > (constant QP == 0.) > So, I agree, let's remove these three lines. Thanks, updated.
-----Original Message----- From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Stefano Sabatini Sent: Saturday, September 2, 2023 5:45 PM To: ffmpeg-devel@ffmpeg.org Cc: kierank@obe.tv; Carotti, Elias <eliascrt@amazon.it> Subject: RE: [EXTERNAL] [FFmpeg-devel] [PATCH] lavc/libx264: enable x4->params.analyse.b_fast_pskip if mb_info is set CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe. On date Saturday 2023-09-02 09:20:08 +0000, Carotti, Elias wrote: > On Thu, 2023-08-31 at 19:09 +0200, Stefano Sabatini wrote: > > > <snip> > > > > > In particular why are you turning on fast_pskip silently based on > > > a completely different setting? > > > > The patch is fixing the regression introduced by the unconditional > > setting of b_fast_pskip. > > > > Now the question is if it makes sense to set mb_info without > > b_fast_pskip (in both case this should be probably documented). > > > > @Elias can you comment about the mb_info/b_fast_pskip use case? > > Sorry again for the delay in responding. > We can safely remove it altogether. It's true we don't need to set it > along with mb_info. > However, it doesn't do any harm, since fast_pskip is by default set to > true by libx264 and only turned off either explicitly by the user, or > when using the placebo preset, or when doing lossless encoding > (constant QP == 0.) So, I agree, let's remove these three lines. Thanks, updated. Ok for me Elias NICE SRL, viale Monte Grappa 3/5, 20124 Milano, Italia, Registro delle Imprese di Milano Monza Brianza Lodi REA n. 2096882, Capitale Sociale: 10.329,14 EUR i.v., Cod. Fisc. e P.IVA 01133050052, Societa con Socio Unico
On date Tuesday 2023-09-05 12:43:35 +0000, ffmpeg-devel Mailing List wrote: [...] > On date Saturday 2023-09-02 09:20:08 +0000, Carotti, Elias wrote: > > On Thu, 2023-08-31 at 19:09 +0200, Stefano Sabatini wrote: > > > > > <snip> > > > > > > > In particular why are you turning on fast_pskip silently based on > > > > a completely different setting? > > > > > > The patch is fixing the regression introduced by the unconditional > > > setting of b_fast_pskip. > > > > > > Now the question is if it makes sense to set mb_info without > > > b_fast_pskip (in both case this should be probably documented). > > > > > > @Elias can you comment about the mb_info/b_fast_pskip use case? > > > > Sorry again for the delay in responding. > > We can safely remove it altogether. It's true we don't need to set it > > along with mb_info. > > However, it doesn't do any harm, since fast_pskip is by default set to > > true by libx264 and only turned off either explicitly by the user, or > > when using the placebo preset, or when doing lossless encoding > > (constant QP == 0.) So, I agree, let's remove these three lines. > > > > Thanks, updated. > > Ok for me Applied, thanks.
diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c index 1a7dc7bdd5..a2877d7f75 100644 --- a/libavcodec/libx264.c +++ b/libavcodec/libx264.c @@ -1190,7 +1190,9 @@ FF_ENABLE_DEPRECATION_WARNINGS } x4->params.analyse.b_mb_info = x4->mb_info; - x4->params.analyse.b_fast_pskip = 1; + if (x4->mb_info) { + x4->params.analyse.b_fast_pskip = x4->mb_info; + } // update AVCodecContext with x264 parameters avctx->has_b_frames = x4->params.i_bframe ?