[FFmpeg-devel] [PATCH 3/6] lavc/pgssubdec: use the PTS from the presentation segment.

[Reimar Döffinger]

On 9 Sep 2012, at 16:50, Nicolas George <nicolas.george at normalesup.org> wrote:

> According to the sample for trac ticket #1722, PGS subtitles
> are decoded from several packets at the same DTS and varying PTS.
> The PTS from the presentation segment seem to be the valid one;
> in particular, clear subtitles are too early with the other PTS.
> 
> Signed-off-by: Nicolas George <nicolas.george at normalesup.org>
> ---
> libavcodec/pgssubdec.c |    8 ++++++++
> 1 file changed, 8 insertions(+)
> 
> diff --git a/libavcodec/pgssubdec.c b/libavcodec/pgssubdec.c
> index 79ebb55..64d20df 100644
> --- a/libavcodec/pgssubdec.c
> +++ b/libavcodec/pgssubdec.c
> @@ -67,6 +67,7 @@ typedef struct PGSSubContext {
>     PGSSubPresentation presentation;
>     uint32_t           clut[256];
>     PGSSubPicture      pictures[UINT16_MAX];
> +    int64_t            pts;
>     int forced_subs_only;
> } PGSSubContext;
> 
> @@ -387,7 +388,11 @@ static int display_end_segment(AVCodecContext *avctx, void *data,
>      *      not been cleared by a subsequent empty display command.
>      */
> 
> +    if (ctx->pts == AV_NOPTS_VALUE) /* if no presentation segment to set it */
> +        ctx->pts = sub->pts;
>     memset(sub, 0, sizeof(*sub));
> +    sub->pts = ctx->pts;
> +    ctx->pts = AV_NOPTS_VALUE;

Really a nit, but:
I think this would be less confusing with a local variable, because the assignment to ctx->pts gives an incorrect first impression that the sub pts actually will be stored in the context.
pts = ctx->pts != AV_NOPTS_VALUE ? ctx->pts : sub->pts;
ctx->pts = AV_NOPTS_VALUE;
memset...
sub->pts = pts;

Alternatively, a clearer comment like: // fallback in case there is no pts from presentation segment
Or just naming then context variable presentation_segment_pts might obsolete the comment...