[FFmpeg-devel,v1] fftools/ffmpeg: set AV_PKT_FLAG_KEY for the subtitle packet

From: Limin Wang <lance.lmwang@gmail.com>

This fixes webvtt segment output.

Please testing with the following command and check the output:
./ffmpeg -i ../fate-suite/sub/MicroDVD_capability_tester.srt -f segment -segment_time 10 \
    -segment_list_size 0  -segment_list sub.m3u8 -segment_format webvtt -scodec webvtt sub-%d.vtt


Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
---
 fftools/ffmpeg.c                 |  1 +
 tests/ref/fate/binsub-movtextenc |  2 +-
 tests/ref/fate/sub2video         | 86 ++++++++++++++++----------------
 3 files changed, 45 insertions(+), 44 deletions(-)

On Fri, Mar 20, 2020 at 5:45 PM <lance.lmwang@gmail.com> wrote:
>
> From: Limin Wang <lance.lmwang@gmail.com>
>
> This fixes webvtt segment output.
>
> Please testing with the following command and check the output:
> ./ffmpeg -i ../fate-suite/sub/MicroDVD_capability_tester.srt -f segment -segment_time 10 \
>     -segment_list_size 0  -segment_list sub.m3u8 -segment_format webvtt -scodec webvtt sub-%d.vtt
>
>
> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> ---
>  fftools/ffmpeg.c                 |  1 +
>  tests/ref/fate/binsub-movtextenc |  2 +-
>  tests/ref/fate/sub2video         | 86 ++++++++++++++++----------------
>  3 files changed, 45 insertions(+), 44 deletions(-)
>
> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> index aaaf241314..c5a2d0731d 100644
> --- a/fftools/ffmpeg.c
> +++ b/fftools/ffmpeg.c
> @@ -1054,6 +1054,7 @@ static void do_subtitle_out(OutputFile *of,
>              else
>                  pkt.pts += av_rescale_q(sub->end_display_time, (AVRational){ 1, 1000 }, ost->mux_timebase);
>          }
> +        pkt.flags |= AV_PKT_FLAG_KEY;
>          pkt.dts = pkt.pts;
>          output_packet(of, &pkt, ost, 0);
>      }

I do wonder if this is just a case of people forgetting to set the
flag for the relevant packets in the relevant modules?

I'm not sure if all API users should be forced to handle this
separately. If the packets are decode'able by themselves, they should
be marked as such.

(Unfortunately, this probably means that all subtitle encoders and
text-based subtitle format demuxers would have to be updated where
this flag is not set)

Best regards,
Jan

Jan Ekström (12020-03-20):
> I do wonder if this is just a case of people forgetting to set the
> flag for the relevant packets in the relevant modules?
> 
> I'm not sure if all API users should be forced to handle this
> separately. If the packets are decode'able by themselves, they should
> be marked as such.

I had the same idea, and I checked: the problem is that the subtitle
encoding API returns just a buffer of data, not a packet. With that
crappy API, this change is the best we can do.

Regards,

On Fri, Mar 20, 2020 at 6:12 PM Nicolas George <george@nsup.org> wrote:
>
> Jan Ekström (12020-03-20):
> > I do wonder if this is just a case of people forgetting to set the
> > flag for the relevant packets in the relevant modules?
> >
> > I'm not sure if all API users should be forced to handle this
> > separately. If the packets are decode'able by themselves, they should
> > be marked as such.
>
> I had the same idea, and I checked: the problem is that the subtitle
> encoding API returns just a buffer of data, not a packet. With that
> crappy API, this change is the best we can do.
>

Just as I noticed that myself in ffmpeg.c :).

In that case, yes... Until the encoding API pushes out AVPackets we
can't help too much with this :/ . Consider my initial comment moot.

Jan

