[FFmpeg-cvslog] avcodec/dvbsubdec: support returning exact end times

[Reimar Döffinger]

Sorry for being late to the discussion, some quick comments I was wondering about..

On 22.06.2014, at 21:16, git at videolan.org (Anshul Maheshwari) wrote:
> -    sub->end_display_time = ctx->time_out * 1000;
> +    if(ctx->compute_edt == 0)
> +        sub->end_display_time = ctx->time_out * 1000;

Is the if actually necessary (except maybe for documentation purposes/robustness against changes)? compute_edt == 1 will just overwrite it I think?

> -    sub->num_rects = 0;
> +    /* Not touching AVSubtitles again*/
> +    if(sub->num_rects) {
> +        avpriv_request_sample(ctx, "Different Version of Segment asked Twice\n");
> +        return;
> +    }

Why do we need to abort also for compute_edt == 0?
This seems like an unrelated behaviour change to me.

> +end:
> +    if(ret < 0) {
> +        *data_size = 0;
> +        avsubtitle_free(sub);
> +        return ret;
> +    } else {
> +        if(ctx->compute_edt == 1 )
> +            FFSWAP(int64_t, ctx->prev_start, sub->pts);
>     }
> 
>     return p - buf;

This looks messy to me.
first instead of
else {
  if ()...
}

it should at least be just
else if () ...

Next, if the if (ret < 0) contains a return there is no point in having a else at all.
But why do we have a combined "end" here instead of a err_out like in other code?
Then the code would look like

>      if(ctx->compute_edt == 1 )
>         FFSWAP(int64_t, ctx->prev_start, sub->pts);
> 
>     return p - buf;
err_out:
    av_assert0(ret < 0);
    *data_size = 0;
    avsubtitle_free();
    return ret;