[FFmpeg-devel] [PATCH] DVB Subtitles Fix

JULIAN GARDNER joolzg at btinternet.com
Fri May 6 14:36:28 CEST 2011




>________________________________
>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 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

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


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

joolz


More information about the ffmpeg-devel mailing list