Jan Ekström:
> On Fri, Mar 20, 2020 at 5:45 PM <lance.lmwang@gmail.com> wrote:
>>
>> From: Limin Wang <lance.lmwang@gmail.com>
>>
>> This fixes webvtt segment output.
>>
>> Please testing with the following command and check the output:
>> ./ffmpeg -i ../fate-suite/sub/MicroDVD_capability_tester.srt -f segment -segment_time 10 \
>>     -segment_list_size 0  -segment_list sub.m3u8 -segment_format webvtt -scodec webvtt sub-%d.vtt
>>
>>
>> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
>> ---
>>  fftools/ffmpeg.c                 |  1 +
>>  tests/ref/fate/binsub-movtextenc |  2 +-
>>  tests/ref/fate/sub2video         | 86 ++++++++++++++++----------------
>>  3 files changed, 45 insertions(+), 44 deletions(-)
>>
>> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
>> index aaaf241314..c5a2d0731d 100644
>> --- a/fftools/ffmpeg.c
>> +++ b/fftools/ffmpeg.c
>> @@ -1054,6 +1054,7 @@ static void do_subtitle_out(OutputFile *of,
>>              else
>>                  pkt.pts += av_rescale_q(sub->end_display_time, (AVRational){ 1, 1000 }, ost->mux_timebase);
>>          }
>> +        pkt.flags |= AV_PKT_FLAG_KEY;
>>          pkt.dts = pkt.pts;
>>          output_packet(of, &pkt, ost, 0);
>>      }
> 
> I do wonder if this is just a case of people forgetting to set the
> flag for the relevant packets in the relevant modules?
> 
> I'm not sure if all API users should be forced to handle this
> separately. If the packets are decode'able by themselves, they should
> be marked as such.
> 
> (Unfortunately, this probably means that all subtitle encoders and
> text-based subtitle format demuxers would have to be updated where
> this flag is not set)
> 
av_read_frame() already sets the AV_PKT_FLAG_KEY-flag for all subtitle
packets (see is_intra_only() in libavformat/utils.c; the subtitle
demuxer based around FFDemuxSubtitlesQueues actually have the flag set
even before it reaches is_intra_only()). One could do something similar
in libavformat/mux.c.
But are we actually sure that all subtitle packets are decodable by
themselves? IIRC this is not true for all PGS subtitles.

- Andreas

PS: The semantics of the AV_CODEC_PROP_INTRA_ONLY-flag seem to be based
around the assumption that subtitle packets are always intra-only: It is
only for video and audio-codecs only.

On Fri, Mar 20, 2020 at 06:03:53PM +0200, Jan Ekström wrote:
> On Fri, Mar 20, 2020 at 5:45 PM <lance.lmwang@gmail.com> wrote:
> >
> > From: Limin Wang <lance.lmwang@gmail.com>
> >
> > This fixes webvtt segment output.
> >
> > Please testing with the following command and check the output:
> > ./ffmpeg -i ../fate-suite/sub/MicroDVD_capability_tester.srt -f segment -segment_time 10 \
> >     -segment_list_size 0  -segment_list sub.m3u8 -segment_format webvtt -scodec webvtt sub-%d.vtt
> >
> >
> > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > ---
> >  fftools/ffmpeg.c                 |  1 +
> >  tests/ref/fate/binsub-movtextenc |  2 +-
> >  tests/ref/fate/sub2video         | 86 ++++++++++++++++----------------
> >  3 files changed, 45 insertions(+), 44 deletions(-)
> >
> > diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> > index aaaf241314..c5a2d0731d 100644
> > --- a/fftools/ffmpeg.c
> > +++ b/fftools/ffmpeg.c
> > @@ -1054,6 +1054,7 @@ static void do_subtitle_out(OutputFile *of,
> >              else
> >                  pkt.pts += av_rescale_q(sub->end_display_time, (AVRational){ 1, 1000 }, ost->mux_timebase);
> >          }
> > +        pkt.flags |= AV_PKT_FLAG_KEY;
> >          pkt.dts = pkt.pts;
> >          output_packet(of, &pkt, ost, 0);
> >      }
> 
> I do wonder if this is just a case of people forgetting to set the
> flag for the relevant packets in the relevant modules?
> 
> I'm not sure if all API users should be forced to handle this
> separately. If the packets are decode'able by themselves, they should
> be marked as such.

If we're using -c:s copy for the subtitle, the flag is set in 
ff_subtitles_queue_insert() of libavformat/subtitles.c.

> 
> (Unfortunately, this probably means that all subtitle encoders and
> text-based subtitle format demuxers would have to be updated where
> this flag is not set)

subtitle encoder haven no way to set the flag in the encoder api now.

> 
> Best regards,
> Jan
> _______________________________________________
> 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".

