[FFmpeg-devel] [PATCH] DVB Subtitles Fix
Ivan Kalvachev
ikalvachev at gmail.com
Fri May 6 13:56:30 CEST 2011
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.
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.
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.
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
More information about the ffmpeg-devel
mailing list