[FFmpeg-devel] fftools/ffmpeg_enc: Prevent duplicate A53 Closed Captions at frame rate up-conversion

When up-converting frame rate, ffmpeg duplicates frames including side data. This causes duplicated characters in the output. This patch removes A53 side data from duplicated frames.

Signed-off-by: Artem Smorodin <artem.smorodin@telestream.net>
---
 fftools/ffmpeg_enc.c | 3 +++
 1 file changed, 3 insertions(+)

Hi Artem,

On Fri, Jun 9, 2023 at 9:19 AM Artem Smorodin
<artem.smorodin@telestream.net> wrote:
>
> When up-converting frame rate, ffmpeg duplicates frames including side data. This causes duplicated characters in the output. This patch removes A53 side data from duplicated frames.
>
> Signed-off-by: Artem Smorodin <artem.smorodin@telestream.net>
> ---
>  fftools/ffmpeg_enc.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/fftools/ffmpeg_enc.c b/fftools/ffmpeg_enc.c
> index 2bf4782a9f..93d18034ca 100644
> --- a/fftools/ffmpeg_enc.c
> +++ b/fftools/ffmpeg_enc.c
> @@ -1104,6 +1104,9 @@ static void do_video_out(OutputFile *of, OutputStream *ost, AVFrame *frame)
>          else if (ret < 0)
>              exit_program(1);
>
> +        //Prevent duplicate Closed Captions at frame rate up-conversion
> +        av_frame_remove_side_data(in_picture, AV_FRAME_DATA_A53_CC);
> +
>          e->next_pts++;
>          e->vsync_frame_number++;
>      }
> --
> 2.25.1
>
> Disclaimer

Unfortunately, the patch needed is more complicated than what you're
proposed.  You can't simply drop the caption from the duplicated
frame, but rather the CC data needs to be rewritten in existing frames
with the appropriate cc_count and distribution of 608 pairs.

I've reworked the logic to behave correctly if you use the "fps" video
filter, but had forgotten that using "-r" doesn't use that filter.

Anton, is there any good reason we couldn't rework the "-r" command
line argument to invoke the fps filter (like we do with scaling)
rather than re-implementing the same functionality inside of the
ffmpeg program itself?  It always seemed strange to me that it didn't
work that way from the beginning...

Artem, in the meantime you can avoid this problem if you specify "-vf
fps" on the command line rather than specifying "-r".

Devin

Hi David,

I think the reworking of the "-r" option is a breaking change.

I understand that output won't conform to the spec, but at least CC won't be broken. For example libx264->a53cc option is 1 by default, so any h264 output with CC with frame rate up-conversion will be broken.
  
The behavior that adds this patch was there before, but was removed after about 5.1.1, so I think it's safe to restore it until a better solution is implemented.

Regarding a better solution:
I don't think we should change the behavior of the "-r" option, instead I suggest moving ccfifo from libavfilter to libavutil and using it in the ffmpeg program.

Thank you
Artem Smorodin

-----Original Message-----
From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Devin Heitmueller
Sent: Friday, June 9, 2023 6:13 PM
To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Cc: Anton Khirnov <anton@khirnov.net>
Subject: [External Sender] Re: [FFmpeg-devel] [PATCH] fftools/ffmpeg_enc: Prevent duplicate A53 Closed Captions at frame rate up-conversion

Hi Artem,

On Fri, Jun 9, 2023 at 9:19 AM Artem Smorodin <artem.smorodin@telestream.net> wrote:
>
> When up-converting frame rate, ffmpeg duplicates frames including side data. This causes duplicated characters in the output. This patch removes A53 side data from duplicated frames.
>
> Signed-off-by: Artem Smorodin <artem.smorodin@telestream.net>
> ---
>  fftools/ffmpeg_enc.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/fftools/ffmpeg_enc.c b/fftools/ffmpeg_enc.c index 
> 2bf4782a9f..93d18034ca 100644
> --- a/fftools/ffmpeg_enc.c
> +++ b/fftools/ffmpeg_enc.c
> @@ -1104,6 +1104,9 @@ static void do_video_out(OutputFile *of, OutputStream *ost, AVFrame *frame)
>          else if (ret < 0)
>              exit_program(1);
>
> +        //Prevent duplicate Closed Captions at frame rate up-conversion
> +        av_frame_remove_side_data(in_picture, AV_FRAME_DATA_A53_CC);
> +
>          e->next_pts++;
>          e->vsync_frame_number++;
>      }
> --
> 2.25.1
>
> Disclaimer