Quoting Andreas Rheinhardt (2020-03-20 17:51:41)
> Jan Ekström:
> > On Fri, Mar 20, 2020 at 5:45 PM <lance.lmwang@gmail.com> wrote:
> >>
> >> From: Limin Wang <lance.lmwang@gmail.com>
> >>
> >> This fixes webvtt segment output.
> >>
> >> Please testing with the following command and check the output:
> >> ./ffmpeg -i ../fate-suite/sub/MicroDVD_capability_tester.srt -f segment -segment_time 10 \
> >>     -segment_list_size 0  -segment_list sub.m3u8 -segment_format webvtt -scodec webvtt sub-%d.vtt
> >>
> >>
> >> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> >> ---
> >>  fftools/ffmpeg.c                 |  1 +
> >>  tests/ref/fate/binsub-movtextenc |  2 +-
> >>  tests/ref/fate/sub2video         | 86 ++++++++++++++++----------------
> >>  3 files changed, 45 insertions(+), 44 deletions(-)
> >>
> >> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> >> index aaaf241314..c5a2d0731d 100644
> >> --- a/fftools/ffmpeg.c
> >> +++ b/fftools/ffmpeg.c
> >> @@ -1054,6 +1054,7 @@ static void do_subtitle_out(OutputFile *of,
> >>              else
> >>                  pkt.pts += av_rescale_q(sub->end_display_time, (AVRational){ 1, 1000 }, ost->mux_timebase);
> >>          }
> >> +        pkt.flags |= AV_PKT_FLAG_KEY;
> >>          pkt.dts = pkt.pts;
> >>          output_packet(of, &pkt, ost, 0);
> >>      }
> > 
> > I do wonder if this is just a case of people forgetting to set the
> > flag for the relevant packets in the relevant modules?
> > 
> > I'm not sure if all API users should be forced to handle this
> > separately. If the packets are decode'able by themselves, they should
> > be marked as such.
> > 
> > (Unfortunately, this probably means that all subtitle encoders and
> > text-based subtitle format demuxers would have to be updated where
> > this flag is not set)
> > 
> av_read_frame() already sets the AV_PKT_FLAG_KEY-flag for all subtitle
> packets (see is_intra_only() in libavformat/utils.c; the subtitle
> demuxer based around FFDemuxSubtitlesQueues actually have the flag set
> even before it reaches is_intra_only()). One could do something similar
> in libavformat/mux.c.
> But are we actually sure that all subtitle packets are decodable by
> themselves? IIRC this is not true for all PGS subtitles.
> 
> - Andreas
> 
> PS: The semantics of the AV_CODEC_PROP_INTRA_ONLY-flag seem to be based
> around the assumption that subtitle packets are always intra-only: It is
> only for video and audio-codecs only.

I would interpret it as simply not being defined for subtitles, rather
than saying they are all always intra only. So we can always add new
semantics for subtitles to it.

On Thu, Mar 26, 2020 at 10:47:37AM +0100, Anton Khirnov wrote:
> Quoting Andreas Rheinhardt (2020-03-20 17:51:41)
> > Jan Ekström:
> > > On Fri, Mar 20, 2020 at 5:45 PM <lance.lmwang@gmail.com> wrote:
> > >>
> > >> From: Limin Wang <lance.lmwang@gmail.com>
> > >>
> > >> This fixes webvtt segment output.
> > >>
> > >> Please testing with the following command and check the output:
> > >> ./ffmpeg -i ../fate-suite/sub/MicroDVD_capability_tester.srt -f segment -segment_time 10 \
> > >>     -segment_list_size 0  -segment_list sub.m3u8 -segment_format webvtt -scodec webvtt sub-%d.vtt
> > >>
> > >>
> > >> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > >> ---
> > >>  fftools/ffmpeg.c                 |  1 +
> > >>  tests/ref/fate/binsub-movtextenc |  2 +-
> > >>  tests/ref/fate/sub2video         | 86 ++++++++++++++++----------------
> > >>  3 files changed, 45 insertions(+), 44 deletions(-)
> > >>
> > >> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> > >> index aaaf241314..c5a2d0731d 100644
> > >> --- a/fftools/ffmpeg.c
> > >> +++ b/fftools/ffmpeg.c
> > >> @@ -1054,6 +1054,7 @@ static void do_subtitle_out(OutputFile *of,
> > >>              else
> > >>                  pkt.pts += av_rescale_q(sub->end_display_time, (AVRational){ 1, 1000 }, ost->mux_timebase);
> > >>          }
> > >> +        pkt.flags |= AV_PKT_FLAG_KEY;
> > >>          pkt.dts = pkt.pts;
> > >>          output_packet(of, &pkt, ost, 0);
> > >>      }
> > > 
> > > I do wonder if this is just a case of people forgetting to set the
> > > flag for the relevant packets in the relevant modules?
> > > 
> > > I'm not sure if all API users should be forced to handle this
> > > separately. If the packets are decode'able by themselves, they should
> > > be marked as such.
> > > 
> > > (Unfortunately, this probably means that all subtitle encoders and
> > > text-based subtitle format demuxers would have to be updated where
> > > this flag is not set)
> > > 
> > av_read_frame() already sets the AV_PKT_FLAG_KEY-flag for all subtitle
> > packets (see is_intra_only() in libavformat/utils.c; the subtitle
> > demuxer based around FFDemuxSubtitlesQueues actually have the flag set
> > even before it reaches is_intra_only()). One could do something similar
> > in libavformat/mux.c.
> > But are we actually sure that all subtitle packets are decodable by
> > themselves? IIRC this is not true for all PGS subtitles.
> > 
> > - Andreas
> > 
> > PS: The semantics of the AV_CODEC_PROP_INTRA_ONLY-flag seem to be based
> > around the assumption that subtitle packets are always intra-only: It is
> > only for video and audio-codecs only.
> 
> I would interpret it as simply not being defined for subtitles, rather
> than saying they are all always intra only. So we can always add new
> semantics for subtitles to it.

