subject:"\[FFmpeg\-devel\] \[PATCH\] lavf\/matroskadec\: fix is

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

2017-02-06 Thread wm4

On Fri,  3 Feb 2017 14:42:44 -0800
Chris Cunningham  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_tu;
>  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.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2017-02-04 Thread wm4

On Fri,  3 Feb 2017 14:42:44 -0800
Chris Cunningham  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_tu;
>  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.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2017-02-03 Thread Vignesh Venkatasubramanian

On Fri, Feb 3, 2017 at 2:42 PM, Chris Cunningham
 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_tu;
>  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



-- 
Vignesh
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2017-02-03 Thread Chris Cunningham

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_tu;
 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,
-- 
2.11.0.483.g087da7b7c-goog

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2017-02-03 Thread wm4

On Fri, 3 Feb 2017 10:04:34 -0800
Vignesh Venkatasubramanian  wrote:

> On Thu, Feb 2, 2017 at 10:16 PM, wm4  wrote:
> > On Thu, 2 Feb 2017 10:47:52 -0800
> > Vignesh Venkatasubramanian  wrote:
> >  
> >> On Tue, Jan 31, 2017 at 10:18 PM, wm4  wrote:  
> >> >
> >> > On Tue, 31 Jan 2017 12:02:01 -0800
> >> > Chris Cunningham  wrote:
> >> >  
> >> > > Thanks for taking a look.
> >> > >
> >> > > Definitely missing a "break;" - will fix in subsequent patch.
> >> > >
> >> > > Agree timestamps should be relative (didn't realize this). Vignesh 
> >> > > points
> >> > > out that "0" in the test file is due to a bug in ffmpeg (and probably 
> >> > > other
> >> > > muxers) where this value is not written as a relative timestamp, but
> >> > > instead as the timestamp of the previous frame. https://github.com/FFmp
> >> > > eg/FFmpeg/blob/master/libavformat/matroskaenc.c#L2053
> >> > > 
> >> > >   
> >> >
> >> > Just a few lines below this reads
> >> >
> >> >mkv->last_track_timestamp[track_number - 1] = ts - mkv->cluster_pts;
> >> >
> >> > which looks like it intends to write a relative value. Though "ts" can
> >> > be a DTS, while the other value is always a PTS.  
> >>
> >> Just to clarify: This line makes the timestamp relative to the
> >> cluster's timestamp. Not relative to the block its referencing (which
> >> is what the spec says if i understand it correctly).  
> >
> > Yeah, the current spec just says "Timestamp of another frame used as a
> > reference (ie: B or P frame). The timestamp is relative to the  block
> > it's attached to."
> >
> > Is this a bug? Did FFmpeg always mux this incorrectly?
> >  
> 
> Technically it is a bug. Yes ffmpeg has always muxed it this way. But
> see the reply below.
> 
> > Is there even an implementation that uses the value written to block
> > reference elements? (And not in a trivial way like FFmpeg.)  
> 
> AFAIK, there is no practical use for the value written into
> BlockReference. All the WebM codecs (vp8, vp9, vorbis, opus) have the
> reference frame information in their bitstream and do not care about
> what is specified in the container. In fact, in case of VP9 there can
> be multiple reference frames and i'm not sure if there's even a way to
> specify that in the container. So far, the only use of this element
> has been to determine whether or not the Block is a keyframe.

Yes, that seems to be pretty much anyone's opinion about this
(including my own).

Still feels weird that FFmpeg is writing essentially arbitrary values.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2017-02-03 Thread Vignesh Venkatasubramanian

On Thu, Feb 2, 2017 at 10:16 PM, wm4  wrote:
> On Thu, 2 Feb 2017 10:47:52 -0800
> Vignesh Venkatasubramanian  wrote:
>
>> On Tue, Jan 31, 2017 at 10:18 PM, wm4  wrote:
>> >
>> > On Tue, 31 Jan 2017 12:02:01 -0800
>> > Chris Cunningham  wrote:
>> >
>> > > Thanks for taking a look.
>> > >
>> > > Definitely missing a "break;" - will fix in subsequent patch.
>> > >
>> > > Agree timestamps should be relative (didn't realize this). Vignesh points
>> > > out that "0" in the test file is due to a bug in ffmpeg (and probably 
>> > > other
>> > > muxers) where this value is not written as a relative timestamp, but
>> > > instead as the timestamp of the previous frame. https://github.com/FFmp
>> > > eg/FFmpeg/blob/master/libavformat/matroskaenc.c#L2053
>> > > 
>> >
>> > Just a few lines below this reads
>> >
>> >mkv->last_track_timestamp[track_number - 1] = ts - mkv->cluster_pts;
>> >
>> > which looks like it intends to write a relative value. Though "ts" can
>> > be a DTS, while the other value is always a PTS.
>>
>> Just to clarify: This line makes the timestamp relative to the
>> cluster's timestamp. Not relative to the block its referencing (which
>> is what the spec says if i understand it correctly).
>
> Yeah, the current spec just says "Timestamp of another frame used as a
> reference (ie: B or P frame). The timestamp is relative to the  block
> it's attached to."
>
> Is this a bug? Did FFmpeg always mux this incorrectly?
>

