[FFmpeg-devel] [PATCH] PGS subtitle blanking and positioning

[Petri Hintukainen]

Mark Goodman wrote:
> The attached patch makes subtitles blank instead of merely timing out

Correct display update time is stored in the PTS field of the PES packet
carrying presentation composition segment.

> and parses previously ignored position information. I noticed some
> subtitles positioned outside a 2.35:1 aspect ratio video. In the
> packet dump, a few presentation segments had 0x40 for the block byte.
> I tested this patch with MythTV trunk.

That's composition state field.
0x00 = Epoch Continue
       (display set contains only changed elements)
0x40 = Acquisition Point
       (display set contains all elements)
0x80 = Epoch Start
       (display set contains all elements, cached elements should be discarded)
0xc0 = Epoch Continue
       (like Acquisition Point, but used only in start of stream in multi-clip streams ?)

This value should be used (or stored in context), as it controls ex.
semantics of palette updates. In start of epoch palette should be
initialized to transparent (palette segments are sparse) and all cached
objects should be discarded.

> Index: libavcodec/pgssubdec.c
> ===================================================================
> --- libavcodec/pgssubdec.c      (revision 25211)
> +++ libavcodec/pgssubdec.c      (working copy)
> @@ -46,6 +46,7 @@
>      int x;
>      int y;
>      int id_number;
> +    int blank;
>  } PGSSubPresentation;
>  
>  typedef struct PGSSubPicture {
> @@ -255,7 +256,6 @@
>   * @param buf_size size of packet to process
>   * @todo TODO: Implement cropping
>   * @todo TODO: Implement forcing of subtitles
> - * @todo TODO: Blanking of subtitle
>   */
>  static void parse_presentation_segment(AVCodecContext *avctx,
>                                         const uint8_t *buf, int buf_size)
> @@ -280,7 +280,7 @@
>  
>      /* Next byte is the state. */
>      block = bytestream_get_byte(&buf);;
> -    if (block == 0x80) {
> +    if (block == 0x80 || block == 0x40) {

Should check only (block & 0xc0), composition state field is only 2
bits.

The whole check seems to be wrong. Composition state has nothing to do
with the number of following composition object(s). Composition
object(s) should be decoded regardless of composition state.

>          /*
>           * Skip 7 bytes of unknown:
>           *     palette_update_flag (0x80),
> @@ -308,13 +308,8 @@
>          /* Fill in dimensions */
>          ctx->presentation.x = x;
>          ctx->presentation.y = y;
> -    } else if (block == 0x00) {
> -        /* TODO: Blank context as subtitle should not be displayed.
> -         *       If the subtitle is blanked now the subtitle is not
> -         *       on screen long enough to read, due to a delay in
> -         *       initial display timing.
> -         */
>      }
> +    ctx->presentation.blank = block == 0x00;

This is wrong; composition state 0x00 is display set update and does not
mean clearing the display.

Display should be cleared only if there are no composition objects in
composition segment. Currently number of composition objects is not
parsed.

>  }
>  
>  /**
> @@ -327,10 +322,6 @@
>   * @param buf pointer to the packet to process
>   * @param buf_size size of packet to process
>   * @todo TODO: Fix start time, relies on correct PTS, currently too late
> - *
> - * @todo TODO: Fix end time, normally cleared by a second display
> - * @todo       segment, which is currently ignored as it clears
> - * @todo       the subtitle too early.
>   */
>  static int display_end_segment(AVCodecContext *avctx, void *data,
>                                 const uint8_t *buf, int buf_size)
> @@ -345,6 +336,9 @@
>       */
>  
>      memset(sub, 0, sizeof(*sub));
> +    if (ctx->presentation.blank) {
> +        return 1;
> +    }
>      sub->start_display_time = 0;
>      sub->end_display_time   = 20000;
>      sub->format             = 0;