+1

[...]

On Fri, Mar 27, 2020 at 01:47:10AM +0100, Michael Niedermayer wrote:
> On Thu, Mar 26, 2020 at 10:47:37AM +0100, Anton Khirnov wrote:
> > Quoting Andreas Rheinhardt (2020-03-20 17:51:41)
> > > Jan Ekström:
> > > > On Fri, Mar 20, 2020 at 5:45 PM <lance.lmwang@gmail.com> wrote:
> > > >>
> > > >> From: Limin Wang <lance.lmwang@gmail.com>
> > > >>
> > > >> This fixes webvtt segment output.
> > > >>
> > > >> Please testing with the following command and check the output:
> > > >> ./ffmpeg -i ../fate-suite/sub/MicroDVD_capability_tester.srt -f segment -segment_time 10 \
> > > >>     -segment_list_size 0  -segment_list sub.m3u8 -segment_format webvtt -scodec webvtt sub-%d.vtt
> > > >>
> > > >>
> > > >> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > > >> ---
> > > >>  fftools/ffmpeg.c                 |  1 +
> > > >>  tests/ref/fate/binsub-movtextenc |  2 +-
> > > >>  tests/ref/fate/sub2video         | 86 ++++++++++++++++----------------
> > > >>  3 files changed, 45 insertions(+), 44 deletions(-)
> > > >>
> > > >> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> > > >> index aaaf241314..c5a2d0731d 100644
> > > >> --- a/fftools/ffmpeg.c
> > > >> +++ b/fftools/ffmpeg.c
> > > >> @@ -1054,6 +1054,7 @@ static void do_subtitle_out(OutputFile *of,
> > > >>              else
> > > >>                  pkt.pts += av_rescale_q(sub->end_display_time, (AVRational){ 1, 1000 }, ost->mux_timebase);
> > > >>          }
> > > >> +        pkt.flags |= AV_PKT_FLAG_KEY;
> > > >>          pkt.dts = pkt.pts;
> > > >>          output_packet(of, &pkt, ost, 0);
> > > >>      }
> > > > 
> > > > I do wonder if this is just a case of people forgetting to set the
> > > > flag for the relevant packets in the relevant modules?
> > > > 
> > > > I'm not sure if all API users should be forced to handle this
> > > > separately. If the packets are decode'able by themselves, they should
> > > > be marked as such.
> > > > 
> > > > (Unfortunately, this probably means that all subtitle encoders and
> > > > text-based subtitle format demuxers would have to be updated where
> > > > this flag is not set)
> > > > 
> > > av_read_frame() already sets the AV_PKT_FLAG_KEY-flag for all subtitle
> > > packets (see is_intra_only() in libavformat/utils.c; the subtitle
> > > demuxer based around FFDemuxSubtitlesQueues actually have the flag set
> > > even before it reaches is_intra_only()). One could do something similar
> > > in libavformat/mux.c.
> > > But are we actually sure that all subtitle packets are decodable by
> > > themselves? IIRC this is not true for all PGS subtitles.
> > > 
> > > - Andreas
> > > 
> > > PS: The semantics of the AV_CODEC_PROP_INTRA_ONLY-flag seem to be based
> > > around the assumption that subtitle packets are always intra-only: It is
> > > only for video and audio-codecs only.
> > 
> > I would interpret it as simply not being defined for subtitles, rather
> > than saying they are all always intra only. So we can always add new
> > semantics for subtitles to it.
> 
> +1
I have updated the patch, please review whether it's expected.

> 
> [...]
> -- 
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> 
> The real ebay dictionary, page 2
> "100% positive feedback" - "All either got their money back or didnt complain"
> "Best seller ever, very honest" - "Seller refunded buyer after failed scam"



> _______________________________________________
> 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".