Technically it is a bug. Yes ffmpeg has always muxed it this way. But
see the reply below.

> Is there even an implementation that uses the value written to block
> reference elements? (And not in a trivial way like FFmpeg.)

AFAIK, there is no practical use for the value written into
BlockReference. All the WebM codecs (vp8, vp9, vorbis, opus) have the
reference frame information in their bitstream and do not care about
what is specified in the container. In fact, in case of VP9 there can
be multiple reference frames and i'm not sure if there's even a way to
specify that in the container. So far, the only use of this element
has been to determine whether or not the Block is a keyframe.

> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel



-- 
Vignesh
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2017-02-02 Thread wm4

On Thu, 2 Feb 2017 10:47:52 -0800
Vignesh Venkatasubramanian  wrote:

> On Tue, Jan 31, 2017 at 10:18 PM, wm4  wrote:
> >
> > On Tue, 31 Jan 2017 12:02:01 -0800
> > Chris Cunningham  wrote:
> >  
> > > Thanks for taking a look.
> > >
> > > Definitely missing a "break;" - will fix in subsequent patch.
> > >
> > > Agree timestamps should be relative (didn't realize this). Vignesh points
> > > out that "0" in the test file is due to a bug in ffmpeg (and probably 
> > > other
> > > muxers) where this value is not written as a relative timestamp, but
> > > instead as the timestamp of the previous frame. https://github.com/FFmp
> > > eg/FFmpeg/blob/master/libavformat/matroskaenc.c#L2053
> > > 
> > >   
> >
> > Just a few lines below this reads
> >
> >mkv->last_track_timestamp[track_number - 1] = ts - mkv->cluster_pts;
> >
> > which looks like it intends to write a relative value. Though "ts" can
> > be a DTS, while the other value is always a PTS.  
> 
> Just to clarify: This line makes the timestamp relative to the
> cluster's timestamp. Not relative to the block its referencing (which
> is what the spec says if i understand it correctly).

Yeah, the current spec just says "Timestamp of another frame used as a
reference (ie: B or P frame). The timestamp is relative to the  block
it's attached to."

Is this a bug? Did FFmpeg always mux this incorrectly?

Is there even an implementation that uses the value written to block
reference elements? (And not in a trivial way like FFmpeg.)
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2017-02-02 Thread Vignesh Venkatasubramanian

On Tue, Jan 31, 2017 at 10:18 PM, wm4  wrote:
>
> On Tue, 31 Jan 2017 12:02:01 -0800
> Chris Cunningham  wrote:
>
> > Thanks for taking a look.
> >
> > Definitely missing a "break;" - will fix in subsequent patch.
> >
> > Agree timestamps should be relative (didn't realize this). Vignesh points
> > out that "0" in the test file is due to a bug in ffmpeg (and probably other
> > muxers) where this value is not written as a relative timestamp, but
> > instead as the timestamp of the previous frame. https://github.com/FFmp
> > eg/FFmpeg/blob/master/libavformat/matroskaenc.c#L2053
> > 
>
> Just a few lines below this reads
>
>mkv->last_track_timestamp[track_number - 1] = ts - mkv->cluster_pts;
>
> which looks like it intends to write a relative value. Though "ts" can
> be a DTS, while the other value is always a PTS.

Just to clarify: This line makes the timestamp relative to the
cluster's timestamp. Not relative to the block its referencing (which
is what the spec says if i understand it correctly).

