[FFmpeg-devel] [PATCH] DVB Subtitles Fix

[Aurelien Jacobs]

On Fri, May 06, 2011 at 02:56:30PM +0300, Ivan Kalvachev wrote:
> [...]
> 
> 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.

I had a look at this exact issue not long ago, and I must say that
this is a fairly accurate description of the problem, and that your
proposed solution is also pretty much what I had in mind (but that
I never got to implement).
So I think it's indeed the way to go.

Aurel