Unfortunately, the patch needed is more complicated than what you're proposed.  You can't simply drop the caption from the duplicated frame, but rather the CC data needs to be rewritten in existing frames with the appropriate cc_count and distribution of 608 pairs.

I've reworked the logic to behave correctly if you use the "fps" video filter, but had forgotten that using "-r" doesn't use that filter.

Anton, is there any good reason we couldn't rework the "-r" command line argument to invoke the fps filter (like we do with scaling) rather than re-implementing the same functionality inside of the ffmpeg program itself?  It always seemed strange to me that it didn't work that way from the beginning...

Artem, in the meantime you can avoid this problem if you specify "-vf fps" on the command line rather than specifying "-r".

Devin

--
Devin Heitmueller, Senior Software Engineer LTN Global Communications
o: +1 (301) 363-1001
w: https://ltnglobal.com  e: devin.heitmueller@ltnglobal.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-request@ffmpeg.org with subject "unsubscribe".

Disclaimer

The information contained in this communication from the sender is confidential. It is intended solely for use by the recipient and others authorized to receive it. If you are not the recipient, you are hereby notified that any disclosure, copying, distribution or taking action in relation of the contents of this information is strictly prohibited and may be unlawful.

This email has been scanned for viruses and malware, and may have been automatically archived by Mimecast Ltd, an innovator in Software as a Service (SaaS) for business. Providing a safer and more useful place for your human generated data. Specializing in; Security, archiving and compliance. To find out more visit the Mimecast website.

Quoting Devin Heitmueller (2023-06-09 17:13:17)
> Hi Artem,
> 
> On Fri, Jun 9, 2023 at 9:19 AM Artem Smorodin
> <artem.smorodin@telestream.net> wrote:
> >
> > When up-converting frame rate, ffmpeg duplicates frames including side data. This causes duplicated characters in the output. This patch removes A53 side data from duplicated frames.
> >
> > Signed-off-by: Artem Smorodin <artem.smorodin@telestream.net>
> > ---
> >  fftools/ffmpeg_enc.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/fftools/ffmpeg_enc.c b/fftools/ffmpeg_enc.c
> > index 2bf4782a9f..93d18034ca 100644
> > --- a/fftools/ffmpeg_enc.c
> > +++ b/fftools/ffmpeg_enc.c
> > @@ -1104,6 +1104,9 @@ static void do_video_out(OutputFile *of, OutputStream *ost, AVFrame *frame)
> >          else if (ret < 0)
> >              exit_program(1);
> >
> > +        //Prevent duplicate Closed Captions at frame rate up-conversion
> > +        av_frame_remove_side_data(in_picture, AV_FRAME_DATA_A53_CC);
> > +
> >          e->next_pts++;
> >          e->vsync_frame_number++;
> >      }
> > --
> > 2.25.1
> >
> > Disclaimer
> 
> Unfortunately, the patch needed is more complicated than what you're
> proposed.  You can't simply drop the caption from the duplicated
> frame, but rather the CC data needs to be rewritten in existing frames
> with the appropriate cc_count and distribution of 608 pairs.
> 
> I've reworked the logic to behave correctly if you use the "fps" video
> filter, but had forgotten that using "-r" doesn't use that filter.
> 
> Anton, is there any good reason we couldn't rework the "-r" command
> line argument to invoke the fps filter (like we do with scaling)
> rather than re-implementing the same functionality inside of the
> ffmpeg program itself?  It always seemed strange to me that it didn't
> work that way from the beginning...

