[FFmpeg-devel] [PATCH] lavf/matroskadec: fix is_keyframe for early Blocks

[wm4]

On Fri,  3 Feb 2017 14:42:44 -0800
Chris Cunningham <chcunningham at chromium.org> wrote:

> Blocks are marked as key frames whenever the "reference" field is
> zero. This breaks for non-keyframe Blocks with a reference timestamp
> of zero.
> 
> The likelihood of reference timestamp being zero is increased by a
> longstanding bug in muxing that encodes reference timestamp as the
> absolute time of the referenced frame (rather than relative to the
> current Block timestamp, as described in MKV spec).
> 
> Now using INT64_MIN to denote "no reference".
> 
> Reported to chromium at http://crbug.com/497889 (contains sample)
> ---
>  libavformat/matroskadec.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> index e6737a70b2..7223e94b55 100644
> --- a/libavformat/matroskadec.c
> +++ b/libavformat/matroskadec.c
> @@ -89,6 +89,7 @@ typedef const struct EbmlSyntax {
>      int list_elem_size;
>      int data_offset;
>      union {
> +        int64_t     i;
>          uint64_t    u;
>          double      f;
>          const char *s;
> @@ -696,7 +697,7 @@ static const EbmlSyntax matroska_blockgroup[] = {
>      { MATROSKA_ID_SIMPLEBLOCK,    EBML_BIN,  0, offsetof(MatroskaBlock, bin) },
>      { MATROSKA_ID_BLOCKDURATION,  EBML_UINT, 0, offsetof(MatroskaBlock, duration) },
>      { MATROSKA_ID_DISCARDPADDING, EBML_SINT, 0, offsetof(MatroskaBlock, discard_padding) },
> -    { MATROSKA_ID_BLOCKREFERENCE, EBML_SINT, 0, offsetof(MatroskaBlock, reference) },
> +    { MATROSKA_ID_BLOCKREFERENCE, EBML_SINT, 0, offsetof(MatroskaBlock, reference), { .i = INT64_MIN } },
>      { MATROSKA_ID_CODECSTATE,     EBML_NONE },
>      {                          1, EBML_UINT, 0, offsetof(MatroskaBlock, non_simple), { .u = 1 } },
>      { 0 }
> @@ -1071,6 +1072,9 @@ static int ebml_parse_nest(MatroskaDemuxContext *matroska, EbmlSyntax *syntax,
>  
>      for (i = 0; syntax[i].id; i++)
>          switch (syntax[i].type) {
> +        case EBML_SINT:
> +            *(int64_t *) ((char *) data + syntax[i].data_offset) = syntax[i].def.i;
> +            break;
>          case EBML_UINT:
>              *(uint64_t *) ((char *) data + syntax[i].data_offset) = syntax[i].def.u;
>              break;
> @@ -3361,7 +3365,7 @@ static int matroska_parse_cluster_incremental(MatroskaDemuxContext *matroska)
>          matroska->current_cluster_num_blocks = blocks_list->nb_elem;
>          i                                    = blocks_list->nb_elem - 1;
>          if (blocks[i].bin.size > 0 && blocks[i].bin.data) {
> -            int is_keyframe = blocks[i].non_simple ? !blocks[i].reference : -1;
> +            int is_keyframe = blocks[i].non_simple ? blocks[i].reference == INT64_MIN : -1;
>              uint8_t* additional = blocks[i].additional.size > 0 ?
>                                      blocks[i].additional.data : NULL;
>              if (!blocks[i].non_simple)
> @@ -3399,7 +3403,7 @@ static int matroska_parse_cluster(MatroskaDemuxContext *matroska)
>      blocks      = blocks_list->elem;
>      for (i = 0; i < blocks_list->nb_elem; i++)
>          if (blocks[i].bin.size > 0 && blocks[i].bin.data) {
> -            int is_keyframe = blocks[i].non_simple ? !blocks[i].reference : -1;
> +            int is_keyframe = blocks[i].non_simple ? blocks[i].reference == INT64_MIN : -1;
>              res = matroska_parse_block(matroska, blocks[i].bin.data,
>                                         blocks[i].bin.size, blocks[i].bin.pos,
>                                         cluster.timecode, blocks[i].duration,

I'm fine with this. Still slightly ugly due to using a dummy value, but
probably better than adding an extra mechanism just for this.