[FFmpeg-devel] [PATCH] DVB Subtitles Fix

Ivan Kalvachev ikalvachev at gmail.com
Fri May 6 16:28:24 CEST 2011


On 5/6/11, JULIAN GARDNER <joolzg at btinternet.com> wrote:
>
>
>
>>________________________________
>>From: Ivan Kalvachev <ikalvachev at gmail.com>
>>To: FFmpeg development discussions and patches <ffmpeg-devel at ffmpeg.org>
>>Sent: Friday, 6 May 2011, 12:56
>>Subject: Re: [FFmpeg-devel] [PATCH] DVB Subtitles Fix
>>
>>On 5/5/11, JULIAN GARDNER <joolzg at btinternet.com> wrote:
>>> ----- Original Message -----
>>>> From: Ivan Kalvachev <ikalvachev at gmail.com>
>>>> To: FFmpeg development discussions and patches <ffmpeg-devel at ffmpeg.org>
>>>> Cc:
>>>> Sent: Wednesday, 4 May 2011, 21:32
>>>> Subject: Re: [FFmpeg-devel] [PATCH] DVB Subtitles Fix
>>>>
>>>> On 5/4/11, Michael Niedermayer <michaelni at gmx.at> wrote:
>>>>>  On Wed, May 04, 2011 at 07:18:12PM +0100, JULIAN GARDNER wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>  ----- Original Message -----
>>>>>>  > From: Michael Niedermayer <michaelni at gmx.at>
>>>>>>  > To: FFmpeg development discussions and patches
>>>> <ffmpeg-devel at ffmpeg.org>
>>>>>>  > Cc:
>>>>>>  > Sent: Wednesday, 4 May 2011, 18:25
>>>>>>  > Subject: Re: [FFmpeg-devel] [PATCH] DVB Subtitles Fix
>>>>>>  >
>>>>>>  > On Wed, May 04, 2011 at 04:11:33PM +0100, JULIAN GARDNER wrote:
>>>>>>  >>
>>>>>>  >>
>>>>>>  >>
>>>>>>  >>
>>>>>>  >>  ----- Original Message -----
>>>>>>  >>  > From: Michael Niedermayer <michaelni at gmx.at>
>>>>>>  >>  > To: FFmpeg development discussions and patches
>>>>>>  > <ffmpeg-devel at ffmpeg.org>
>>>>>>  >>  > Cc:
>>>>>>  >>  > Sent: Wednesday, 4 May 2011, 15:14
>>>>>>  >>  > Subject: Re: [FFmpeg-devel] [PATCH] DVB Subtitles Fix
>>>>>>  >>  >
>>>>>>  >>  > On Tue, Apr 26, 2011 at 11:34:10PM +0100, JULIAN GARDNER
>>>> wrote:
>>>>>>  >>  >>  Ok first patch to fix the decoding and re-encoding
>>>> of DVB
>>>>>>  > Subtitles.
>>>>>>  >>  >>
>>>>>>  >>  >>  I went through and validated against the spec, my
>>>> own engine and
>>>>>>  > fixed the
>>>>>>  >>  > bits that were failing or wrong.
>>>>>>  >>  >
>>>>>>  >>  > please explain how to reproduce the bugs that your patch
>>>> fixes
>>>>>>  >>  >
>>>>>>  >>  > Also please explain more elaborately what and why each
>>>> change is
>>>>>>  >> made.
>>>>>>  >>  >
>>>>>>  >>
>>>>>>  >>  I use LIVE and RECORDED streams from various sources which i
>>>> have
>>>>>>  >> collected
>>>>>>  > over the past years, these are put through ffmpeg and then the
>>>> output
>>>>>>  > stream is
>>>>>>  > played out in VLC and a couple of my own receivers which are used
>>>> in
>>>>>>  > professional cable networks.
>>>>>>  >
>>>>>>  > understood, but does this patch fix any problem with these ffmpeg
>>>>>>  > generated streams?
>>>>>>  > Iam asking because id like to reproduce the problem and the fix.
>>>>>>  >
>>>>>>  >
>>>>>>  it fixes all the problems with the encoded streams, also it allows
>>>> FFMPEG
>>>>>>  to process a file correctly without bailing out with an error.
>>>>>>
>>>>>>  I can upload a couple of streams if you want for you to see the
>>>> problems.
>>>>>
>>>>>  thanks, yes this would help me.
>>>>>
>>>>>  if they are small you can upload to our issue tracker
>>>>>  http://ffmpeg.org/trac/ffmpeg
>>>>>
>>>>>
>>>>>>
>>>>>>  >>
>>>>>>  >>  >
>>>>>>  >>  >>
>>>>>>  >>  >>  I have tested this on recorded streams and live
>>>> streams and found
>>>>>>  > no
>>>>>>  >>  > problems except for one which i will try and explain
>>>> about.
>>>>>>  >>  >>
>>>>>>  >>  >>  Here in the UK some of our channels use a live
>>>> speech convertor
>>>>>>  > where you
>>>>>>  >>  > see the words appear as they are spoken, now this works
>>>> fine but the
>>>>>>  >>
>>>>>>  > way the
>>>>>>  >>  > decode/encode works a lot more space is taken by this
>>>> encoder for
>>>>>>  >> the
>>>>>>  > subsequent
>>>>>>  >>  > encoded stream. In one test the incoming packets made up
>>>> around 3k
>>>>>>  >> of
>>>>>>  > data, but
>>>>>>  >>  > the amount of data in the output stream was around 15k.
>>>>>>  >>  >>
>>>>>>  >>  >>  joolz
>>>>>>  >>  >
>>>>>>  >>  >>   ffmpeg.c               |   11 -
>>>>>>  >>  >>   libavcodec/dvbsub.c    |  103 ++++++++---------
>>>>>>  >>  >>   libavcodec/dvbsubdec.c |  296
>>>>>>  >>  > +++++++++++++++++++++++++++++--------------------
>>>>>>  >>  >>   libavcodec/utils.c     |    3
>>>>>>  >>  >>   4 files changed, 233 insertions(+), 180
>>>> deletions(-)
>>>>>>  >>  >>  d22a3a04cf764a0b25138652ad82fe484311405f
>>>> dvbsubs.diff
>>>>>>  >>  >>  diff --git a/ffmpeg.c b/ffmpeg.c
>>>>>>  >>  >>  index 2c69608..e88bcfc 100644
>>>>>>  >>  >>  --- a/ffmpeg.c
>>>>>>  >>  >>  +++ b/ffmpeg.c
>>>>>>  >>  >>  @@ -1092,15 +1092,8 @@ static void
>>>>>>  > do_subtitle_out(AVFormatContext *s,
>>>>>>  >>  >>           subtitle_out =
>>>> av_malloc(subtitle_out_max_size);
>>>>>>  >>  >>       }
>>>>>>  >>  >>
>>>>>>  >>  >>  -    /* Note: DVB subtitle need one packet to draw
>>>> them and one
>>>>>>  > other
>>>>>>  >>  >>  -       packet to clear them */
>>>>>>  >>  >>  -    /* XXX: signal it in the codec context ? */
>>>>>>  >>  >>  -    if (enc->codec_id == CODEC_ID_DVB_SUBTITLE)
>>>>>>  >>  >>  -        nb = 2;
>>>>>>  >>  >>  -    else
>>>>>>  >>  >>  -        nb = 1;
>>>>>>  >>  >>  -
>>>>>>  >>  >>  -    for(i = 0; i < nb; i++) {
>>>>>>  >>  >>  +    i = 0;
>>>>>>  >>  >>  +    {
>>>>>>  >>  >>           sub->pts = av_rescale_q(pts,
>>>>>>  > ist->st->time_base,
>>>>>>  >>  > AV_TIME_BASE_Q);
>>>>>>  >>  >>           // start_display_time is required to be 0
>>>>>>  >>  >>           sub->pts              +=
>>>>>>  >>  > av_rescale_q(sub->start_display_time, (AVRational){1,
>>>> 1000},
>>>>>>  > AV_TIME_BASE_Q);
>>>>>>  >>  >
>>>>>>  >>  > why is this changed?
>>>>>>  >>  >
>>>>>>  >>
>>>>>>  >>  Because only one stream is needed to process DVB Subtitles, i
>>>> could
>>>>>>  >> not
>>>>>>  > work out why you needed this. The NOTE is wrong regarding needed
>>>> one to
>>>>>>  > draw and
>>>>>>  > one to remove
>>>>>>  >>
>>>>>>  >>  What you will get from the DVB Stream is a packet with NO
>>>> regions,
>>>>>>  >> this is
>>>>>>  > the one that removes the subtitles from the picture, as per the
>>>> spec,
>>>>>>  > there is a
>>>>>>  > timeout value, which in the ffmpeg engine is always set to 30
>>>> seconds,
>>>>>>  > this
>>>>>>  > would cause the receiving unit to switch off the display after 30
>>>>>>  > seconds of no
>>>>>>  > valid dvb stream.
>>>>>>  >
>>>>>>  > Lets assume something generates subtitles, lets say it generates
>>>> one
>>>>>>  > displaying "hello world" at start time 0:10 and end time
>>>> 0:50
>>>>>>  > If i understand you correctly this needs a packet muxed with time
>>>> 0:10
>>>>>>  > and a empty clear packet muxed at time 0:50
>>>>>>  >
>>>>>>  > And this loop generates these 2 packets from 1 subtitle with start
>>>> and
>>>>>>  > end times.
>>>>>>  >
>>>>>>  > You remove it and i dont see how this could still work afterwards
>>>>>>  > maybe iam missing something ?
>>>>>>
>>>>>>  The subtitles dont have an end time per se, what you have is a
>>>>>> display
>>>>>>  time and a timeout value, this is incase of a stream problem or
>>>> corruption
>>>>
>>>>>>  for a missing packet.
>>>>>>
>>>>>>  now what happens is in you example you would get a display packet
>>>>>> with
>>>>>>  regions/objects/cluts etc and a timeout of 60, then around 50 seconds
>>>> into
>>>>>>  the stream you would get a packet which has no objects/regions/cluts.
>>>> This
>>>>>>  packet would turn the subs off, now if this packet was corrupted then
>>>> the
>>>>>>  timeout would take care of the removal.
>>>>>
>>>>>  I understand that, but this is only so when the source is dvb subs too
>>>>>  it can be something else.a different subtitle format for example.
>>>>
>>>> This nb variable doesn't have anything with the numbers of streams.
>>>>
>>>> It ensures that in case of DVB output it would write 2 packets.
>>>> The first one with the subtitles and the second one with no
>>>> regions/segments/cluts.
>>>> It does call avcodec_encode_subtitles() twice, so it does get
>>>> different packets (I hope).
>>>> It is kind of running the whole function twice in a row.
>>>>
>>>> It seems that the dvb encoder have variable "hide_state" that always
>>>> inverts itself when called, so every second encoded packet would be
>>>> "hide" packet. And It seems that removing this variable is part of
>>>> your patch.
>>>>
>>>> So I guess the question is. If the source (input) subtitles are of the
>>>> type that have both valid start and end time, how do you output 2
>>>> packets (one to show and one to hide) ?
>>>> Well, I guess this is mostly problem for the time when we have
>>>> ass/ssa->dvb rendering.
>>>
>>> i think your all missing the point, there is no such thing as an off in
>>> dvb
>>> subtitles. what you have is a set of display/regions/cluts and objects
>>> that
>>> make up a subtitle frame. If there are not object/cluts/regions then in
>>> theory you have an off frame. it does not always work that you have a on
>>> followed by an off. If you look at the bbc_small.ts i uploaded you can
>>> see
>>> that it draws 2 regions, 1 is a full width region with a single object,
>>> the
>>> 2nd line is made up of multiple objects as the new text is added.
>>
>>Yes. This is what we call subtitle hiding - displaying off frame.
>>
>>There are text subtitle formats like sami, that have only start time
>>and no duration. You need to display an empty line if you want to hide
>>the text from the previous line. While DVB subtitles allow more
>>complicated rendering they work on same principle.
>>
>>I think this is also the analogy Michael had in mind.
>>
>>> So in this case you get 6 packets before we get a new region whch will
>>> cause
>>> a full redraw, no where in this stream is an "off" packet, so the
>>> inverting
>>> of the hide_state is NOT CORRECT and is not the correct way of doing dvb
>>> subtitles.
>>
>>I don't think there is anybody who would disagree with you.
>>The old method is hack, it seems to relay on the fact that the whole
>>subtitle could be coded in a single packet. It should be replaced by
>>proper solution. That's however a little tricky. (e.g. the encoder api
>>can't output more than one packet at the moment).
>>
>>> Also you will have noticed that i made changes to how the resetting and
>>> clearing of the cluts/objects etc are performed only as to only reset
>>> when
>>> the main display mode changes.
>>>
>>> ass/ssa -> dvb subtitles
>>>
>>> Well i did post a message 2 weeks ago asking about is, no replys, as to
>>> if
>>
>>Do you refer to the "Hard subtitles" thread? IMHO it is a different issue.
>>
>>> there was some way to do what you mentioned ass/ssa to dvb subtitles. In
>>> this case all that needs to be done is that when you get the off time
>>> from
>>> the input you send to the encoder a 0 region subtitle block, the dvb
>>> subtitles encoder will then output an "off" packet.
>>
>>Yes, I think this is exactly what the problem is circling around.
>>It could be done, but there is no code that does it at the moment.
>>
>>Now, the whole things is a little bit more complicated if you consider
>>that the subtitle code should handle other types of subtitles than
>>DVB.
>>To illustrate the problems more clearly I'll be using the following
>> convention.
>>#1 type. In this mode the packet have only one timestamp, in
>>AVSubtitle this would be start_display_time. In order to show subtitle
>>line you need one AVSubtitle, and to hide it you need another. In
>>short it's the DVB and Sami case.
>>#2 type. In this mode there are two timestamps available. In
>>AVSubtitle they would be start/end_display_time. This is the
>>srt/ass/DVD(vobsub)/xsub etc...
>>
>>I would assume the case where we have input:output as  #1#1 and #2#2
>>to be trivial cases. (In your changes you seem to assume ffmpeg should
>>work in #1#1 while at the moment it does work as #2#2).
>>
>>The most troublesome case is #1#2. We can't output encoded packet
>>before we know the end time. This means we need to wait for a second
>>input subtitle packet (or timeout), before we can output the first
>>encoded. This introduces a lag, quite bit lag as subtitles have
>>(relatively) huge durations. So the first output subtitle packet in
>>normal case would appear about 2 seconds after the corresponding audio
>>and video packets have been fed to the muxer. Have in mind that video
>>packets also have their lag, but it is only 1 or 2 frames.
>>
>
> Why do we need to wait for another packet? you have the timeout value, this

In the sami case, It may not have any timeout value.

> added to your start_time gives the end_time. Does this cause a problem is
> another packet comes in with a start_time less than the last packets
> end_time, for example

> in   #1 start_time 1.0s timeout 30.0s
> out #2 start_time 1.0s end_time 31.0s
> in   #1 start_time 5.0s timeout 30.0s
> out #2 start_time 5.0s end_time 35.0s

First, page_time_out is not meant to be used as duration. It is in
whole seconds and allows +5s delay before triggering.

Second, at least with text subtitles there exists so called
"overlapped subtitles". e.g.
0:0 -> 0:2 "I want to tell you...."
0:1 -> 0:3 "No, no, no."
And for the time these subtitles time overlap (0:1-0:2) both would be visible.

While for DVB design this is not issue (I think it supports native
composition), it could be issue in the general case.

> Now the other way you would subtract the two to get the timeout
> in   #2 start_time 1.0s end_time 2.3s
> out #1 start_time 1.0s timeout 2.6s  (double just for an extra bit of
> space)in   #2 start_time 5.0s end_time 7.3s
> out #2 start_time 5.0s timeout 5.2s

in #2 out #1 is described bellow as #2#1 case.

What I have in mind as description for #1#2 working is:

in #1 start_time=1.0
in #1 start_time=5.0
in #1 start_time=9.0
out #2 start_time=1.0 end_time=5.0
out #2 start_time=5.0 end_time=9.0
...



>>The less troublesome case is #2#1. We have both times and the subtitle
>
>>encoder should output 2 packets. One for the start time with the text
>>and one with the empty text/image at the end time.
>>
>>--
>>Now, about your patch. If I understand it correctly assumes the #1#1
>>for proper implementation.
>>
>>However if you insist that the decoders should always output #1 type
>>subtitles, the #2 type encoder will be forced into the #1#2 case, even
>>when the source subtitles are #2 type. I'd like to avoid that.
>
> Nope i dont assume this, this is how it works. You send a subtitle frame to
> the encoder and you will get what you send, a packet with subtitles or an
> empty packet.
>
>
>>
>>On the other hand, if we insist on all decoders always output #2 type
>>subtitles, we are faced by the same lag as in #1#2 case, but this time
>>in the decoder. It would happen even when we have #1#1 chain. Even
>>worse, such lag would be quite troublesome for video players that use
>>only the decoder. I'd say avoid at all cost.
>>
>>So there are 2 proper solutions. simple and complicated.
>>The complicated one is implementing support for handling the #1#2 case
>>lag. I don't think I can ask that from you.
>
> i dont see that problem occuring, the time lag wont appear, the #1 packet
> comes in and we use the timeout, see above
>
>>
>>The simple solution is to make at least the other 3 modes operational.
>>Here are some steps that are needed.
>>
>>1. Find a way to signal #1 or #2 type.
>>Simple solution would be setting end_display_time=0. Right now the dvb
>>decoder fills it with timeout value.
>>Another way is to add new flag field to AVSubtitle. I recommend
>>uint16_t value right after the "uint16_t format" as it may not even
>>break ABI due to struct members alignment.
>>
>>2. Make the dvb decoder output #1 type subtitles.
>>
>>3. Make the dvb encoder output one additional packet with #2 type subtitle.
>>
>>3.1. The dvb encoder should check if the AVSubtitle is #2 type, and if
>>it is so, at the end output a packet with pts of end_display_time and
>>empty subtitle. In essence instead of having "hide_state" iterate at
>>every call, the variable should keep the #2 type flag and output empty
>>packet if it is fed with empty subtitle.
>>
>>3.2. The function do_subtitle_out() in ffmpeg.c should be fixed so it
>>would call subtitle encoder once with the AVSubtitle packet and then
>>loop at encoder (with empty sub)  & writing packets until encoder
>>stops producing data. A number of problems here. the
>>avcodec_encode_subtitle() doesn't return packets, so the encoder can't
>>adjust pts on its own. No "got_picture" variable either.
>>So you may pick the easy road (just make it work) or go the highway
>>(create avcodec_encode_subtitle2() that return packets. so the loops
>>turns into
>>  for(;;){
>>    if (avcodec_subtitle_encode2(enc, &pkt, sub) <= 0 ) break;
>>    write_frame(s,&pkt, ...);
>>  }
>>
>>---
>>
>>The proposed changes should probably be discussed with the other
>>developers before agreeing on final implementation.
>>
>>
>>Sorry for the long delay. I've never ever messed with the subtitles
>>code before and I don't even know how to use it. So I needed some time
>>to look up the internal working. And then I needed some more time to
>>figure out the possible correct solutions.
>>
>>I would understand if you loose patience, but we are just trying to
>>don't break one thing while fixing another.
>>
>>Your work is really appreciated and it is great that there is somebody
>>interested in fixing code that haven't been touched for over 6 years.
>>Just don't give up (yet:)
>>
>>Best Regards.
>>  Ivan Kalvachev
> Ok ill wait on your talks but in the meantime im moving on to finishing my
> modifications, if they dont make it in then hey.
>
> But i need to start looking into adding "hard subs" so once im happy i have
> everything done im moving onto the that.
>
> Once and you have decided on your direction of changes let me know and i
> will try and help with the dvbsubs engine, either in code or knowledge of
> how the system works.
>
> Its a pity that removing around 12 lines of code will at least allow dvbsubs
> to be encoded and not cause a crash.
>
> Is there/has there been any work on getting something that would allow
> transcoding of subtitles from differing formats yet, as this would be a good
> way for me to try and see the problems you mention.

By looking at the code it seems that dvdsub* and xsub* are both #2
type. I would advice you to test dvd->dvb and dvb->dvd conversion, as
we would expect it to be quite common.


More information about the ffmpeg-devel mailing list