I don't think there's a fundamental reason -r handling could not be
moved to lavfi, but it's a nontrivial task. E.g. the dup/drop counts are
integrated with the CLI and would have to be exported from lavfi somehow
if you wanted to keep that functionality.

On Fri, Jun 9, 2023 at 12:33 PM Artem Smorodin
<artem.smorodin@telestream.net> wrote:
> The behavior that adds this patch was there before, but was removed after about 5.1.1, so I think it's safe to restore it until a better solution is implemented.

I have to admit that I'm hesitant about introducing a patch that only
works for upconversion (i.e. if you go from 59.94 to 29.97 you will
lose half the caption data), as well as when it does work it produces
caption blocks which are malformed and will cause issues with some
downstream decoders.

> Regarding a better solution:
> I don't think we should change the behavior of the "-r" option, instead I suggest moving ccfifo from libavfilter to libavutil and using it in the ffmpeg program.

In my original patch series I actually had the ccfifo stuff as part of
libavutil so it could be shared across the project.  However there was
concern about having to maintain a public ABI for a brand new
interface and thus we decided to keep it private for now (and even
with private APIs there is a mechanism to share them across multiple
libav* libraries).

That said, I hadn't considered the notion of fftools needing access to
it, and I think the general trend has been to move away from fftools
using private APIs.

I suspect we could get "-r" to transparently add the FPS filter to the
end of the filtergraph, just like we do with the vf_scale when users
specify "-s" on the command line.  Although this does raise the issues
that Anton raised regarding dropped/duplicated frames.

My inclination would be to tell people "Don't use '-r' if you care
about captions being properly preserved.  Use '-vf fps' instead."  But
I recognize that there are plenty of people who won't know the
difference.

Devin

On Sat, Jun 10, 2023 at 11:55 PM Anton Khirnov <anton@khirnov.net> wrote:
> I don't think there's a fundamental reason -r handling could not be
> moved to lavfi, but it's a nontrivial task. E.g. the dup/drop counts are
> integrated with the CLI and would have to be exported from lavfi somehow
> if you wanted to keep that functionality.

I think the big question here in my mind is:  does anyone actually
care about the dup/drop counts?  I recognize it's a change in behavior
for the application to not show that in the status, but how useful is
it really to know that the app duplicated 50% of your frames because
you put "-r 60000/1001" on the command line?

I could go further in suggesting it's actually misleading, as people
might very well think there is a problem that resulted in frames being
dropped or duplicated, when in fact it's expected behavior if you
specify "-r" on the command-line.  Adding to the confusion is the fact
that "-r" and "-vf fps" behave differently in terms of how the status
is shown to the user (even though from a user's standpoint they are
doing the same thing).

Devin

On 2023-06-13 07:46 pm, Devin Heitmueller wrote:
> On Sat, Jun 10, 2023 at 11:55 PM Anton Khirnov <anton@khirnov.net> wrote:
>> I don't think there's a fundamental reason -r handling could not be
>> moved to lavfi, but it's a nontrivial task. E.g. the dup/drop counts are
>> integrated with the CLI and would have to be exported from lavfi somehow
>> if you wanted to keep that functionality.
> I think the big question here in my mind is:  does anyone actually
> care about the dup/drop counts?  I recognize it's a change in behavior
> for the application to not show that in the status, but how useful is
> it really to know that the app duplicated 50% of your frames because
> you put "-r 60000/1001" on the command line?

The dup/drop code is active in the absence of output `-r` if the output 
format is not VFR flagged.

Then the dup/drop count is useful to diagnose an abnormally large frame 
rate detected in the input stream,
such as a WebM with a 1k tbr or a cover image with a 90k tbr.
More mundanely,  one can check the 'CFR'-ness of a stream by looking at 
those counts, especially if lavfi is not present.

Regards,
Gyan