diff mbox series

[FFmpeg-devel] fftools/ffmpeg_opt: restore to use codec_info_nb_frames to calculate score

Message ID 20210124130033.1086545-1-linjie.justin.fu@gmail.com
State New
Headers show
Series [FFmpeg-devel] fftools/ffmpeg_opt: restore to use codec_info_nb_frames to calculate score
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Linjie Fu Jan. 24, 2021, 1 p.m. UTC
From: Linjie Fu <linjie.fulinjie@alibaba-inc.com>

Regression since 87f0c8280.

If the extradata of a stream could not be extracted correctly,
codec_info_nb_frames would remain zero, while st->event_flag would still
be marked as AVSTREAM_EVENT_FLAG_NEW_PACKETS since it's a new packet
indeed.

The two expressions could be different in this case, hence restore to
calculate the correct score.

Fix #9029.

Signed-off-by: Linjie Fu <linjie.justin.fu@gmail.com>
---
 fftools/ffmpeg_opt.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Comments

James Almer Jan. 24, 2021, 1:38 p.m. UTC | #1
On 1/24/2021 10:00 AM, Linjie Fu wrote:
> From: Linjie Fu <linjie.fulinjie@alibaba-inc.com>
> 
> Regression since 87f0c8280.
> 
> If the extradata of a stream could not be extracted correctly,
> codec_info_nb_frames would remain zero, while st->event_flag would still
> be marked as AVSTREAM_EVENT_FLAG_NEW_PACKETS since it's a new packet
> indeed.
> 
> The two expressions could be different in this case, hence restore to
> calculate the correct score.
> 
> Fix #9029.

codec_info_nb_frames is not public, which is why its usage was removed, 
so this patch is not ok.

A proper fix would perhaps be to reset event_flags after this, as its 
API says it should. Can you try that and see if it works?

> 
> Signed-off-by: Linjie Fu <linjie.justin.fu@gmail.com>
> ---
>   fftools/ffmpeg_opt.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/fftools/ffmpeg_opt.c b/fftools/ffmpeg_opt.c
> index bf2eb26246..ba813ea7bc 100644
> --- a/fftools/ffmpeg_opt.c
> +++ b/fftools/ffmpeg_opt.c
> @@ -2228,8 +2228,7 @@ static int open_output_file(OptionsContext *o, const char *filename)
>               for (i = 0; i < nb_input_streams; i++) {
>                   int score;
>                   ist = input_streams[i];
> -                score = ist->st->codecpar->width * ist->st->codecpar->height
> -                           + 100000000 * !!(ist->st->event_flags & AVSTREAM_EVENT_FLAG_NEW_PACKETS)
> +                score = ist->st->codecpar->width * ist->st->codecpar->height + 100000000*!!ist->st->codec_info_nb_frames
>                              + 5000000*!!(ist->st->disposition & AV_DISPOSITION_DEFAULT);
>                   if (ist->user_set_discard == AVDISCARD_ALL)
>                       continue;
>
Linjie Fu Jan. 24, 2021, 2:43 p.m. UTC | #2
On Sun, Jan 24, 2021 at 9:38 PM James Almer <jamrial@gmail.com> wrote:
>
> On 1/24/2021 10:00 AM, Linjie Fu wrote:
> > From: Linjie Fu <linjie.fulinjie@alibaba-inc.com>
> >
> > Regression since 87f0c8280.
> >
> > If the extradata of a stream could not be extracted correctly,
> > codec_info_nb_frames would remain zero, while st->event_flag would still
> > be marked as AVSTREAM_EVENT_FLAG_NEW_PACKETS since it's a new packet
> > indeed.
> >
> > The two expressions could be different in this case, hence restore to
> > calculate the correct score.
> >
> > Fix #9029.
>
> codec_info_nb_frames is not public, which is why its usage was removed,
> so this patch is not ok.
Okay, thx.

> A proper fix would perhaps be to reset event_flags after this, as its
> API says it should. Can you try that and see if it works?
>
Yes it works,  and I've tried this before I sent this patch out.

However, what makes me hesitant is the definition of
AVSTREAM_EVENT_FLAG_NEW_PACKETS , which says this flag indicates "new
packets for this stream were read from the file".
IIRC the behaviour in this case matches the definition well, hence I
didn't  reset event_flags to zero at first.

Will send it out for discussion as well, thx.

- linjie
diff mbox series

Patch

diff --git a/fftools/ffmpeg_opt.c b/fftools/ffmpeg_opt.c
index bf2eb26246..ba813ea7bc 100644
--- a/fftools/ffmpeg_opt.c
+++ b/fftools/ffmpeg_opt.c
@@ -2228,8 +2228,7 @@  static int open_output_file(OptionsContext *o, const char *filename)
             for (i = 0; i < nb_input_streams; i++) {
                 int score;
                 ist = input_streams[i];
-                score = ist->st->codecpar->width * ist->st->codecpar->height
-                           + 100000000 * !!(ist->st->event_flags & AVSTREAM_EVENT_FLAG_NEW_PACKETS)
+                score = ist->st->codecpar->width * ist->st->codecpar->height + 100000000*!!ist->st->codec_info_nb_frames
                            + 5000000*!!(ist->st->disposition & AV_DISPOSITION_DEFAULT);
                 if (ist->user_set_discard == AVDISCARD_ALL)
                     continue;