diff mbox

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

Message ID 20170203224244.30066-1-chcunningham@chromium.org
State Accepted
Commit ac25840ee32888f0c13118edeb9404a123cd3a79
Headers show

Commit Message

chcunningham@chromium.org Feb. 3, 2017, 10:42 p.m. UTC 
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(-)

Comments

Vignesh Venkat Feb. 3, 2017, 11:31 p.m. UTC | #1 
On Fri, Feb 3, 2017 at 2:42 PM, Chris Cunningham
<chcunningham@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,

lgtm.

> --
> 2.11.0.483.g087da7b7c-goog
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
wm4 Feb. 4, 2017, noon UTC | #2 
On Fri,  3 Feb 2017 14:42:44 -0800
Chris Cunningham <chcunningham@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.
wm4 Feb. 6, 2017, 8:34 a.m. UTC | #3 
On Fri,  3 Feb 2017 14:42:44 -0800
Chris Cunningham <chcunningham@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,

Pushed.
diff mbox

Patch

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,