[FFmpeg-devel] Another possible bug in dvbsub.c

[Tomas Härdin]

Ronen Mizrahi skrev 2011-02-26 20:56:
> On Wed, Feb 16, 2011 at 2:50 AM, Tomas H?rdin<tomas.hardin at codemill.se>wrote:
>
>> Ronen Mizrahi skrev 2011-02-16 03:05:
>>
>>   On Sat, Feb 12, 2011 at 5:27 PM, Ronen Mizrahi<ronen at tversity.com>
>>>   wrote:
>>>
>>>   Hello,
>>>>
>>>> The problem with current dvbsub encoded with ffmpeg is that the subtitles
>>>> are not remove from display as expected (they remain displayed for a much
>>>> longer period, unless some other sub replaces them). This was tested with
>>>> VLC, ffplay, and also with other hardware devices (set-top-boxes).
>>>>
>>>> By comparing to VLC code base (
>>>>
>>>> http://git.videolan.org/?p=vlc.git;a=blob;f=modules/codec/dvbsub.c;h=432f9b0bb18ee300080af6a30a1fc8648cae7492;hb=HEAD
>>>> )
>>>> we were able to make a small change that seem to have fixed the
>>>> problem: when hide_state is TRUE, no region related information should be
>>>> written. This is unlike the current code where it does write the region
>>>> ids
>>>> and their x,y position, as well as some other information.
>>>>
>>>> Since I am not a dvbsub expert and since the spec is not very clear with
>>>> regard to this situation I wanted to see if someone on the list can
>>>> confirm
>>>> this correction. If so, I am happy to submit a patch.
>>>>
>>>> Best,
>>>>
>>>> Ronen
>>>>
>>>>
>>>>
>>>>
>>>>   Any feedback from someone on the list? Would you consider a patch?
>>>
>>
>> Well, it's hard to comment on patches that aren't attached :)  Also, if it
>> fixes the subs for a lot of players as well as STBs, then ovbiously such a
>> patch is of interest.
>>
>> /Tomas
>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel at mplayerhq.hu
>> https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel
>>
>
>
> Please see attached a patch for dvbsub.c with the following changes:
> - Do not output any region related data when in hide state (fixes issues
> with subtitles not being removed from screen when they expire)
> - Put the region composition segment before the clut (as required by the
> standard - see
> http://www.etsi.org/deliver/etsi_en/300700_300799/300743/01.03.01_60/en_300743v010301p.pdf
> )
>
> With these fixes we can confirm that dvb subtitles encoded by ffmpeg seem to
> work correctly with VLC, ffplay and several set-top-boxes we tested.
>
> Best,
>
> Ronen
>
> From 8cbc516a56033e1dce7e1065b3253904b8fcc6d9 Mon Sep 17 00:00:00 2001
> From: Ronen Mizrahi <ronen at tversity.com>
> Date: Sat, 26 Feb 2011 14:45:26 -0500
> Subject: [PATCH] dvb subtitles - put the region composition segment before the clut (as required by the standard). Do not output any region related data when in hide state
>
> ---
>  libavcodec/dvbsub.c |   90 ++++++++++++++++++++++++++-------------------------
>  1 files changed, 46 insertions(+), 44 deletions(-)
>
> diff --git a/libavcodec/dvbsub.c b/libavcodec/dvbsub.c
> index ed12823..71260fe 100644
> --- a/libavcodec/dvbsub.c
> +++ b/libavcodec/dvbsub.c
> @@ -225,16 +225,57 @@ static int encode_dvb_subtitles(DVBSubtitleContext *s,
>      /* page_version = 0 + page_state */
>      *q++ = (s->object_version << 4) | (page_state << 2) | 3;
>
> -    for (region_id = 0; region_id < h->num_rects; region_id++) {
> -        *q++ = region_id;
> -        *q++ = 0xff; /* reserved */
> -        bytestream_put_be16(&q, h->rects[region_id]->x); /* left pos */
> -        bytestream_put_be16(&q, h->rects[region_id]->y); /* top pos */
> +    if (!s->hide_state) {
> +        for (region_id = 0; region_id < h->num_rects; region_id++) {
> +            *q++ = region_id;
> +            *q++ = 0xff; /* reserved */
> +            bytestream_put_be16(&q, h->rects[region_id]->x); /* left pos */
> +            bytestream_put_be16(&q, h->rects[region_id]->y); /* top pos */
> +        }
>      }

Indent should be done in a separate patch. That way it's easier so see 
that this one doesn't change the behavior of the loop.

>      bytestream_put_be16(&pseg_len, q - pseg_len - 2);
>
>      if (!s->hide_state) {
> +        for (region_id = 0; region_id < h->num_rects; region_id++) {
> +
> +            /* region composition segment */
> +
> +            if (h->rects[region_id]->nb_colors <= 4) {
> +                /* 2 bpp, some decoders do not support it correctly */
> +                bpp_index = 0;
> +            } else if (h->rects[region_id]->nb_colors <= 16) {
> +                /* 4 bpp, standard encoding */
> +                bpp_index = 1;
> +            } else {
> +                return -1;
> +            }
> +
> +            *q++ = 0x0f; /* sync_byte */
> +            *q++ = 0x11; /* segment_type */
> +            bytestream_put_be16(&q, page_id);
> +            pseg_len = q;
> +            q += 2; /* segment length */
> +            *q++ = region_id;
> +            *q++ = (s->object_version << 4) | (0 << 3) | 0x07; /* version , no fill */
> +            bytestream_put_be16(&q, h->rects[region_id]->w); /* region width */
> +            bytestream_put_be16(&q, h->rects[region_id]->h); /* region height */
> +            *q++ = ((1 + bpp_index) << 5) | ((1 + bpp_index) << 2) | 0x03;
> +            *q++ = region_id; /* clut_id == region_id */
> +            *q++ = 0; /* 8 bit fill colors */
> +            *q++ = 0x03; /* 4 bit and 2 bit fill colors */
> +
> +            if (!s->hide_state) {

This check can be dropped since you're already doing it outside the loop.

Also, indent.

Overall it looks OK. I'll take your word on it following the spec, 
especially if several STBs take the output.

/Tomas