>
> > Anyway, I'm all for following Haali. Its not obvious how best to do this. I
> > don't think there's any great default value to indicate "not-set" and the
> > generic embl parsing code that reads the reference timestamp doesn't really
> > lend itself to setting an additional field like
> > MatroskaBlock.has_reference. I can sort this out, but I'll pause in case
> > you have a recommendation in-mind.
>
> I don't know a nice way either. Here are 3 potential ways I came up
> with:
>
> 1. Use a better dummy value, like INT64_MIN, which is almost 100%
>unlikely to appear in a valid file. (This is probably equivalent to
>your intention in original patch.)
> 2. Add a new "presence_flag_offset" field to EbmlSyntax - the EBML
>reader will write a 1 to it if the element was found. (This is a bit
>annoying, because 0 is a valid offset, and you surely wouldn't want
>to change all the other element definitions.)
> 3. Add a new type like EBML_PRESENCE, which writes whether the element
>was present or not to data_offset, instead of the element's content
>or its default value. (There are some other special "types" like
>EBML_STOP, so maybe this idea isn't all too hacky.)
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel




-- 
Vignesh
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2017-02-01 Thread Chris Cunningham

On Tue, Jan 31, 2017 at 10:18 PM, wm4  wrote:

> On Tue, 31 Jan 2017 12:02:01 -0800
> Chris Cunningham  wrote:
>
> > Thanks for taking a look.
> >
> > Definitely missing a "break;" - will fix in subsequent patch.
> >
> > Agree timestamps should be relative (didn't realize this). Vignesh points
> > out that "0" in the test file is due to a bug in ffmpeg (and probably
> other
> > muxers) where this value is not written as a relative timestamp, but
> > instead as the timestamp of the previous frame. https://github.com/FFmp
> > eg/FFmpeg/blob/master/libavformat/matroskaenc.c#L2053
> >  2FFFmpeg%2FFFmpeg%2Fblob%2Fmaster%2Flibavformat%
> 2Fmatroskaenc.c%23L2053=D=1=AFQjCNGs8m6GsWbhTvCZl0Q_juGAldQblA
> >
>
> Just a few lines below this reads
>
>mkv->last_track_timestamp[track_number - 1] = ts - mkv->cluster_pts;
>
> which looks like it intends to write a relative value. Though "ts" can
> be a DTS, while the other value is always a PTS.
>
> > Anyway, I'm all for following Haali. Its not obvious how best to do
> this. I
> > don't think there's any great default value to indicate "not-set" and the
> > generic embl parsing code that reads the reference timestamp doesn't
> really
> > lend itself to setting an additional field like
> > MatroskaBlock.has_reference. I can sort this out, but I'll pause in case
> > you have a recommendation in-mind.
>
> I don't know a nice way either. Here are 3 potential ways I came up
> with:
>
> 1. Use a better dummy value, like INT64_MIN, which is almost 100%
>unlikely to appear in a valid file. (This is probably equivalent to
>your intention in original patch.)
> 2. Add a new "presence_flag_offset" field to EbmlSyntax - the EBML
>reader will write a 1 to it if the element was found. (This is a bit
>annoying, because 0 is a valid offset, and you surely wouldn't want
>to change all the other element definitions.)
> 3. Add a new type like EBML_PRESENCE, which writes whether the element
>was present or not to data_offset, instead of the element's content
>or its default value. (There are some other special "types" like
>EBML_STOP, so maybe this idea isn't all too hacky.)
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>


Sorry, for crossed threads - thanks for these ideas. I like the first
option best. Will upload a new patch using INT64_MIN and add the break; you
pointed out earlier.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2017-01-31 Thread Chrome Cunningham

On Tuesday, January 31, 2017, Chris Cunningham 
wrote:

>
> On Tue, Jan 31, 2017 at 1:07 AM, wm4  > wrote:
>
>> On Tue, 31 Jan 2017 09:57:24 +0100
>> wm4 > > wrote:
>>
>> > On Mon, 30 Jan 2017 17:05:49 -0800
>> > Chris Cunningham > > wrote:
>> >
>> > > Blocks are marked as key frames whenever the "reference" field is
>> > > zero. This is incorrect for non-keyframe Blocks that take a refernce
>> > > on a keyframe at time zero.
>> > >
>> > > Now using -1 to denote "no reference".
>> > >
>> > > Reported to chromium at http://crbug.com/497889 (contains sample)
>> > > ---
>> > >  libavformat/matroskadec.c | 9 ++---
>> > >  1 file changed, 6 insertions(+), 3 deletions(-)
>> > >
>> > > diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
>> > > index e6737a70b2..0d033b574c 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_tu;
>> > >  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 = -1 } },
>> > >  { MATROSKA_ID_CODECSTATE, EBML_NONE },
>> > >  {  1, EBML_UINT, 0,
>> offsetof(MatroskaBlock, non_simple), { .u = 1 } },
>> > >  { 0 }
>> > > @@ -1071,6 +1072,8 @@ 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;
>> > >  case EBML_UINT:
>> >
>> > Isn't there a break missing?
>> >
>> > >  *(uint64_t *) ((char *) data + syntax[i].data_offset) =
>> syntax[i].def.u;
>> > >  break;
>> > > @@ -3361,7 +3364,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 == -1 : -1;
>> > >  uint8_t* additional = blocks[i].additional.size > 0 ?
>> > >  blocks[i].additional.data : NULL;
>> > >  if (!blocks[i].non_simple)
>> > > @@ -3399,7 +3402,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 == -1 : -1;
>> > >  res = matroska_parse_block(matroska, blocks[i].bin.data,
>> > > blocks[i].bin.size,
>> blocks[i].bin.pos,
>> > > cluster.timecode,
>> blocks[i].duration,
>> >
>> > I don't quite trust this. The file has negative block references too
>> > (what do they even mean?). E.g. one block uses "-123". This doesn't
>> > make much sense to me, and at the very least it means -1 is not a safe
>> > dummy value (because negative values don't mean non-keyframe according
>> > to your patch, while -1 as exception does).
>> >
>> > The oldest/most used (until recently at least) mkv demuxer, Haali
>> > actually does every block reference element as a non-keyframe:
>> >
>> > http://git.1f0.de/gitweb?p=ffmpeg.git;a=blob;f=libavformat/
>> MatroskaParser.c;h=173c2e1c20da59d4cf0b501639c470331cd4515f;hb=HEAD#l2354
>> >
>> > This seems much safer.
>> >
>> > Do you have any insight why the file contains such erratic seeming
>> > reference values? I'm sure I'm missing something. Or is it a broken
>> > muxer/broken

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

2017-01-31 Thread wm4

On Tue, 31 Jan 2017 12:02:01 -0800
Chris Cunningham  wrote:

> Thanks for taking a look.
> 
> Definitely missing a "break;" - will fix in subsequent patch.
> 
> Agree timestamps should be relative (didn't realize this). Vignesh points
> out that "0" in the test file is due to a bug in ffmpeg (and probably other
> muxers) where this value is not written as a relative timestamp, but
> instead as the timestamp of the previous frame. https://github.com/FFmp
> eg/FFmpeg/blob/master/libavformat/matroskaenc.c#L2053
> 

Just a few lines below this reads

   mkv->last_track_timestamp[track_number - 1] = ts - mkv->cluster_pts;

which looks like it intends to write a relative value. Though "ts" can
be a DTS, while the other value is always a PTS.

> Anyway, I'm all for following Haali. Its not obvious how best to do this. I
> don't think there's any great default value to indicate "not-set" and the
> generic embl parsing code that reads the reference timestamp doesn't really
> lend itself to setting an additional field like
> MatroskaBlock.has_reference. I can sort this out, but I'll pause in case
> you have a recommendation in-mind.

I don't know a nice way either. Here are 3 potential ways I came up
with:

1. Use a better dummy value, like INT64_MIN, which is almost 100%
   unlikely to appear in a valid file. (This is probably equivalent to
   your intention in original patch.)
2. Add a new "presence_flag_offset" field to EbmlSyntax - the EBML
   reader will write a 1 to it if the element was found. (This is a bit
   annoying, because 0 is a valid offset, and you surely wouldn't want
   to change all the other element definitions.)
3. Add a new type like EBML_PRESENCE, which writes whether the element
   was present or not to data_offset, instead of the element's content
   or its default value. (There are some other special "types" like
   EBML_STOP, so maybe this idea isn't all too hacky.)
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2017-01-31 Thread Chris Cunningham

On Tue, Jan 31, 2017 at 1:07 AM, wm4  wrote:

> On Tue, 31 Jan 2017 09:57:24 +0100
> wm4  wrote:
>
> > On Mon, 30 Jan 2017 17:05:49 -0800
> > Chris Cunningham  wrote:
> >
> > > Blocks are marked as key frames whenever the "reference" field is
> > > zero. This is incorrect for non-keyframe Blocks that take a refernce
> > > on a keyframe at time zero.
> > >
> > > Now using -1 to denote "no reference".
> > >
> > > Reported to chromium at http://crbug.com/497889 (contains sample)
> > > ---
> > >  libavformat/matroskadec.c | 9 ++---
> > >  1 file changed, 6 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> > > index e6737a70b2..0d033b574c 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_tu;
> > >  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 = -1 } },
> > >  { MATROSKA_ID_CODECSTATE, EBML_NONE },
> > >  {  1, EBML_UINT, 0,
> offsetof(MatroskaBlock, non_simple), { .u = 1 } },
> > >  { 0 }
> > > @@ -1071,6 +1072,8 @@ 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;
> > >  case EBML_UINT:
> >
> > Isn't there a break missing?
> >
> > >  *(uint64_t *) ((char *) data + syntax[i].data_offset) =
> syntax[i].def.u;
> > >  break;
> > > @@ -3361,7 +3364,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 == -1 : -1;
> > >  uint8_t* additional = blocks[i].additional.size > 0 ?
> > >  blocks[i].additional.data : NULL;
> > >  if (!blocks[i].non_simple)
> > > @@ -3399,7 +3402,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 == -1 : -1;
> > >  res = matroska_parse_block(matroska, blocks[i].bin.data,
> > > blocks[i].bin.size,
> blocks[i].bin.pos,
> > > cluster.timecode,
> blocks[i].duration,
> >
> > I don't quite trust this. The file has negative block references too
> > (what do they even mean?). E.g. one block uses "-123". This doesn't
> > make much sense to me, and at the very least it means -1 is not a safe
> > dummy value (because negative values don't mean non-keyframe according
> > to your patch, while -1 as exception does).
> >
> > The oldest/most used (until recently at least) mkv demuxer, Haali
> > actually does every block reference element as a non-keyframe:
> >
> > http://git.1f0.de/gitweb?p=ffmpeg.git;a=blob;f=
> libavformat/MatroskaParser.c;h=173c2e1c20da59d4cf0b501639c470
> 331cd4515f;hb=HEAD#l2354
> >
> > This seems much safer.
> >
> > Do you have any insight why the file contains such erratic seeming
> > reference values? I'm sure I'm missing something. Or is it a broken
> > muxer/broken file?
>
> Oh, nevermind. The values in the reference elements are
> supposed to be _relative_ timestamps. This means -1 is still not a safe
> dummy value. But then, what is a value of "0" supposed to mean?
>
> Going after Haali seems the safest fix, as it most likely won't break
> anything.
> ___
> ffmpeg-devel mailing list
>

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

2017-01-31 Thread wm4

On Tue, 31 Jan 2017 09:57:24 +0100
wm4  wrote:

> On Mon, 30 Jan 2017 17:05:49 -0800
> Chris Cunningham  wrote:
> 
> > Blocks are marked as key frames whenever the "reference" field is
> > zero. This is incorrect for non-keyframe Blocks that take a refernce
> > on a keyframe at time zero.
> > 
> > Now using -1 to denote "no reference".
> > 
> > Reported to chromium at http://crbug.com/497889 (contains sample)
> > ---
> >  libavformat/matroskadec.c | 9 ++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> > index e6737a70b2..0d033b574c 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_tu;
> >  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 = -1 } },
> >  { MATROSKA_ID_CODECSTATE, EBML_NONE },
> >  {  1, EBML_UINT, 0, offsetof(MatroskaBlock, 
> > non_simple), { .u = 1 } },
> >  { 0 }
> > @@ -1071,6 +1072,8 @@ 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;
> >  case EBML_UINT:  
> 
> Isn't there a break missing?
> 
> >  *(uint64_t *) ((char *) data + syntax[i].data_offset) = 
> > syntax[i].def.u;
> >  break;
> > @@ -3361,7 +3364,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 
> > == -1 : -1;
> >  uint8_t* additional = blocks[i].additional.size > 0 ?
> >  blocks[i].additional.data : NULL;
> >  if (!blocks[i].non_simple)
> > @@ -3399,7 +3402,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 
> > == -1 : -1;
> >  res = matroska_parse_block(matroska, blocks[i].bin.data,
> > blocks[i].bin.size, 
> > blocks[i].bin.pos,
> > cluster.timecode, 
> > blocks[i].duration,  
> 
> I don't quite trust this. The file has negative block references too
> (what do they even mean?). E.g. one block uses "-123". This doesn't
> make much sense to me, and at the very least it means -1 is not a safe
> dummy value (because negative values don't mean non-keyframe according
> to your patch, while -1 as exception does).
> 
> The oldest/most used (until recently at least) mkv demuxer, Haali
> actually does every block reference element as a non-keyframe:
> 
> http://git.1f0.de/gitweb?p=ffmpeg.git;a=blob;f=libavformat/MatroskaParser.c;h=173c2e1c20da59d4cf0b501639c470331cd4515f;hb=HEAD#l2354
> 
> This seems much safer.
> 
> Do you have any insight why the file contains such erratic seeming
> reference values? I'm sure I'm missing something. Or is it a broken
> muxer/broken file?

Oh, nevermind. The values in the reference elements are
supposed to be _relative_ timestamps. This means -1 is still not a safe
dummy value. But then, what is a value of "0" supposed to mean?

Going after Haali seems the safest fix, as it most likely won't break
anything.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2017-01-31 Thread wm4

On Mon, 30 Jan 2017 17:05:49 -0800
Chris Cunningham  wrote:

> Blocks are marked as key frames whenever the "reference" field is
> zero. This is incorrect for non-keyframe Blocks that take a refernce
> on a keyframe at time zero.
> 
> Now using -1 to denote "no reference".
> 
> Reported to chromium at http://crbug.com/497889 (contains sample)
> ---
>  libavformat/matroskadec.c | 9 ++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> index e6737a70b2..0d033b574c 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_tu;
>  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 = -1 } },
>  { MATROSKA_ID_CODECSTATE, EBML_NONE },
>  {  1, EBML_UINT, 0, offsetof(MatroskaBlock, 
> non_simple), { .u = 1 } },
>  { 0 }
> @@ -1071,6 +1072,8 @@ 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;
>  case EBML_UINT:

Isn't there a break missing?

>  *(uint64_t *) ((char *) data + syntax[i].data_offset) = 
> syntax[i].def.u;
>  break;
> @@ -3361,7 +3364,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 == 
> -1 : -1;
>  uint8_t* additional = blocks[i].additional.size > 0 ?
>  blocks[i].additional.data : NULL;
>  if (!blocks[i].non_simple)
> @@ -3399,7 +3402,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 == 
> -1 : -1;
>  res = matroska_parse_block(matroska, blocks[i].bin.data,
> blocks[i].bin.size, blocks[i].bin.pos,
> cluster.timecode, blocks[i].duration,

I don't quite trust this. The file has negative block references too
(what do they even mean?). E.g. one block uses "-123". This doesn't
make much sense to me, and at the very least it means -1 is not a safe
dummy value (because negative values don't mean non-keyframe according
to your patch, while -1 as exception does).

The oldest/most used (until recently at least) mkv demuxer, Haali
actually does every block reference element as a non-keyframe:

http://git.1f0.de/gitweb?p=ffmpeg.git;a=blob;f=libavformat/MatroskaParser.c;h=173c2e1c20da59d4cf0b501639c470331cd4515f;hb=HEAD#l2354

This seems much safer.

Do you have any insight why the file contains such erratic seeming
reference values? I'm sure I'm missing something. Or is it a broken
muxer/broken file?
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2017-01-30 Thread Chris Cunningham

Blocks are marked as key frames whenever the "reference" field is
zero. This is incorrect for non-keyframe Blocks that take a refernce
on a keyframe at time zero.

Now using -1 to denote "no reference".

Reported to chromium at http://crbug.com/497889 (contains sample)
---
 libavformat/matroskadec.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
index e6737a70b2..0d033b574c 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_tu;
 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 = -1 } },
 { MATROSKA_ID_CODECSTATE, EBML_NONE },
 {  1, EBML_UINT, 0, offsetof(MatroskaBlock, 
non_simple), { .u = 1 } },
 { 0 }
@@ -1071,6 +1072,8 @@ 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;
 case EBML_UINT:
 *(uint64_t *) ((char *) data + syntax[i].data_offset) = 
syntax[i].def.u;
 break;
@@ -3361,7 +3364,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 == -1 
: -1;
 uint8_t* additional = blocks[i].additional.size > 0 ?
 blocks[i].additional.data : NULL;
 if (!blocks[i].non_simple)
@@ -3399,7 +3402,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 == -1 
: -1;
 res = matroska_parse_block(matroska, blocks[i].bin.data,
blocks[i].bin.size, blocks[i].bin.pos,
cluster.timecode, blocks[i].duration,
-- 
2.11.0.483.g087da7b7c-goog

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

15 matches

Site Navigation

Mail list logo

Footer information