from:"Chris Cunningham"

[FFmpeg-devel] [PATCH] avformat/oggparseogm: unknown codec triggers error

2019-06-13 Thread Chris Cunningham

Only "succeed" to read a header if the codec is valid. Otherwise
return AVERROR_INVALIDDATA.
---
 libavformat/oggparseogm.c | 12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/libavformat/oggparseogm.c b/libavformat/oggparseogm.c
index a07453760b..e71298d39a 100644
--- a/libavformat/oggparseogm.c
+++ b/libavformat/oggparseogm.c
@@ -24,9 +24,9 @@
 
 #include 
 
-#include "libavutil/intreadwrite.h"
-
+#include "libavcodec/avcodec.h"
 #include "libavcodec/bytestream.h"
+#include "libavutil/intreadwrite.h"
 
 #include "avformat.h"
 #include "internal.h"
@@ -58,6 +58,8 @@ ogm_header(AVFormatContext *s, int idx)
 tag = bytestream2_get_le32(&p);
 st->codecpar->codec_id = ff_codec_get_id(ff_codec_bmp_tags, tag);
 st->codecpar->codec_tag = tag;
+if (st->codecpar->codec_id == AV_CODEC_ID_NONE)
+return AVERROR_INVALIDDATA;
 if (st->codecpar->codec_id == AV_CODEC_ID_MPEG4)
 st->need_parsing = AVSTREAM_PARSE_HEADERS;
 } else if (bytestream2_peek_byte(&p) == 't') {
@@ -73,6 +75,8 @@ ogm_header(AVFormatContext *s, int idx)
 acid[4] = 0;
 cid = strtol(acid, NULL, 16);
 st->codecpar->codec_id = ff_codec_get_id(ff_codec_wav_tags, cid);
+if (st->codecpar->codec_id == AV_CODEC_ID_NONE)
+return AVERROR_INVALIDDATA;
 // our parser completely breaks AAC in Ogg
 if (st->codecpar->codec_id != AV_CODEC_ID_AAC)
 st->need_parsing = AVSTREAM_PARSE_FULL;
@@ -147,6 +151,8 @@ ogm_dshow_header(AVFormatContext *s, int idx)
 
 st->codecpar->codec_type = AVMEDIA_TYPE_VIDEO;
 st->codecpar->codec_id = ff_codec_get_id(ff_codec_bmp_tags, AV_RL32(p 
+ 68));
+if (st->codecpar->codec_id == AV_CODEC_ID_NONE)
+return AVERROR_INVALIDDATA;
 avpriv_set_pts_info(st, 64, AV_RL64(p + 164), 1000);
 st->codecpar->width = AV_RL32(p + 176);
 st->codecpar->height = AV_RL32(p + 180);
@@ -156,6 +162,8 @@ ogm_dshow_header(AVFormatContext *s, int idx)
 
 st->codecpar->codec_type = AVMEDIA_TYPE_AUDIO;
 st->codecpar->codec_id = ff_codec_get_id(ff_codec_wav_tags, AV_RL16(p 
+ 124));
+if (st->codecpar->codec_id == AV_CODEC_ID_NONE)
+return AVERROR_INVALIDDATA;
 st->codecpar->channels = AV_RL16(p + 126);
 st->codecpar->sample_rate = AV_RL32(p + 128);
 st->codecpar->bit_rate = AV_RL32(p + 132) * 8;
-- 
2.22.0.rc2.383.gf4fbbf30c2-goog

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] avformat/oggparseogm: unknown codec triggers error

2019-06-13 Thread Chris Cunningham

+James

This is  patch is a follow up from an earlier thread:
https://patchwork.ffmpeg.org/patch/11983/

I've implemented the easy part of that proposal. We also discussed
disallowing multiple headers in an ogm change (generally a revert of:
https://github.com/FFmpeg/FFmpeg/commit/81b743eb1026547270b88ac6a5cb451a3907ee94?diff=split#diff-efd47f302b213c4dc0e419c52a192819L384),
but this was a little out of my depth and not critical to address my
particular bug

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH] avformat/oggdec: only parse headers before data

2019-06-17 Thread Chris Cunningham

This behavior was added in 2010 to suport some old (and invalid) ogm
files. 
https://github.com/FFmpeg/FFmpeg/commit/81b743eb1026547270b88ac6a5cb451a3907ee94

But this makes it possible to change the codec in the later headers,
causing codec to be out of sync with internal avctx (eventually
triggering Abrt). Updating the internal ctx for this degenerate case
was deemed not worth it. See discussion here:
https://patchwork.ffmpeg.org/patch/11983/
---
 libavformat/oggdec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavformat/oggdec.c b/libavformat/oggdec.c
index e815f42134..19d77f3107 100644
--- a/libavformat/oggdec.c
+++ b/libavformat/oggdec.c
@@ -545,7 +545,7 @@ static int ogg_packet(AVFormatContext *s, int *sid, int 
*dstart, int *dsize,
 ogg->curidx= idx;
 os->incomplete = 0;
 
-if (os->header) {
+if (!ogg->headers) {
 if ((ret = os->codec->header(s, idx)) < 0) {
 av_log(s, AV_LOG_ERROR, "Header processing failed: %s\n", 
av_err2str(ret));
 return ret;
-- 
2.22.0.410.gd8fdbe21b5-goog

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] avformat/oggparseogm: unknown codec triggers error

2019-06-17 Thread Chris Cunningham

New patch implements the other half of James suggestion (stop parsing
headers after data) and does not include the AVERROR_INVALIDDATA returns.
http://ffmpeg.org/pipermail/ffmpeg-devel/2019-June/245502.html

On Sun, Jun 16, 2019 at 6:16 AM Reimar Döffinger 
wrote:

> On 14.06.2019, at 17:01, James Almer  wrote:
>
> > On 6/14/2019 11:52 AM, Reimar Döffinger wrote:
> >>
> >>
> >> On 14.06.2019, at 03:15, Chris Cunningham 
> wrote:
> >>
> >>> Only "succeed" to read a header if the codec is valid. Otherwise
> >>> return AVERROR_INVALIDDATA.
> >>
> >> That doesn't sound right to me, an unknown codec in (possibly) a single
> stream is not an error.
> >> I understood the discussion more to say the if it's an unknown codec,
> we should not try to override valid codec configuration with a broken one.
> >
> > I did request this change, seeing that returning codec_id none in this
> > scenario results in a crash at a later point due to conflicting
> parameters.
> >
> > Do you suggest we should limit the change to only reject any duplicate
> > header that may show up after the first one (and before the first data
> > packet)?
>
> I don't know or understand the details, but I understood the suggestion as
> "do not override a valid codec ID to NONE".
> Either way, I would have suggested only skipping the affected stream - but
> I admit I have not checked how far-reaching it is to return an error here.
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] avformat/oggdec: only parse headers before data

2019-06-17 Thread Chris Cunningham

+James

On Mon, Jun 17, 2019 at 6:31 PM Chris Cunningham 
wrote:

> This behavior was added in 2010 to suport some old (and invalid) ogm
> files.
> https://github.com/FFmpeg/FFmpeg/commit/81b743eb1026547270b88ac6a5cb451a3907ee94
>
> But this makes it possible to change the codec in the later headers,
> causing codec to be out of sync with internal avctx (eventually
> triggering Abrt). Updating the internal ctx for this degenerate case
> was deemed not worth it. See discussion here:
> https://patchwork.ffmpeg.org/patch/11983/
> ---
>  libavformat/oggdec.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libavformat/oggdec.c b/libavformat/oggdec.c
> index e815f42134..19d77f3107 100644
> --- a/libavformat/oggdec.c
> +++ b/libavformat/oggdec.c
> @@ -545,7 +545,7 @@ static int ogg_packet(AVFormatContext *s, int *sid,
> int *dstart, int *dsize,
>  ogg->curidx= idx;
>  os->incomplete = 0;
>
> -if (os->header) {
> +if (!ogg->headers) {
>  if ((ret = os->codec->header(s, idx)) < 0) {
>  av_log(s, AV_LOG_ERROR, "Header processing failed: %s\n",
> av_err2str(ret));
>  return ret;
> --
> 2.22.0.410.gd8fdbe21b5-goog
>
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] avformat/oggdec: only parse headers before data

2019-06-19 Thread Chris Cunningham

On Wed, Jun 19, 2019 at 11:25 AM Michael Niedermayer 
wrote:

> breaks:
> ./ffmpeg -i bgc.sub.dub.ogm -vframes 3 -y test.webm
> sample: http://samples.mplayerhq.hu/ogg/bgc.sub.dub.ogm
>
> [...]
>
> --
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>

Thanks Michael.

That file is example of the "invalid" sort we were discussing previously.
As I understand it, the spec requires that data packets not be interleaved
with header packets - the headers for all streams should conclude before
data packets begin. In this file we see a few headers followed by a data
packet (stream 0), followed by more headers for streams 1 - 3.

We knew this change would break such files. Can we live it? James, any
workaround? If not I'm still open to
setting st->internal->need_context_update as discussed in the earlier patch
(https://patchwork.ffmpeg.org/patch/11983/)

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] avformat/oggdec: only parse headers before data

2019-06-19 Thread Chris Cunningham

On Wed, Jun 19, 2019 at 7:11 PM Chris Cunningham 
wrote:

> On Wed, Jun 19, 2019 at 11:25 AM Michael Niedermayer
>  wrote:
>
>> breaks:
>> ./ffmpeg -i bgc.sub.dub.ogm -vframes 3 -y test.webm
>> sample: http://samples.mplayerhq.hu/ogg/bgc.sub.dub.ogm
>>
>> [...]
>>
>> --
>> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>>
>
> Thanks Michael.
>
> That file is example of the "invalid" sort we were discussing previously.
> As I understand it, the spec requires that data packets not be interleaved
> with header packets - the headers for all streams should conclude before
> data packets begin. In this file we see a few headers followed by a data
> packet (stream 0), followed by more headers for streams 1 - 3.
>
> We knew this change would break such files. Can we live it? James, any
> workaround? If not I'm still open to
> setting st->internal->need_context_update as discussed in the earlier patch
> (https://patchwork.ffmpeg.org/patch/11983/)
>
> Chris
>

Quick refresher on my fuzzer input

avformat_find_stream_info finds 3 streams
[0] AV_CODEC_ID_OPUS
[1] AV_CODEC_ID_TEXT
[2] AV_CODEC_ID_NONE

A bit later we're reading frames out and stream 2 encounters a header that
changes the codec from NONE -> AV_CODEC_ID_GSM_MS. ogm_header() assigns
this to codecpar->codec_id, but st->internal->avctx->codec_id is never
updated (remains NONE). This mismatch ultimately triggers an assert0 in
gsm_parse (expecting only gsm codecs).

Things tried so far:
- keep codec internal in sync (need_context_update = 1)
https://patchwork.ffmpeg.org/patch/11983/
- don't allow codec id = none in ogm parse
https://patchwork.ffmpeg.org/patch/13529/
- and disallow headers after data (this thread)

I think this last one misses the mark slightly. It happens that the a codec
change corresponds with a header that comes after data packets start, but I
think it could just as easily have occurred without that interleaved data
packet. The root question is whether there is some way to know "this header
is garbage" ... but a header that assigns a reasonable gsm codec
(particularly when the codec was not yet known) seems pretty reasonable.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] avformat/oggparseogm: sync avctx w/ codecpar

2019-06-20 Thread Chris Cunningham

On Thu, Feb 28, 2019 at 9:13 AM James Almer  wrote:

> On 2/26/2019 10:18 PM, Chris Cunningham wrote:
> > On Thu, Feb 21, 2019 at 4:46 PM Chris Cunningham
> > mailto:chcunning...@chromium.org>> wrote:
> >
> > I'm fine to do either. James, do you still prefer to skip the later
> > headers if this breaks some old ogm files?
> >
> >
> > James, friendly ping
>
> Yes, i'd prefer if we skip any superfluous parameter header that shows
> up before the first data packet.
> The return value of ff_codec_get_id() should also be checked to not
> propagate AV_CODEC_ID_NONE, which was the source of this issue.
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Renewing request to apply for this patch.

The alternate route of identifying/skipping the bad packets was more
difficult than expected (https://patchwork.ffmpeg.org/patch/13593/). James
signed on this approach instead.

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] avformat/oggparseogm: sync avctx w/ codecpar

2019-06-25 Thread Chris Cunningham

Friendly ping.

On Thu, Jun 20, 2019 at 11:17 AM Chris Cunningham 
wrote:

> On Thu, Feb 28, 2019 at 9:13 AM James Almer  wrote:
>
>> On 2/26/2019 10:18 PM, Chris Cunningham wrote:
>> > On Thu, Feb 21, 2019 at 4:46 PM Chris Cunningham
>> > mailto:chcunning...@chromium.org>> wrote:
>> >
>> > I'm fine to do either. James, do you still prefer to skip the later
>> > headers if this breaks some old ogm files?
>> >
>> >
>> > James, friendly ping
>>
>> Yes, i'd prefer if we skip any superfluous parameter header that shows
>> up before the first data packet.
>> The return value of ff_codec_get_id() should also be checked to not
>> propagate AV_CODEC_ID_NONE, which was the source of this issue.
>> ___
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>
> Renewing request to apply for this patch.
>
> The alternate route of identifying/skipping the bad packets was more
> difficult than expected (https://patchwork.ffmpeg.org/patch/13593/).
> James signed on this approach instead.
>
> Chris
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] vp9_parser: set profile in AVCodecContext

2018-10-24 Thread Chris Cunningham

On Tue, Oct 23, 2018 at 6:28 PM James Almer  wrote:

> On 10/23/2018 10:16 PM, chcunningham wrote:
> > ---
> >  libavcodec/vp9_parser.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/libavcodec/vp9_parser.c b/libavcodec/vp9_parser.c
> > index 9531f34a32..b6b621198b 100644
> > --- a/libavcodec/vp9_parser.c
> > +++ b/libavcodec/vp9_parser.c
> > @@ -43,6 +43,8 @@ static int parse(AVCodecParserContext *ctx,
> >  profile |= get_bits1(&gb) << 1;
> >  if (profile == 3) profile += get_bits1(&gb);
> >
> > +avctx->profile = profile;
> > +
> >  if (get_bits1(&gb)) {
> >  keyframe = 0;
> >  } else {
> >
>
> I already sent a patch for this earlier this month.
> https://patchwork.ffmpeg.org/patch/10602/
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


And yours is much more complete. Ignore this one.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avcodec/vp9_parser: export profile and pixel format

2018-10-24 Thread Chris Cunningham

On Wed, Oct 3, 2018 at 10:49 AM James Almer  wrote:

> Signed-off-by: James Almer 
> ---
>  libavcodec/vp9_parser.c | 82 -
>  1 file changed, 80 insertions(+), 2 deletions(-)
>
> diff --git a/libavcodec/vp9_parser.c b/libavcodec/vp9_parser.c
> index 9531f34a32..d4110f20bf 100644
> --- a/libavcodec/vp9_parser.c
> +++ b/libavcodec/vp9_parser.c
> @@ -23,36 +23,114 @@
>
>  #include "libavutil/intreadwrite.h"
>  #include "libavcodec/get_bits.h"
> +#include "libavcodec/internal.h"
>  #include "parser.h"
>
> +#define VP9_SYNCCODE 0x498342
> +
> +static int read_colorspace_details(AVCodecParserContext *ctx,
> AVCodecContext *avctx,
> +   GetBitContext *gb)
> +{
> +static const enum AVColorSpace colorspaces[8] = {
> +AVCOL_SPC_UNSPECIFIED, AVCOL_SPC_BT470BG, AVCOL_SPC_BT709,
> AVCOL_SPC_SMPTE170M,
> +AVCOL_SPC_SMPTE240M, AVCOL_SPC_BT2020_NCL, AVCOL_SPC_RESERVED,
> AVCOL_SPC_RGB,
> +};
> +int colorspace, bits = avctx->profile <= 1 ? 0 : 1 + get_bits1(gb);
> // 0:8, 1:10, 2:12
> +
> +colorspace = colorspaces[get_bits(gb, 3)];
> +if (colorspace == AVCOL_SPC_RGB) { // RGB = profile 1
> +static const enum AVPixelFormat pix_fmt_rgb[3] = {
> +AV_PIX_FMT_GBRP, AV_PIX_FMT_GBRP10, AV_PIX_FMT_GBRP12
> +};
> +if (avctx->profile & 1) {
> +if (get_bits1(gb)) // reserved bit
> +return AVERROR_INVALIDDATA;
> +} else
> +return AVERROR_INVALIDDATA;
> +ctx->format = pix_fmt_rgb[bits];
> +} else {
> +static const enum AVPixelFormat pix_fmt_for_ss[3][2 /* v */][2 /*
> h */] = {
> +{ { AV_PIX_FMT_YUV444P,   AV_PIX_FMT_YUV422P },
> +  { AV_PIX_FMT_YUV440P,   AV_PIX_FMT_YUV420P } },
> +{ { AV_PIX_FMT_YUV444P10, AV_PIX_FMT_YUV422P10 },
> +  { AV_PIX_FMT_YUV440P10, AV_PIX_FMT_YUV420P10 } },
> +{ { AV_PIX_FMT_YUV444P12, AV_PIX_FMT_YUV422P12 },
> +  { AV_PIX_FMT_YUV440P12, AV_PIX_FMT_YUV420P12 } }
> +};
> +if (avctx->profile & 1) {
> +int ss_h, ss_v, format;
> +
> +ss_h = get_bits1(gb);
> +ss_v = get_bits1(gb);
> +format = pix_fmt_for_ss[bits][ss_v][ss_h];
> +if (format == AV_PIX_FMT_YUV420P)
> +// YUV 4:2:0 not supported in profiles 1 and 3
> +return AVERROR_INVALIDDATA;
> +else if (get_bits1(gb)) // color details reserved bit
> +return AVERROR_INVALIDDATA;
> +ctx->format = format;
> +} else {
> +ctx->format = pix_fmt_for_ss[bits][1][1];
> +}
> +}
> +
> +return 0;
> +}
> +
>  static int parse(AVCodecParserContext *ctx,
>   AVCodecContext *avctx,
>   const uint8_t **out_data, int *out_size,
>   const uint8_t *data, int size)
>  {
>  GetBitContext gb;
> -int res, profile, keyframe;
> +int res, profile, keyframe, invisible, errorres;
>
>  *out_data = data;
>  *out_size = size;
>
> -if ((res = init_get_bits8(&gb, data, size)) < 0)
> +if (!size || (res = init_get_bits8(&gb, data, size)) < 0)
>  return size; // parsers can't return errors
>  get_bits(&gb, 2); // frame marker
>  profile  = get_bits1(&gb);
>  profile |= get_bits1(&gb) << 1;
>  if (profile == 3) profile += get_bits1(&gb);
> +if (profile > 3)
> +return size;
>
> +avctx->profile = profile;
>  if (get_bits1(&gb)) {
>  keyframe = 0;
> +skip_bits(&gb, 3);
>  } else {
>  keyframe  = !get_bits1(&gb);
>  }
>
> +invisible = !get_bits1(&gb);
> +errorres  = get_bits1(&gb);
> +
>  if (!keyframe) {
> +int intraonly = invisible ? get_bits1(&gb) : 0;
> +if (!errorres)
> +skip_bits(&gb, 2);
> +if (intraonly) {
> +if (get_bits_long(&gb, 24) != VP9_SYNCCODE) // synccode
> +return size;
> +if (profile >= 1) {
> +if ((read_colorspace_details(ctx, avctx, &gb)) < 0)
> +return size;
> +} else {
> +ctx->format = AV_PIX_FMT_YUV420P;
> +}
> +}
> +
>  ctx->pict_type = AV_PICTURE_TYPE_P;
>  ctx->key_frame = 0;
>  } else {
> +if (get_bits_long(&gb, 24) != VP9_SYNCCODE) // synccode
> +return size;
> +if (read_colorspace_details(ctx, avctx, &gb) < 0)
> +return size;
> +
>  ctx->pict_type = AV_PICTURE_TYPE_I;
>  ctx->key_frame = 1;
>  }
> --
> 2.19.0
>
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


Friendly ping for review.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinf

Re: [FFmpeg-devel] [PATCH] avcodec/vp9_parser: export profile and pixel format

2018-10-29 Thread Chris Cunningham

Thanks for pushing the profile part. Turns out I'm also interested in pixel
format, but I follow your comments about dropping the rest of patch. With
the code as-is, how busted are we when parsing a super frame. Does it
correctly grab the profile of the first frame?

Chrome actually has a fairly complete VP9 parser
<https://cs.chromium.org/chromium/src/media/filters/vp9_parser.cc?rcl=adf255ef1da95255174643a6c1482e4305f877fd&l=478>
but the CodecPrivate is no longer set in extradata for VP9 so we don't
(AFAIK) have easy access to the frame header. Would you be open to
surfacing codec private via extradata (reverting this change
<http://ffmpeg.org/pipermail/ffmpeg-devel/2018-February/225548.html>)?

On Thu, Oct 25, 2018 at 4:50 PM James Almer  wrote:

> On 10/24/2018 3:12 PM, Chris Cunningham wrote:
> > On Wed, Oct 3, 2018 at 10:49 AM James Almer  wrote:
> >
> >> Signed-off-by: James Almer 
> >> ---
> >>  libavcodec/vp9_parser.c | 82 -
> >>  1 file changed, 80 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/libavcodec/vp9_parser.c b/libavcodec/vp9_parser.c
> >> index 9531f34a32..d4110f20bf 100644
> >> --- a/libavcodec/vp9_parser.c
> >> +++ b/libavcodec/vp9_parser.c
> >> @@ -23,36 +23,114 @@
> >>
> >>  #include "libavutil/intreadwrite.h"
> >>  #include "libavcodec/get_bits.h"
> >> +#include "libavcodec/internal.h"
> >>  #include "parser.h"
> >>
> >> +#define VP9_SYNCCODE 0x498342
> >> +
> >> +static int read_colorspace_details(AVCodecParserContext *ctx,
> >> AVCodecContext *avctx,
> >> +   GetBitContext *gb)
> >> +{
> >> +static const enum AVColorSpace colorspaces[8] = {
> >> +AVCOL_SPC_UNSPECIFIED, AVCOL_SPC_BT470BG, AVCOL_SPC_BT709,
> >> AVCOL_SPC_SMPTE170M,
> >> +AVCOL_SPC_SMPTE240M, AVCOL_SPC_BT2020_NCL, AVCOL_SPC_RESERVED,
> >> AVCOL_SPC_RGB,
> >> +};
> >> +int colorspace, bits = avctx->profile <= 1 ? 0 : 1 + get_bits1(gb);
> >> // 0:8, 1:10, 2:12
> >> +
> >> +colorspace = colorspaces[get_bits(gb, 3)];
> >> +if (colorspace == AVCOL_SPC_RGB) { // RGB = profile 1
> >> +static const enum AVPixelFormat pix_fmt_rgb[3] = {
> >> +AV_PIX_FMT_GBRP, AV_PIX_FMT_GBRP10, AV_PIX_FMT_GBRP12
> >> +};
> >> +if (avctx->profile & 1) {
> >> +if (get_bits1(gb)) // reserved bit
> >> +return AVERROR_INVALIDDATA;
> >> +} else
> >> +return AVERROR_INVALIDDATA;
> >> +ctx->format = pix_fmt_rgb[bits];
> >> +} else {
> >> +static const enum AVPixelFormat pix_fmt_for_ss[3][2 /* v */][2
> /*
> >> h */] = {
> >> +{ { AV_PIX_FMT_YUV444P,   AV_PIX_FMT_YUV422P },
> >> +  { AV_PIX_FMT_YUV440P,   AV_PIX_FMT_YUV420P } },
> >> +{ { AV_PIX_FMT_YUV444P10, AV_PIX_FMT_YUV422P10 },
> >> +  { AV_PIX_FMT_YUV440P10, AV_PIX_FMT_YUV420P10 } },
> >> +{ { AV_PIX_FMT_YUV444P12, AV_PIX_FMT_YUV422P12 },
> >> +  { AV_PIX_FMT_YUV440P12, AV_PIX_FMT_YUV420P12 } }
> >> +};
> >> +if (avctx->profile & 1) {
> >> +int ss_h, ss_v, format;
> >> +
> >> +ss_h = get_bits1(gb);
> >> +ss_v = get_bits1(gb);
> >> +format = pix_fmt_for_ss[bits][ss_v][ss_h];
> >> +if (format == AV_PIX_FMT_YUV420P)
> >> +// YUV 4:2:0 not supported in profiles 1 and 3
> >> +return AVERROR_INVALIDDATA;
> >> +else if (get_bits1(gb)) // color details reserved bit
> >> +return AVERROR_INVALIDDATA;
> >> +ctx->format = format;
> >> +} else {
> >> +ctx->format = pix_fmt_for_ss[bits][1][1];
> >> +}
> >> +}
> >> +
> >> +return 0;
> >> +}
> >> +
> >>  static int parse(AVCodecParserContext *ctx,
> >>   AVCodecContext *avctx,
> >>   const uint8_t **out_data, int *out_size,
> >>   const uint8_t *data, int size)
> >>  {
> >>  GetBitContext gb;
> >> -int res, profile, keyframe;
> >> +int res, profile, keyframe, invisible, errorres;
> >>
> >>  *

Re: [FFmpeg-devel] [PATCH] avcodec/vp9_parser: export profile and pixel format

2018-11-02 Thread Chris Cunningham

> Also, when dealing with a super frame, to get other values like pixel
> format and frame dimensions we'd have to make sure to parse the visible
> frame, and if it's an inter frame also keep the reference frames around
> to take said values from them.

Could we avoid the challenges around reference and invisible frame by only
doing this parsing for key frames? IIUC these properties would only be
expected to change at the GOP boundary.

> I don't think that change should be reverted, at least not without
> further changes. Otherwise, WebM specific extradata (the one defined in
> https://www.webmproject.org/docs/container) could make its way to other
> containers. We'd have to add checks in supported muxers to ignore it
> instead.

Sorry, I forgot just how different webm is in this regard. We do have some
separate parsing code for the CodecPrivate, but you're also correct that it
is often not set.

> Also, what muxer currently writes that to CodecPrivate? I've looked at
> Youtube samples and none has it.

Maybe libWebM
https://github.com/webmproject/libwebm/blob/01c1d1d76f139345c442bfc8e61b4e1cba809059/common/hdr_util.h#L31

YouTube isn't doing it yet for VP9. But I think its required with AV1
content, so maybe we'll see it with new VP9 encodes too down the road.
Parsing the frame is definitely our best bet.

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

Re: [FFmpeg-devel] [PATCH] avcodec/vp9_parser: export profile and pixel format

2018-11-12 Thread Chris Cunningham

On Fri, Nov 2, 2018 at 4:57 PM Chris Cunningham 
wrote:

> > Also, when dealing with a super frame, to get other values like pixel
> > format and frame dimensions we'd have to make sure to parse the visible
> > frame, and if it's an inter frame also keep the reference frames around
> > to take said values from them.
>
> Could we avoid the challenges around reference and invisible frame by only
> doing this parsing for key frames? IIUC these properties would only be
> expected to change at the GOP boundary.
>

Friendly ping for feedback on this idea.

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

Re: [FFmpeg-devel] [PATCH] lavf/id3v2: fail read_apic on EOF reading mimetype

2018-12-13 Thread Chris Cunningham

>
> Yet another problem that could have been caught by static analysis..
> Wouldn't it be better to always leave the array in a valid state?
>

Will add that in the next patch. It has the extra benefit of protecting the
isv34 branch.

Goto fail; skips a lot of lines that aren't needed if mimetype is empty, so
I think its worth keeping as well. I'd love to do similar for the isv34
branch, but I'm not sure how to detect the condition given the EOF behavior
for  avio_get_str:

 * @return number of bytes read (is always <= maxlen).
 * If reading ends on EOF or error, the return value will be one more than
 * bytes actually read.

How do callers differentiate between cases where you read 5 bytes vs
reading just 4 bytes and hitting an error - IIIUC both cases return 5.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] lavf/mov: ensure only one tkhd per trak

2018-12-13 Thread Chris Cunningham

> I have at least 2 files which have a id of 0
> Iam not sure where they are from so iam not sure i can share them

This was my fear as well. Also, we currently default the ID for a new
stream to be the number of streams now in the list. I worried that some
files may lack a tkhd or could be structured in such a way that they're
dependent on this defaulting and might break if I instead defaulted to
zero.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] lavf/mov: ensure only one tkhd per trak

2018-12-13 Thread Chris Cunningham

>
> "st->id" is not necessary for demuxing AFAIK, please correct me if Im
> wrong.
> Would an init value to -1 for st->id work ?
>

st->id does get used here and there. For ex, mov_read_trun reads the id to
decide which stream corresponds to the current fragment. But if the ID
isn't coming from a tkhd (either because none exits, or because you have
truns before the tkhd appears), perhaps we can consider invalid. Using a
value of -1 would make that easier to detect. It risks breaking some bad
files, but I'm fine with that if you'd support it. I'll send a new patch
and we can see if Michael finds new breaks.

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

Re: [FFmpeg-devel] [PATCH] lavf/mov: ensure only one tkhd per trak

2018-12-13 Thread Chris Cunningham

> But if the ID isn't coming from a tkhd (either because none exits, or
because you have truns before the tkhd appears), perhaps we can consider
invalid.

Taking a closer look at the spec, I think it actually _is valid_ to have
truns before tkhd. They "strongly recommend" that the header boxes like
tkhd be placed first, but its not a "must". No clue how often ffmpeg
encounters such a file, but today's defaulting of id = the number of
streams probably facilitates playback in such cases because the probability
of tkhd's track id matching that default is reasonably high.

Still, I'll take a stab at the approach, if just for discussion/testing.

On Thu, Dec 13, 2018 at 12:39 PM Chris Cunningham 
wrote:

> "st->id" is not necessary for demuxing AFAIK, please correct me if Im
>> wrong.
>> Would an init value to -1 for st->id work ?
>>
>
> st->id does get used here and there. For ex, mov_read_trun reads the id to
> decide which stream corresponds to the current fragment. But if the ID
> isn't coming from a tkhd (either because none exits, or because you have
> truns before the tkhd appears), perhaps we can consider invalid. Using a
> value of -1 would make that easier to detect. It risks breaking some bad
> files, but I'm fine with that if you'd support it. I'll send a new patch
> and we can see if Michael finds new breaks.
>
> Chris
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] lavf/id3v2: fail read_apic on EOF reading mimetype

2018-12-14 Thread Chris Cunningham

>
> -char mimetype[64];
> +char mimetype[64] = {0};
>
> would be enough
>

Agree! Next patch.

I'd love to do similar for the isv34 branch, but I'm not sure how to detect
> the condition given the EOF behavior for  avio_get_str:
>  * @return number of bytes read (is always <= maxlen).
>  * If reading ends on EOF or error, the return value will be one more than
>  * bytes actually read.
> How do callers differentiate between cases where you read 5 bytes vs
> reading just 4 bytes and hitting an error - IIIUC both cases return 5.


Anyone familiar with this? Seems like a bad way to signal EOF.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] lavf/mov: ensure only one tkhd per trak

2018-12-14 Thread Chris Cunningham

>
> from a quick look, i did not find a file this breaks
>

Woot.  Baptiste, I'm happy with this last patch if you are.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avcodec/gsm_parser: return -1 on parse error

2019-01-30 Thread Chris Cunningham

On Wed, Jan 30, 2019 at 1:40 PM James Almer  wrote:

> Parsers can't return negative values, only the output packet size. For
> the purpose of errors, they usually return the entire untouched packet
> size.
>

Understood. Is this documented somewhere? I noted the comment in
av_paser_parse2(), "/* WARNING: the returned index can be negative */", and
guessed that signaled an error.

> And this definitely means there's a bug elsewhere. This parser should
> have not been used for codecs ids other than GSM and GSM_MS. That's
> precisely what this assert() is making sure of.
>

I'll take a closer look at how this parser was selected.

Thanks for the quick reply.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avcodec/gsm_parser: return -1 on parse error

2019-01-30 Thread Chris Cunningham

>
> >> And this definitely means there's a bug elsewhere. This parser should
> >> have not been used for codecs ids other than GSM and GSM_MS. That's
> >> precisely what this assert() is making sure of.
> >>
> >
> > I'll take a closer look at how this parser was selected.
>

Ok, here are full details of how this happens:

   1. AV_CODEC_ID_GSM_MS is assigned to on st->codecpar->codec_id
   in ogm_header() (oggparseogm.c:75)
   2. The (deprecated) st->codec->codec_id is not assigned at that time and
   remains 0 (UNKNOWN)
   3. AV_CODEC_ID_GSM_MS from st->codecpar is passed to av_parser_init,
   selecting the gsm parser (in read_frame_internal())
   4. The fuzzer next (only) packet has a size of 0, so the first call to
   parse_packet() (still in read_frame_internal()) does nothing
   5. After this call we assign several members of st->internal->avctx to
   st->codecpar. This leaves codecpar->codec_id = UNKNOWN. Interestingly, we
   do not reset the st->parser at this point (maybe we should?)
   6. Next iteration of the read_frame_internal() loop we get EOF from
   ff_read_packet. This triggers the "flush the parsers" call to
   parse_packet() which finally reaches gsm_parse() to trigger the abort
   (avctx->codec_id is still 0).

Questions (guessing at the fix):

   - At what point should st->codec->codec_id be synchronized with
   st->codecpar->codec_id? It never is in this test.
   - Should we reset st->parser whenever we reset st->codecpar->codec_id
   (step 5 above)?

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

Re: [FFmpeg-devel] [PATCH] avcodec/gsm_parser: return -1 on parse error

2019-01-31 Thread Chris Cunningham

On Wed, Jan 30, 2019 at 5:43 PM James Almer  wrote:

> On 1/30/2019 10:20 PM, Chris Cunningham wrote:
> >>
> >>>> And this definitely means there's a bug elsewhere. This parser should
> >>>> have not been used for codecs ids other than GSM and GSM_MS. That's
> >>>> precisely what this assert() is making sure of.
> >>>>
> >>>
> >>> I'll take a closer look at how this parser was selected.
> >>
> >
> > Ok, here are full details of how this happens:
> >
> >1. AV_CODEC_ID_GSM_MS is assigned to on st->codecpar->codec_id
> >in ogm_header() (oggparseogm.c:75)
> >2. The (deprecated) st->codec->codec_id is not assigned at that time
> and
> >remains 0 (UNKNOWN)
> >3. AV_CODEC_ID_GSM_MS from st->codecpar is passed to av_parser_init,
> >selecting the gsm parser (in read_frame_internal())
> >4. The fuzzer next (only) packet has a size of 0, so the first call to
> >parse_packet() (still in read_frame_internal()) does nothing
> >5. After this call we assign several members of st->internal->avctx to
> >st->codecpar. This leaves codecpar->codec_id = UNKNOWN.
> Interestingly, we
> >do not reset the st->parser at this point (maybe we should?)
>
> Where does this happen? A call to avcodec_parameters_from_context()
> should also copy codec_id.
>

Ignore #5 here - I'm not seeing that today - was likely confused.

>
> >6. Next iteration of the read_frame_internal() loop we get EOF from
> >ff_read_packet. This triggers the "flush the parsers" call to
> >parse_packet() which finally reaches gsm_parse() to trigger the abort
> >(avctx->codec_id is still 0).
> >
> > Questions (guessing at the fix):
> >
> >- At what point should st->codec->codec_id be synchronized with
> >st->codecpar->codec_id? It never is in this test.
>
> In avformat_find_stream_info(), i think. In any case, st->codec should
> have no effect on parsers. What is used in parse_packet() however is
> st->internal->avctx, so that context is the one that evidently contains
> codec_id == UNKNOWN when it should be in sync with codecpar.
>

We do call avformat_find_stream_info, and avcodec_parameters_from_context
is called during that process. Everything seems OK when
avformat_find_stream_info is done. The codecpar->codec_id is in sync with
internal->avctx->codec_id for all streams.

But then we start calling av_read_frame as part of normal demuxing.
avcodec_parameters_from_context() does not appear to be called during this
process. Eventually we get this stack:

ogm_header
ogg_packet
ogg_read_packet
ff_read_packet
read_frame_internal
av_read_frame

Inside ogm_header, st->codecpar->codec_id is assigned AV_CODEC_ID_GSM_MS
(previous value was codec NONE). But *st->internal->avctx->codec_id is
never updated. It remains NONE for the rest of this test. *

When this unwinds back to read_frame_internal, st->parser is assigned using
this new codec (GSM_MS)
st->parser = av_parser_init(st->codecpar->codec_id);

Later on, still inside read_frame_internal's loop, we get EOF and call,
parse_packet (/* flush the parsers */)
parse_packet() calls av_parser_parse2(), passing st->internal->avctx
(codec_id still NONE, while codecpar still says GSM_MS). This ultimately
gets passed to gsm_parse, which triggers the assert0.

The underlined bit above seems like the root cause. Where should we be
updating st->internal->avctx->codec_id?
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avformat/mov: validate chunk_count vs stsc_data

2019-01-31 Thread Chris Cunningham

Some extra context: this remedies some bad math that otherwise triggers an
abort near the bottom of mov_seek_stream().

My first thought on seeing this was we should probably be
returning AVERROR_INVALIDDATA somewhere. The stsc and stco boxes aren't
agreeing (stco says no chunks, stsc.first points to chunk 135). stsc and
stco aren't gauranteed to come in a particular order, but we could try to
validate that they agree whenever both arrive. One challenge is
that mov_read_stco is already pretty relaxed about setting sc->chunk_count
(it sets to some lower number if we hit EOF during parsing).

Feedback welcome.

On Thu, Jan 31, 2019 at 5:26 PM chcunningham 
wrote:

> Bad content may contain stsc boxes with a first_chunk index that
> exceeds stco.entries (chunk_count).
>
> mov_get_stsc_samples now checks for this and returns 0 when
> values are invalid.
>
> Also updates MOVStsc to use unsigned ints, per spec.
> ---
>  libavformat/isom.h | 6 +++---
>  libavformat/mov.c  | 4 ++--
>  2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/libavformat/isom.h b/libavformat/isom.h
> index e629663949..8e0d8355b3 100644
> --- a/libavformat/isom.h
> +++ b/libavformat/isom.h
> @@ -59,9 +59,9 @@ typedef struct MOVStts {
>  } MOVStts;
>
>  typedef struct MOVStsc {
> -int first;
> -int count;
> -int id;
> +unsigned int first;
> +unsigned int count;
> +unsigned int id;
>  } MOVStsc;
>
>  typedef struct MOVElst {
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index 9b9739f788..dcf4ee8dc1 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -2690,11 +2690,11 @@ static inline int mov_stsc_index_valid(unsigned
> int index, unsigned int count)
>  /* Compute the samples value for the stsc entry at the given index. */
>  static inline int64_t mov_get_stsc_samples(MOVStreamContext *sc, unsigned
> int index)
>  {
> -int chunk_count;
> +unsigned int chunk_count = 0;
>
>  if (mov_stsc_index_valid(index, sc->stsc_count))
>  chunk_count = sc->stsc_data[index + 1].first -
> sc->stsc_data[index].first;
> -else
> +else if (sc->chunk_count >= sc->stsc_data[index].first)
>  chunk_count = sc->chunk_count - (sc->stsc_data[index].first - 1);
>
>  return sc->stsc_data[index].count * (int64_t)chunk_count;
> --
> 2.20.1.611.gfbb209baf1-goog
>
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avformat/isom.h: use usnigned types in MOVStsc

2019-02-04 Thread Chris Cunningham

On Sat, Feb 2, 2019 at 3:37 AM Michael Niedermayer 
wrote:

> Is this change needed by some valid file ?
>

No, just a drive by fix since I happened to be looking at these types and
the spec.


> The change taken on its own is probably correct but its increasing the
> range
> of values without actually adding support for that thus quite possibly
> introducing bugs.
>
> In case of the id, we use signed int for the entries this indexes into,
> so the extended range is not going to work.  also wonder if billions
> of STSD entries in a stream will work when more than 1 cause problems
> already.
>
> Another obvious issue is that currently we scan this table for negative
> values and eliminate them but when this is made unsigned suddenly
> larger values pass through. This has the potential to introduce
> integer overflow bugs in later stages. More so unsigned overflows dont
> show up in asan and these first/count values might affect array indexes
> iam not saying theres a bug here just that its the set of circunstances
> that is fertile for such bugs.
>
> variables related to indexes or counts always require extra scrutiny
> when their type is changed
>

I really appreciate the scrutiny. Given I don't have a file that needs
this, happy to abandon this patch.

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

Re: [FFmpeg-devel] [PATCH] avformat/mov: validate chunk_count vs stsc_data

2019-02-04 Thread Chris Cunningham

On Sat, Feb 2, 2019 at 3:55 AM Michael Niedermayer 
wrote:

> >  static inline int64_t mov_get_stsc_samples(MOVStreamContext *sc,
> unsigned int index)
> >  {
> > -int chunk_count;
> > +unsigned int chunk_count = 0;
> >
> >  if (mov_stsc_index_valid(index, sc->stsc_count))
> >  chunk_count = sc->stsc_data[index + 1].first -
> sc->stsc_data[index].first;
> > -else
> > +else if (sc->chunk_count >= sc->stsc_data[index].first)
> >  chunk_count = sc->chunk_count - (sc->stsc_data[index].first -
> 1);
>
> This construct occurs a 2nd time (in mov_build_index()) is this not
> affected?
>

Didn't notice it, but I think it would be affected. I'll leave this alone
for now, but I'm open to adding a mov_get_chunk_count helper to call from
both mov_build_index and mov_get_stsc_samples.

> mov_read_trak() contains a check for chunk_count and the first index(es)
> (obviously this is not catching this one but)
> is there a reason not to eliminate the inconsistancy at that or some other
> "early" point?
>

Agree this sounds better. Stand by for patch.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avcodec/gsm_parser: return -1 on parse error

2019-02-04 Thread Chris Cunningham

>
> The underlined bit above seems like the root cause. Where should we be
> updating st->internal->avctx->codec_id?
>

Friendly ping for review
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avformat/oggparseogm: sync avctx w/ codecpar

2019-02-06 Thread Chris Cunningham

This is a follow up to feedback in
http://ffmpeg.org/pipermail/ffmpeg-devel/2019-February/239751.html

On Wed, Feb 6, 2019 at 5:13 PM chcunningham 
wrote:

> Codec information may change while reading ogg packets. Update the
> stream's internal avctx to match.
> ---
>  libavformat/oggparseogm.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/libavformat/oggparseogm.c b/libavformat/oggparseogm.c
> index a07453760b..b07a5d55ba 100644
> --- a/libavformat/oggparseogm.c
> +++ b/libavformat/oggparseogm.c
> @@ -114,6 +114,9 @@ ogm_header(AVFormatContext *s, int idx)
>  bytestream2_get_buffer(&p, st->codecpar->extradata,
> st->codecpar->extradata_size);
>  }
>  }
> +
> +// Update internal avctx with changes to codecpar above.
> +st->internal->need_context_update = 1;
>  } else if (bytestream2_peek_byte(&p) == 3) {
>  bytestream2_skip(&p, 7);
>  if (bytestream2_get_bytes_left(&p) > 1)
> --
> 2.20.1.611.gfbb209baf1-goog
>
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avcodec/gsm_parser: return -1 on parse error

2019-02-06 Thread Chris Cunningham

Thanks, this works great. Stand by for patch.

On Wed, Feb 6, 2019 at 8:38 AM Hendrik Leppkes  wrote:

> On Thu, Jan 31, 2019 at 11:29 PM Chris Cunningham
>  wrote:
> >
> > On Wed, Jan 30, 2019 at 5:43 PM James Almer  wrote:
> >
> > > On 1/30/2019 10:20 PM, Chris Cunningham wrote:
> > > >>
> > > >>>> And this definitely means there's a bug elsewhere. This parser
> should
> > > >>>> have not been used for codecs ids other than GSM and GSM_MS.
> That's
> > > >>>> precisely what this assert() is making sure of.
> > > >>>>
> > > >>>
> > > >>> I'll take a closer look at how this parser was selected.
> > > >>
> > > >
> > > > Ok, here are full details of how this happens:
> > > >
> > > >1. AV_CODEC_ID_GSM_MS is assigned to on st->codecpar->codec_id
> > > >in ogm_header() (oggparseogm.c:75)
> > > >2. The (deprecated) st->codec->codec_id is not assigned at that
> time
> > > and
> > > >remains 0 (UNKNOWN)
> > > >3. AV_CODEC_ID_GSM_MS from st->codecpar is passed to
> av_parser_init,
> > > >selecting the gsm parser (in read_frame_internal())
> > > >4. The fuzzer next (only) packet has a size of 0, so the first
> call to
> > > >parse_packet() (still in read_frame_internal()) does nothing
> > > >5. After this call we assign several members of
> st->internal->avctx to
> > > >st->codecpar. This leaves codecpar->codec_id = UNKNOWN.
> > > Interestingly, we
> > > >do not reset the st->parser at this point (maybe we should?)
> > >
> > > Where does this happen? A call to avcodec_parameters_from_context()
> > > should also copy codec_id.
> > >
> >
> > Ignore #5 here - I'm not seeing that today - was likely confused.
> >
> >
> > >
> > > >6. Next iteration of the read_frame_internal() loop we get EOF
> from
> > > >ff_read_packet. This triggers the "flush the parsers" call to
> > > >parse_packet() which finally reaches gsm_parse() to trigger the
> abort
> > > >(avctx->codec_id is still 0).
> > > >
> > > > Questions (guessing at the fix):
> > > >
> > > >- At what point should st->codec->codec_id be synchronized with
> > > >st->codecpar->codec_id? It never is in this test.
> > >
> > > In avformat_find_stream_info(), i think. In any case, st->codec should
> > > have no effect on parsers. What is used in parse_packet() however is
> > > st->internal->avctx, so that context is the one that evidently contains
> > > codec_id == UNKNOWN when it should be in sync with codecpar.
> > >
> >
> > We do call avformat_find_stream_info, and avcodec_parameters_from_context
> > is called during that process. Everything seems OK when
> > avformat_find_stream_info is done. The codecpar->codec_id is in sync with
> > internal->avctx->codec_id for all streams.
> >
> > But then we start calling av_read_frame as part of normal demuxing.
> > avcodec_parameters_from_context() does not appear to be called during
> this
> > process. Eventually we get this stack:
> >
> > ogm_header
> > ogg_packet
> > ogg_read_packet
> > ff_read_packet
> > read_frame_internal
> > av_read_frame
> >
> > Inside ogm_header, st->codecpar->codec_id is assigned AV_CODEC_ID_GSM_MS
> > (previous value was codec NONE). But *st->internal->avctx->codec_id is
> > never updated. It remains NONE for the rest of this test. *
> >
> > When this unwinds back to read_frame_internal, st->parser is assigned
> using
> > this new codec (GSM_MS)
> > st->parser = av_parser_init(st->codecpar->codec_id);
> >
> > Later on, still inside read_frame_internal's loop, we get EOF and call,
> > parse_packet (/* flush the parsers */)
> > parse_packet() calls av_parser_parse2(), passing st->internal->avctx
> > (codec_id still NONE, while codecpar still says GSM_MS). This ultimately
> > gets passed to gsm_parse, which triggers the assert0.
> >
> > The underlined bit above seems like the root cause. Where should we be
> > updating st->internal->avctx->codec_id?
>
> We have a flag to set that causes avformat to fix its internal state
> if a demuxer changes it after the initial opening.
> st->internal->need_context_update = 1
>
> Try setting that at the position where the demuxer changes codecpar
> (ie. in ogm_header?), and see if that resolves it.
>
> - Hendrik
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avformat/mov.c: require tfhd to begin parsing trun

2019-02-07 Thread Chris Cunningham

This will reject the file entirely. The testcase I have (can share once
chromium bug is fixed) was previously hitting an av_assert0 in
mov_read_trun, arguably also a total rejection ;). Recovery for this could
be pretty tricky since the dropped trun will break decode dependencies. I
would be surprised to find files with missing tfhd's in the wild (outside
of fuzzer / maliciously crafted sorts).

On Thu, Feb 7, 2019 at 7:58 AM Derek Buitenhuis 
wrote:

> On 07/02/2019 00:12, chcunningham wrote:
> > Detecting missing tfhd avoids re-using tfhd track info from the previous
> > moof. For files with multiple tracks, this may make a mess of the
> > avindex and fragindex, which can later trigger av_assert0 in
> > mov_read_trun().
> > ---
> >  libavformat/isom.h |  1 +
> >  libavformat/mov.c  | 10 ++
> >  2 files changed, 11 insertions(+)
>
> I think this seems reasonable.
>
> Is the intent to entirely reject these files, or only the broken parts?
> (lack of patch context for how it cascades for me)
>
> - Derek
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avformat/mov: validate chunk_count vs stsc_data

2019-02-07 Thread Chris Cunningham

Thanks! Looking at that file, I notice the stsc refers to some unknown
chunks (stsco has no chunk offsets), but this is a non-issue since stts has
no samples. Next patch will detect this condition and simply clear out stsc
structures since they're not needed and contradict stco.

On Wed, Feb 6, 2019 at 7:57 AM Michael Niedermayer 
wrote:

> On Mon, Feb 04, 2019 at 04:30:29PM -0800, chcunningham wrote:
> > Bad content may contain stsc boxes with a first_chunk index that
> > exceeds stco.entries (chunk_count). This ammends the existing check to
> > include cases where chunk_count == 0.
> > ---
> >  libavformat/mov.c | 7 +--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
>
> This seems to break GF9720Repeal20the20Eighth20with20Helen20Linehan.m4a
> Thats a sample from chromium issue 822666
>
> thx
>
>
> [...]
> --
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Many things microsoft did are stupid, but not doing something just because
> microsoft did it is even more stupid. If everything ms did were stupid they
> would be bankrupt already.
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avformat/oggparseogm: sync avctx w/ codecpar

2019-02-07 Thread Chris Cunningham

On Wed, Feb 6, 2019 at 6:03 PM James Almer  wrote:

> Can a valid ogm stream contain more than one header packet?

Looking at ogg_packet oggdec.c, we read headers until hitting the first
non-header (counted in ogg stream field nb_headers), so multiple headers
per stream is probably ok. But multiple codecs in a given stream seems
supsect to me. Someone with more ogg expertise should chime in.

Because the
> issue here as far as i can see is that ogm_header() is not validating
> the output of ff_codec_get_id() and is happily accepting and propagating
> AV_CODEC_ID_NONE as derived from it in a late packet, long after the
> parser and everything else was already initialized.
>
> No other ogg module sets st->internal->need_context_update and all of
> them may also end up calling the respective header reading function more
> than once on invalid files, but unlike the ogm one, all of them hardcode
> the codec_id instead of blindly accepting a potentially bogus value
> derived from the bitstream.
>

I'm open to hard-coding codec for gsm. One thing I notice is that the codec
is just one of several codecpar fields that are potentially changing.
If any of those change without need_context_update, it seems like values in
the internal avctx could become stale?
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avformat/oggparseogm: sync avctx w/ codecpar

2019-02-08 Thread Chris Cunningham

I'll re-send with my full name.

On Fri, Feb 8, 2019 at 4:57 AM Michael Niedermayer 
wrote:

> Hi
>
> On Wed, Feb 06, 2019 at 05:13:03PM -0800, chcunningham wrote:
> > From: chcunningham 
>
> Is the name intended ? As this ends up as author name in git, and several
> developers dislike this:
> (and i cannot edit other authors names of course, that would be not right)
>
> (from IRC)
>  Could you stop committing things like this?
>  his name is "Chris Cunningham", not "chcunningham"
>
> [...]
> --
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> The real ebay dictionary, page 1
> "Used only once"- "Some unspecified defect prevented a second use"
> "In good condition" - "Can be repaird by experienced expert"
> "As is" - "You wouldnt want it even if you were payed for it, if you knew
> ..."
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] avformat/oggparseogm: sync avctx w/ codecpar

2019-02-08 Thread Chris Cunningham

Codec information may change while reading ogg packets. Update the
stream's internal avctx to match.
---
 libavformat/oggparseogm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/libavformat/oggparseogm.c b/libavformat/oggparseogm.c
index a07453760b..b07a5d55ba 100644
--- a/libavformat/oggparseogm.c
+++ b/libavformat/oggparseogm.c
@@ -114,6 +114,9 @@ ogm_header(AVFormatContext *s, int idx)
 bytestream2_get_buffer(&p, st->codecpar->extradata, 
st->codecpar->extradata_size);
 }
 }
+
+// Update internal avctx with changes to codecpar above.
+st->internal->need_context_update = 1;
 } else if (bytestream2_peek_byte(&p) == 3) {
 bytestream2_skip(&p, 7);
 if (bytestream2_get_bytes_left(&p) > 1)
-- 
2.20.1.791.gb4d0f1c61a-goog

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

Re: [FFmpeg-devel] [PATCH] avformat/oggparseogm: sync avctx w/ codecpar

2019-02-11 Thread Chris Cunningham

On Fri, Feb 8, 2019 at 2:37 PM Michael Niedermayer  wrote:

> ogg allows chaining streams when they have differing serial numbers
> https://xiph.org/ogg/doc/oggstream.html
>
> i think ive seen actual files doing this
>
> ogg_replace_stream() might assign these into existing avstreams i think
>

If I'm reading this correctly, I think that should always happen at the
boundary of a new ogg page, meaning it wouldn't rely on this multiple
headers logic?

I found the commit where this was introduced
https://github.com/FFmpeg/FFmpeg/commit/81b743eb1026547270b88ac6a5cb451a3907ee94?diff=split

With the description:
This fixes some old ogm files that had the 3rd vorbis header after a data
packet in another stream. This is invalid in ogg, but this change shouldn't
affect the behaviour of any valid file.

So, I don't think we're going to find spec text for this. No spec for OGM
and committer indicates its not valid Ogg. I'm guessing we want to still
support these ogm files?
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avformat/oggparseogm: sync avctx w/ codecpar

2019-02-15 Thread Chris Cunningham

On Mon, Feb 11, 2019 at 1:55 PM Chris Cunningham 
wrote:

> On Fri, Feb 8, 2019 at 2:37 PM Michael Niedermayer 
> wrote:
>
>> ogg allows chaining streams when they have differing serial numbers
>> https://xiph.org/ogg/doc/oggstream.html
>>
>> i think ive seen actual files doing this
>>
>> ogg_replace_stream() might assign these into existing avstreams i think
>>
>
> If I'm reading this correctly, I think that should always happen at the
> boundary of a new ogg page, meaning it wouldn't rely on this multiple
> headers logic?
>
> I found the commit where this was introduced
>
> https://github.com/FFmpeg/FFmpeg/commit/81b743eb1026547270b88ac6a5cb451a3907ee94?diff=split
>
> With the description:
> This fixes some old ogm files that had the 3rd vorbis header after a data
> packet in another stream. This is invalid in ogg, but this change shouldn't
> affect the behaviour of any valid file.
>
> So, I don't think we're going to find spec text for this. No spec for OGM
> and committer indicates its not valid Ogg. I'm guessing we want to still
> support these ogm files?
>
>
Hey friends, just checking in on this discussion. Pls advise on findings
above.

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

Re: [FFmpeg-devel] [PATCH] avformat/oggparseogm: sync avctx w/ codecpar

2019-02-21 Thread Chris Cunningham

I'm fine to do either. James, do you still prefer to skip the later headers
if this breaks some old ogm files?

On Sat, Feb 16, 2019 at 4:52 PM Michael Niedermayer 
wrote:

> On Fri, Feb 15, 2019 at 02:56:18PM -0800, Chris Cunningham wrote:
> > On Mon, Feb 11, 2019 at 1:55 PM Chris Cunningham <
> chcunning...@chromium.org>
> > wrote:
> >
> > > On Fri, Feb 8, 2019 at 2:37 PM Michael Niedermayer 
> > > wrote:
> > >
> > >> ogg allows chaining streams when they have differing serial numbers
> > >> https://xiph.org/ogg/doc/oggstream.html
> > >>
> > >> i think ive seen actual files doing this
> > >>
> > >> ogg_replace_stream() might assign these into existing avstreams i
> think
> > >>
> > >
> > > If I'm reading this correctly, I think that should always happen at the
> > > boundary of a new ogg page, meaning it wouldn't rely on this multiple
> > > headers logic?
> > >
> > > I found the commit where this was introduced
> > >
> > >
> https://github.com/FFmpeg/FFmpeg/commit/81b743eb1026547270b88ac6a5cb451a3907ee94?diff=split
> > >
> > > With the description:
> > > This fixes some old ogm files that had the 3rd vorbis header after a
> data
> > > packet in another stream. This is invalid in ogg, but this change
> shouldn't
> > > affect the behaviour of any valid file.
> > >
> > > So, I don't think we're going to find spec text for this. No spec for
> OGM
> > > and committer indicates its not valid Ogg. I'm guessing we want to
> still
> > > support these ogm files?
> > >
> > >
> > Hey friends, just checking in on this discussion. Pls advise on findings
> > above.
>
> I have no real oppinion on which solution to pick. If disallowing these
> changes in OGM works, thats fine with me as is updating values
>
> thanks
>
> [...]
>
> --
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> The real ebay dictionary, page 2
> "100% positive feedback" - "All either got their money back or didnt
> complain"
> "Best seller ever, very honest" - "Seller refunded buyer after failed scam"
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avformat/oggparseogm: sync avctx w/ codecpar

2019-02-26 Thread Chris Cunningham

On Thu, Feb 21, 2019 at 4:46 PM Chris Cunningham 
wrote:

> I'm fine to do either. James, do you still prefer to skip the later
> headers if this breaks some old ogm files?
>

James, friendly ping
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] libavformat/matroskadec: fix unsigned overflow to improve seeking

2016-07-21 Thread Chris Cunningham

When seeking a file where codec delay is greater than 0, the timecode
can become negative after offsetting by the codec delay. Failing to cast
to a signed int64 will cause the check against skip_to_timecode to evaluate
true for these negative values. This breaks the "skip_to" seek mechanism.
---
 libavformat/matroskadec.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
index f3d701f..60b1b34 100644
--- a/libavformat/matroskadec.c
+++ b/libavformat/matroskadec.c
@@ -3150,7 +3150,10 @@ static int matroska_parse_block(MatroskaDemuxContext 
*matroska, uint8_t *data,
 
 if (matroska->skip_to_keyframe &&
 track->type != MATROSKA_TRACK_TYPE_SUBTITLE) {
-if (timecode < matroska->skip_to_timecode)
+// Compare signed timecodes. Timecode may be negative due to codec 
delay
+// offset. We don't support timestamps greater than int64_t anyway - 
see
+// AVPacket's pts.
+if ((int64_t)timecode < (int64_t)(matroska->skip_to_timecode))
 return res;
 if (is_keyframe)
 matroska->skip_to_keyframe = 0;
-- 
2.8.0.rc3.226.g39d4020

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

Re: [FFmpeg-devel] [PATCH] libavformat/matroskadec: fix unsigned overflow to improve seeking

2016-07-21 Thread Chris Cunningham

You can demonstrate the overflow using ffplay and this opus file:
https://storage.googleapis.com/chcunningham-chrome-shared/bear-opus.webm

Before patching, try:
ffplay -ss 2 bear-opus.webm

Notice that, in spite of the seek to 2 seconds, the file plays back from
time 0. After patching, re-run and find that the playback begins from the 2
second time.

On Thu, Jul 21, 2016 at 12:01 PM, Chris Cunningham <
chcunning...@chromium.org> wrote:

> When seeking a file where codec delay is greater than 0, the timecode
> can become negative after offsetting by the codec delay. Failing to cast
> to a signed int64 will cause the check against skip_to_timecode to evaluate
> true for these negative values. This breaks the "skip_to" seek mechanism.
> ---
>  libavformat/matroskadec.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> index f3d701f..60b1b34 100644
> --- a/libavformat/matroskadec.c
> +++ b/libavformat/matroskadec.c
> @@ -3150,7 +3150,10 @@ static int
> matroska_parse_block(MatroskaDemuxContext *matroska, uint8_t *data,
>
>  if (matroska->skip_to_keyframe &&
>  track->type != MATROSKA_TRACK_TYPE_SUBTITLE) {
> -if (timecode < matroska->skip_to_timecode)
> +// Compare signed timecodes. Timecode may be negative due to
> codec delay
> +// offset. We don't support timestamps greater than int64_t
> anyway - see
> +// AVPacket's pts.
> +if ((int64_t)timecode < (int64_t)(matroska->skip_to_timecode))
>  return res;
>  if (is_keyframe)
>  matroska->skip_to_keyframe = 0;
> --
> 2.8.0.rc3.226.g39d4020
>
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] libavformat/matroskadec: fix unsigned overflow to improve seeking

2016-07-26 Thread Chris Cunningham

Happy to make a test. Stand by for patch.

On Sat, Jul 23, 2016 at 2:26 AM, Michael Niedermayer  wrote:

> On Thu, Jul 21, 2016 at 12:01:45PM -0700, Chris Cunningham wrote:
> > When seeking a file where codec delay is greater than 0, the timecode
> > can become negative after offsetting by the codec delay. Failing to cast
> > to a signed int64 will cause the check against skip_to_timecode to
> evaluate
> > true for these negative values. This breaks the "skip_to" seek mechanism.
> > ---
> >  libavformat/matroskadec.c | 5 -
> >  1 file changed, 4 insertions(+), 1 deletion(-)
>
> applied
>
> can you create a fate testcase for this ?
> (maybe libavformat/tests/seek.c could be used)
>
> i can upload the sample file to the fate-samples
>
> Thanks
>
> [...]
> --
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> While the State exists there can be no freedom; when there is freedom there
> will be no State. -- Vladimir Lenin
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] libavformat/matroskadec: Add test for seeking with codec delay.

2016-07-27 Thread Chris Cunningham

The file to upload to fate-suite can be found here:
https://storage.googleapis.com/chcunningham-chrome-shared/codec_delay_opus.mkv

Alternatively, you can generate it via:
ffmpeg -i ../../tests/data/lavf/lavf.mkv -acodec opus -vn
codec_delay_opus.mkv

On Wed, Jul 27, 2016 at 6:33 PM,  wrote:

> From: Chris Cunningham 
>
> Also cleanup parens for the skip_to_timecode check.
> ---
>  libavformat/matroskadec.c  |  2 +-
>  tests/fate/seek.mak|  3 +++
>  tests/ref/seek/mkv-codec-delay | 48
> ++
>  3 files changed, 52 insertions(+), 1 deletion(-)
>  create mode 100644 tests/ref/seek/mkv-codec-delay
>
> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> index 60b1b34..d07a092 100644
> --- a/libavformat/matroskadec.c
> +++ b/libavformat/matroskadec.c
> @@ -3153,7 +3153,7 @@ static int matroska_parse_block(MatroskaDemuxContext
> *matroska, uint8_t *data,
>  // Compare signed timecodes. Timecode may be negative due to
> codec delay
>  // offset. We don't support timestamps greater than int64_t
> anyway - see
>  // AVPacket's pts.
> -if ((int64_t)timecode < (int64_t)(matroska->skip_to_timecode))
> +if ((int64_t)timecode < (int64_t)matroska->skip_to_timecode)
>  return res;
>  if (is_keyframe)
>  matroska->skip_to_keyframe = 0;
> diff --git a/tests/fate/seek.mak b/tests/fate/seek.mak
> index b831cf8..f835da5 100644
> --- a/tests/fate/seek.mak
> +++ b/tests/fate/seek.mak
> @@ -247,8 +247,11 @@ FATE_SEEK += $(FATE_SEEK_LAVF-yes:%=fate-seek-lavf-%)
>
>  FATE_SEEK_EXTRA-$(CONFIG_MP3_DEMUXER)   += fate-seek-extra-mp3
>  FATE_SEEK_EXTRA-$(call ALLYES, CACHE_PROTOCOL PIPE_PROTOCOL MP3_DEMUXER)
> += fate-seek-cache-pipe
> +FATE_SEEK_EXTRA-$(CONFIG_MATROSKA_DEMUXER) += fate-seek-mkv-codec-delay
>  fate-seek-extra-mp3:  CMD = run libavformat/tests/seek$(EXESUF)
> $(TARGET_SAMPLES)/gapless/gapless.mp3 -fastseek 1
>  fate-seek-cache-pipe: CMD = cat $(TARGET_SAMPLES)/gapless/gapless.mp3 |
> run libavformat/tests/seek$(EXESUF) cache:pipe:0 -read_ahead_limit -1
> +fate-seek-mkv-codec-delay:   CMD = run libavformat/tests/seek$(EXESUF)
> $(TARGET_SAMPLES)/mkv/codec_delay_opus.mkv
> +
>  FATE_SEEK_EXTRA += $(FATE_SEEK_EXTRA-yes)
>
>
> diff --git a/tests/ref/seek/mkv-codec-delay
> b/tests/ref/seek/mkv-codec-delay
> new file mode 100644
> index 000..9d4582c
> --- /dev/null
> +++ b/tests/ref/seek/mkv-codec-delay
> @@ -0,0 +1,48 @@
> +ret: 0 st: 0 flags:1 dts:-0.007000 pts:-0.007000 pos:748
> size:   320
> +ret: 0 st:-1 flags:0  ts:-1.00
> +ret: 0 st: 0 flags:1 dts:-0.007000 pts:-0.007000 pos:748
> size:   320
> +ret: 0 st:-1 flags:1  ts: 1.894167
> +ret: 0 st: 0 flags:1 dts: 1.014000 pts: 1.014000 pos:   9306
> size:   291
> +ret: 0 st: 0 flags:0  ts: 0.788000
> +ret: 0 st: 0 flags:1 dts: 0.794000 pts: 0.794000 pos:   7358
> size:   154
> +ret: 0 st: 0 flags:1  ts:-0.317000
> +ret: 0 st: 0 flags:1 dts:-0.007000 pts:-0.007000 pos:748
> size:   320
> +ret:-1 st:-1 flags:0  ts: 2.576668
> +ret: 0 st:-1 flags:1  ts: 1.470835
> +ret: 0 st: 0 flags:1 dts: 1.014000 pts: 1.014000 pos:   9306
> size:   291
> +ret: 0 st: 0 flags:0  ts: 0.365000
> +ret: 0 st: 0 flags:1 dts: 0.374000 pts: 0.374000 pos:   3963
> size:   150
> +ret: 0 st: 0 flags:1  ts:-0.741000
> +ret: 0 st: 0 flags:1 dts:-0.007000 pts:-0.007000 pos:748
> size:   320
> +ret:-1 st:-1 flags:0  ts: 2.153336
> +ret: 0 st:-1 flags:1  ts: 1.047503
> +ret: 0 st: 0 flags:1 dts: 1.014000 pts: 1.014000 pos:   9306
> size:   291
> +ret: 0 st: 0 flags:0  ts:-0.058000
> +ret: 0 st: 0 flags:1 dts:-0.007000 pts:-0.007000 pos:748
> size:   320
> +ret: 0 st: 0 flags:1  ts: 2.836000
> +ret: 0 st: 0 flags:1 dts: 1.014000 pts: 1.014000 pos:   9306
> size:   291
> +ret:-1 st:-1 flags:0  ts: 1.730004
> +ret: 0 st:-1 flags:1  ts: 0.624171
> +ret: 0 st: 0 flags:1 dts: 0.614000 pts: 0.614000 pos:   5903
> size:   159
> +ret: 0 st: 0 flags:0  ts:-0.482000
> +ret: 0 st: 0 flags:1 dts:-0.007000 pts:-0.007000 pos:748
> size:   320
> +ret: 0 st: 0 flags:1  ts: 2.413000
> +ret: 0 st: 0 flags:1 dts: 1.014000 pts: 1.014000 pos:   9306
> size:   291
> +ret:-1 st:-1 flags:0  ts: 1.306672
> +ret: 0 st:-1 flags:1  ts: 0.200839
> +ret: 0 st: 0 flags:1 dts: 0.194000 pts: 0.194000 pos:   2512
> size:   159
> +ret: 0 st: 0 flags:0  ts:-0.905000
> +re

Re: [FFmpeg-devel] [PATCH] libavformat/matroskadec: Add test for seeking with codec delay.

2016-07-28 Thread Chris Cunningham

Thanks Michael. Do you mean to also apply the patch to add the test? Maybe
you're waiting for further review.

On Thu, Jul 28, 2016 at 9:48 AM, Michael Niedermayer  wrote:

> On Wed, Jul 27, 2016 at 06:35:50PM -0700, Chris Cunningham wrote:
> > The file to upload to fate-suite can be found here:
> >
> https://storage.googleapis.com/chcunningham-chrome-shared/codec_delay_opus.mkv
>
> uploaded
>
> thx
>
> [...]
> --
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Those who are best at talking, realize last or never when they are wrong.
>
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] libavformat/matroskadec: Add test for seeking with codec delay.

2016-07-31 Thread Chris Cunningham

Thanks Michael. Do you mean to also apply the patch to add the test? Maybe
you're waiting for further review.

On Thu, Jul 28, 2016 at 9:48 AM, Michael Niedermayer  wrote:

> On Wed, Jul 27, 2016 at 06:35:50PM -0700, Chris Cunningham wrote:
> > The file to upload to fate-suite can be found here:
> >
> https://storage.googleapis.com/chcunningham-chrome-shared/codec_delay_opus.mkv
>
> uploaded
>
> thx
>
> [...]
> --
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Those who are best at talking, realize last or never when they are wrong.
>
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] size=0, but av_malloc(1)

2016-03-22 Thread Chris Cunningham

Hey Group,

I'm seeing an interesting pattern [0][1] where we allocate 1 byte in places
where I would expect no allocation to be necessary. Why is this being done?

[0] https://github.com/FFmpeg/FFmpeg/blob/master/libavutil/mem.c#L136
[1]
https://github.com/FFmpeg/FFmpeg/blob/master/libavformat/oggparsevorbis.c#L286

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

Re: [FFmpeg-devel] size=0, but av_malloc(1)

2016-03-24 Thread Chris Cunningham

>
> Well, what else would you do?



In both cases I was expecting to see null instead of the 1 byte alloc.

In the case of the metadata, I was missing the subtlety about "no update"
vs  "update to empty"... makes sense now.

malloc(0) can return NULL or non NULL whchever way libc prefers
> this makes reproducing bugreports harder if the developer and user
> have differening libcs


Makes sense, but then I wondered why not just skip malloc(0) and return
null. Your next sentence sheds some light.

also error checks become more complex if NULL can be a non error
> return value


Seems like the meat of the answer. I hadn't considered signalling error vs
empty in this way. IIUC this still isn't quite perfect since malloc itself
is not guaranteed to return null in the OOM error case... but then the ship
is probably going down anyway.



On Wed, Mar 23, 2016 at 1:56 PM, Reimar Döffinger 
wrote:

> On Wed, Mar 23, 2016 at 03:31:38PM +0100, Michael Niedermayer wrote:
> > On Tue, Mar 22, 2016 at 11:43:50PM -0700, Chris Cunningham wrote:
> > > Hey Group,
> > >
> > > I'm seeing an interesting pattern [0][1] where we allocate 1 byte in
> places
> > > where I would expect no allocation to be necessary. Why is this being
> done?
> > >
> > > [0] https://github.com/FFmpeg/FFmpeg/blob/master/libavutil/mem.c#L136
> > > [1]
> > >
> https://github.com/FFmpeg/FFmpeg/blob/master/libavformat/oggparsevorbis.c#L286
> >
> > to add another reason why malloc(0) _can_ be a problem
> > malloc(0) can return NULL or non NULL whchever way libc prefers
> > this makes reproducing bugreports harder if the developer and user
> > have differening libcs
> > also error checks become more complex if NULL can be a non error
> > return value
>
> Since you already said that: if that code used malloc(0) - note I
> don't know about av_malloc - and it returned 0, the behaviour would
> be incorrect.
> In this code, a NULL pointer means "no metadata update" which is very
> different from "metadata update to empty metadata".
> Also even if it does not return NULL it could always return the same
> pointer, which could trigger yet another class of bugs (probably
> not in this case though).
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] libavformat/oggdec: Free stream private when header parsing fails.

2016-05-09 Thread Chris Cunningham

Leaking this private structure opens up the possibility that it may
be re-used when parsing later packets in the stream. This is
problematic if the later packets are not the same codec type (e.g.
private allocated during Vorbis parsing, but later packets are Opus
and the private is assumed to be the oggopus_private type in
opus_header()).
---
 libavformat/oggdec.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libavformat/oggdec.c b/libavformat/oggdec.c
index 528d8db..47a0cba 100644
--- a/libavformat/oggdec.c
+++ b/libavformat/oggdec.c
@@ -701,6 +701,7 @@ static int ogg_read_header(AVFormatContext *s)
 if (ogg->streams[i].header < 0) {
 av_log(s, AV_LOG_ERROR, "Header parsing failed for stream %d\n", 
i);
 ogg->streams[i].codec = NULL;
+av_freep(&ogg->streams[i].private);
 } else if (os->codec && os->nb_header < os->codec->nb_header) {
 av_log(s, AV_LOG_WARNING,
"Headers mismatch for stream %d: "
-- 
2.8.0.rc3.226.g39d4020

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

Re: [FFmpeg-devel] [PATCH] avformat/utils: Check negative bps before shifting in ff_get_pcm_codec_id()

2016-05-17 Thread Chris Cunningham

New patch drops the U. Hendrik, you'r right about the negative, but you're
also right about the 0. 0 is a case of undefined shift behavior (shift
becomes -1).

On Tue, May 17, 2016 at 11:28 AM,  wrote:

> From: Chris Cunningham 
>
> Fixes: undefined shift.
> ---
>  libavformat/utils.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libavformat/utils.c b/libavformat/utils.c
> index 5f5f03e..d1e4306 100644
> --- a/libavformat/utils.c
> +++ b/libavformat/utils.c
> @@ -2955,7 +2955,7 @@ enum AVCodecID ff_codec_get_id(const AVCodecTag
> *tags, unsigned int tag)
>
>  enum AVCodecID ff_get_pcm_codec_id(int bps, int flt, int be, int sflags)
>  {
> -if (bps > 64U)
> +if (bps <= 0 || bps > 64)
>  return AV_CODEC_ID_NONE;
>
>  if (flt) {
> --
> 2.8.0.rc3.226.g39d4020
>
>
___
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

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-ke

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
> > <https://www.google.com/url?q=https%3A%2F%2Fgithub.com%
> 2FFFmpeg%2FFFmpeg%2Fblob%2Fmaster%2Flibavformat%
> 2Fmatroskaenc.c%23L2053&sa=D&sntz=1&usg=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

[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

[FFmpeg-devel] chromium-review spam - my mistake

2015-06-17 Thread Chris Cunningham

Hey Group,

Please ignore a wave of emails that were just sent out asking for code
review at https://chromium-review.googlesource.com/
.

Sorry for the spam!

I'm rolling upstream ffmpeg into chromium and our review tools have changed
recently. Our documented upstream-roll steps apparently need to be updated
to avoid sending out emails for every change in the merge :(

You may see a follow up mail as the reviews are deleted. Sorry for those
too.

Much shame,
Chris
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] avformat/mp3dec: prefer "fast_seek" to TOC seek for CBR files.

2015-11-04 Thread Chris Cunningham

"Fast seek" uses linear interpolation to find the position of the
requested seek time. For CBR this is more direct than using the
mp3 TOC and bypassing the TOC avoids problems when the TOC is
corrupted (e.g. https://crbug.com/545914).

For VBR, fast seek is not precise, so continue to prefer the TOC
when available.
---
 libavformat/mp3dec.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c
index 32ca00c..7946261 100644
--- a/libavformat/mp3dec.c
+++ b/libavformat/mp3dec.c
@@ -515,8 +515,10 @@ static int mp3_seek(AVFormatContext *s, int stream_index, 
int64_t timestamp,
 filesize = size - s->internal->data_offset;
 }
 
-if (   (mp3->is_cbr || fast_seek)
-&& (mp3->usetoc == 0 || !mp3->xing_toc)
+// Always use "fast seek" (linear interpolation) for CBR. For CBR this is
+// a precise method and more direct than using TOC. For VBR this is not
+// perfectly accurate, so prefer TOC if available.
+if ((mp3->is_cbr || (fast_seek && (mp3->usetoc == 0 || !mp3->xing_toc)))
 && st->duration > 0
 && filesize > 0) {
 ie = &ie1;
-- 
2.6.0.rc2.230.g3dd15c0

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

Re: [FFmpeg-devel] [PATCH] avformat/mp3dec: prefer "fast_seek" to TOC seek for CBR files.

2015-11-16 Thread Chris Cunningham

Hey, sorry for the slow reply.

Use this sample file. Its a CBR file with a corrupt TOC.
http://happyworm.com/jPlayerLab/halite/jumptest/testcount.mp3

Before applying my patch:

./ffplay -ss 00:33:20 testcount.mp3  -usetoc 0
plays out "1395", which is the correct audio for the given seek time

./ffplay -ss 00:33:20 testcount.mp3  -usetoc 1
plays out "...378, 1379", way off because it used the bad TOC

After applying my patch these commands both play out the correct time of
"1395" because usetoc=1 is ignored/overridden by virtue of this file being
CBR.

However, I've *just* thought of another (better) approach. My goal is
really to improve the behavior of AVFMT_FLAG_FAST_SEEK
<https://github.com/FFmpeg/FFmpeg/blob/b9cd3e1add3a3db86aaeba8680ad5a4ee57d519b/libavformat/mp3dec.c#L348>,
which defaults to using the TOC whenever its available. Standby for a
second patch that will only change TOC behavior when the
AVFMT_FLAG_FAST_SEEK is being used.

Chris

On Mon, Nov 9, 2015 at 3:34 PM, Michael Niedermayer 
wrote:

> On Wed, Nov 04, 2015 at 05:21:57PM -0800, Chris Cunningham wrote:
> > "Fast seek" uses linear interpolation to find the position of the
> > requested seek time. For CBR this is more direct than using the
> > mp3 TOC and bypassing the TOC avoids problems when the TOC is
> > corrupted (e.g. https://crbug.com/545914).
> >
> > For VBR, fast seek is not precise, so continue to prefer the TOC
> > when available.
> > ---
> >  libavformat/mp3dec.c | 6 --
> >  1 file changed, 4 insertions(+), 2 deletions(-)
>
> how can this be reproduced with FFmpeg ?
> do you have a sample which you can share ?
>
> [...]
> --
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> I have often repented speaking, but never of holding my tongue.
> -- Xenocrates
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avformat/mp3dec: prefer "fast_seek" to TOC seek for CBR files.

2015-11-16 Thread Chris Cunningham

To test this new patch, again use testcount.mp3
<http://happyworm.com/jPlayerLab/halite/jumptest/testcount.mp3> (CBR,
corrupt TOC).

Current ffmpeg (with none of my mp3 patches)

./ffplay testcount.mp3 -ss 00:33:20 -usetoc 0

plays out "1395", which is correct

./ffplay testcount.mp3 -ss 00:33:20 -usetoc 1
plays out "..378, 1389", which is incorrect due to the corrupted TOC.

./ffplay testcount.mp3 -ss 00:33:20 -fflags fastseek
also plays out "..378, 1389" because current fast_seek logic uses the TOC
whenever available for CBR and VBR alike.

After applying my *latest* patch:

./ffplay testcount.mp3 -ss 00:33:20 -usetoc 0
is unchanged: plays out "1395", which is correct

./ffplay testcount.mp3 -ss 00:33:20 -usetoc 1
is unchanged: still plays out "378, 1389". This is wrong, but it allows
users to force using TOC should they prefer.

./ffplay testcount.mp3 -ss 00:33:20 -fflags fastseek
*is changed: *plays out "1395", which is correct. fast seek no longer uses
the TOC for CBR files.

What do you think? I personally prefer this way so that usetoc is still
meaningful for CBR files.

Thanks,
Chris

On Mon, Nov 16, 2015 at 4:58 PM,  wrote:

> From: Chris Cunningham 
>
> "Fast seek" uses linear interpolation to find the position of the
> requested seek time. For CBR this is more direct than using the
> mp3 TOC and bypassing the TOC avoids problems when the TOC is
> corrupted (e.g. https://crbug.com/545914).
>
> For VBR, fast seek is not precise, so continue to prefer the TOC
> when available.
> ---
>  libavformat/mp3dec.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c
> index 32ca00c..e12266c 100644
> --- a/libavformat/mp3dec.c
> +++ b/libavformat/mp3dec.c
> @@ -515,8 +515,10 @@ static int mp3_seek(AVFormatContext *s, int
> stream_index, int64_t timestamp,
>  filesize = size - s->internal->data_offset;
>  }
>
> -if (   (mp3->is_cbr || fast_seek)
> -&& (mp3->usetoc == 0 || !mp3->xing_toc)
> +// When fast seeking, prefer to use the TOC when available for VBR
> files
> +// since av_rescale may not be accurate for VBR. For CBR, rescaling is
> +// always accurate and more direct than a TOC lookup.
> +if (fast_seek && (mp3->is_cbr || !mp3->xing_toc)
>  && st->duration > 0
>  && filesize > 0) {
>  ie = &ie1;
> --
> 2.6.0.rc2.230.g3dd15c0
>
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avformat/mp3dec: prefer "fast_seek" to TOC seek for CBR files.

2015-11-23 Thread Chris Cunningham

Adding everyone back on the cc list for a convo with wm4 (meant for
everyone). Expect a new patch shortly to go with this discussion.

Also, just a note for posterity and to make sure I haven't missed anything:
It seems the mp3 TOC is really not a precise tool for seeking, particularly
in large files. I break down what I know about the design in this bug:
https://code.google.com/p/chromium/issues/detail?id=545914#c13 - LMK if I'm
missing something.

On Wed, Nov 18, 2015 at 3:08 AM, wm4  wrote:

> On Tue, 17 Nov 2015 14:21:08 -0800
> Chris Cunningham  wrote:
>
> > On Tue, Nov 17, 2015 at 2:38 AM, wm4  wrote:
> >
> > > On Mon, 16 Nov 2015 16:58:57 -0800
> > > chcunning...@chromium.org wrote:
> > >
> > > > From: Chris Cunningham 
> > > >
> > > > "Fast seek" uses linear interpolation to find the position of the
> > > > requested seek time. For CBR this is more direct than using the
> > > > mp3 TOC and bypassing the TOC avoids problems when the TOC is
> > > > corrupted (e.g. https://crbug.com/545914).
> > > >
> > > > For VBR, fast seek is not precise, so continue to prefer the TOC
> > > > when available.
> > > > ---
> > > >  libavformat/mp3dec.c | 6 --
> > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c
> > > > index 32ca00c..e12266c 100644
> > > > --- a/libavformat/mp3dec.c
> > > > +++ b/libavformat/mp3dec.c
> > > > @@ -515,8 +515,10 @@ static int mp3_seek(AVFormatContext *s, int
> > > stream_index, int64_t timestamp,
> > > >  filesize = size - s->internal->data_offset;
> > > >  }
> > > >
> > > > -if (   (mp3->is_cbr || fast_seek)
> > > > -&& (mp3->usetoc == 0 || !mp3->xing_toc)
> > > > +// When fast seeking, prefer to use the TOC when available for
> VBR
> > > files
> > > > +// since av_rescale may not be accurate for VBR. For CBR,
> rescaling
> > > is
> > > > +// always accurate and more direct than a TOC lookup.
> > > > +if (fast_seek && (mp3->is_cbr || !mp3->xing_toc)
> > > >  && st->duration > 0
> > > >  && filesize > 0) {
> > > >  ie = &ie1;
> > >
> > > Hm, it's still a bit weird IMHO. Here's a suggestion: if usetoc is
> > > unset (its value is -1), then always set it 0. Other uses of the usetoc
> > > or fast seek flag could be simplified in the code.
> > >
> > > I think this sounds good. Then the behavior becomes:
> > -1 - unset implies default to 0
> > 0 - generic indexing code
> > 1 - toc
> > 2 - goes away, this is now the behavior of 0
> >
> >
> >
> > > Ultimately, the fast seek flag just sets the default mode. Disabling
> > > the flag makes it always use the generic indexing code. Enabling it
> > > used to enable TOC seeking, but we really always want CBR seeking. We
> > > want TOC seeking only if the user explicitly enables it by setting
> > > usetoc. Is that what we want?
> > >
> > >
> > *"We want TOC seeking only if the user explicitly enables it by setting*
> > *usetoc. Is that what we want?"*
> >
> > I think this is the heart of the complexity.
> > Is it useful for users to be able to say things like "fastseek, but never
> > use toc (always use scaling, even for VBR)"?
> > Or alternatively, "fastseek, but always use toc when available (even for
> > CBR)"?
> >
> > I'm not sure. We can support those combinations, but its more complex and
> > harder to reason about.
> >
> > It may be simpler to simply say that fastseek *overrides* usetoc. It
> could
> > cause toc to be used for VBR, and scaling for CBR, with no regard for
> user
> > set values of usetoc (we might detect a user setting and warn that we're
> > ignoring it). I'm happy to take this approach if everyone is on board.
> What
> > do you think? (Also, not sure if you mean to reply just to me, but we'd
> > have to ask Michael too).
>
> No, this was accidental. My mail client selected the wrong reply
> address because the mail had multiple recipients.


I'll merge the threads.


>
> I think there are two things: sane defaults, which apply if usetoc is
> not used, and forcing the demuxer to use a specific method if the user
&

Re: [FFmpeg-devel] [PATCH] avformat/mp3dec: prefer "fast_seek" to TOC seek for CBR files.

2015-11-24 Thread Chris Cunningham

The new patch incorporates the "3 use-cases" feedback from wm4.

1. default: use generic (accurate but slow) seeking
2. fast: set -fflags fastseek to get fast accurate scaled seek for CBR, TOC
seek for VBR falling back to scaling if no TOC exists
3. custom: set -usetoc 1 to always use TOC for VBR & CBR. Will override
fastseek flag. This is very inaccurate for large files.

fastseek is really very close to generic seek for CBR files. for VBR its
pretty rough.

The fate-seek-extra-mp3 test is still failing. Its easy to fix, but I need
some guidance on what the test is supposed to focus on. It was originally
going through the av_rescale path of mp3_seek. With my patch it goes
through the return -1 (slow generic index) path. IIUC, the generic index
path is covered elsewhere (
https://github.com/FFmpeg/FFmpeg/blob/a62178be80dd6a643973f62002fc0ed42495c358/tests/fate-run.sh#L229).
I think I can restore the behavior of this test to use the av_rescale path
if I replace -usetoc 0 with -fflags fastseek (and update args parsing)...
is that what we want?

Thanks,
Chris

On Tue, Nov 24, 2015 at 4:55 PM,  wrote:

> From: Chris Cunningham 
>
> "Fast seek" uses linear interpolation to find the position of the
> requested seek time. For CBR this is more direct than using the
> mp3 TOC and bypassing the TOC avoids problems with TOC precision.
> (see https://crbug.com/545914#c13)
>
> For VBR, fast seek is not precise, so continue to prefer the TOC
> when available (the lesser of two evils).
>
> Also, some re-ordering of the logic in mp3_seek to simplify and
> give usetoc=1 precedence over fastseek flag.
> ---
>  libavformat/mp3dec.c | 45 +++--
>  1 file changed, 23 insertions(+), 22 deletions(-)
>
> diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c
> index 32ca00c..a1f21b7 100644
> --- a/libavformat/mp3dec.c
> +++ b/libavformat/mp3dec.c
> @@ -115,7 +115,8 @@ static void read_xing_toc(AVFormatContext *s, int64_t
> filesize, int64_t duration
>  {
>  int i;
>  MP3DecContext *mp3 = s->priv_data;
> -int fill_index = mp3->usetoc == 1 && duration > 0;
> +int fast_seek = (s->flags & AVFMT_FLAG_FAST_SEEK) ? 1 : 0;
> +int fill_index = (mp3->usetoc || fast_seek) && duration > 0;
>
>  if (!filesize &&
>  !(filesize = avio_size(s->pb))) {
> @@ -344,9 +345,6 @@ static int mp3_read_header(AVFormatContext *s)
>  int ret;
>  int i;
>
> -if (mp3->usetoc < 0)
> -mp3->usetoc = (s->flags & AVFMT_FLAG_FAST_SEEK) ? 1 : 2;
> -
>  st = avformat_new_stream(s, NULL);
>  if (!st)
>  return AVERROR(ENOMEM);
> @@ -501,40 +499,43 @@ static int mp3_seek(AVFormatContext *s, int
> stream_index, int64_t timestamp,
>  MP3DecContext *mp3 = s->priv_data;
>  AVIndexEntry *ie, ie1;
>  AVStream *st = s->streams[0];
> -int64_t ret  = av_index_search_timestamp(st, timestamp, flags);
> -int64_t best_pos;
>  int fast_seek = (s->flags & AVFMT_FLAG_FAST_SEEK) ? 1 : 0;
>  int64_t filesize = mp3->header_filesize;
>
> -if (mp3->usetoc == 2)
> -return -1; // generic index code
> -
>  if (filesize <= 0) {
>  int64_t size = avio_size(s->pb);
>  if (size > 0 && size > s->internal->data_offset)
>  filesize = size - s->internal->data_offset;
>  }
>
> -if (   (mp3->is_cbr || fast_seek)
> -&& (mp3->usetoc == 0 || !mp3->xing_toc)
> -&& st->duration > 0
> -&& filesize > 0) {
> -ie = &ie1;
> -timestamp = av_clip64(timestamp, 0, st->duration);
> -ie->timestamp = timestamp;
> -ie->pos   = av_rescale(timestamp, filesize, st->duration) +
> s->internal->data_offset;
> -} else if (mp3->xing_toc) {
> +if (mp3->xing_toc && (mp3->usetoc || (fast_seek && !mp3->is_cbr))) {
> +av_log(s, AV_LOG_ERROR, "XING seeking. filesize = %"PRId64"\n",
> filesize);
> +// NOTE: The MP3 TOC is not a precise lookup table. The precision
> is
> +// worse closer to the end of the file, especially for large
> files.
> +// Seeking to 90% of duration in file of size 4M will be off by
> roughly 1 second.
> +if (filesize > 400)
> +av_log(s, AV_LOG_WARNING, "Using MP3 TOC to seek; may be
> imprecise.\n");
> +
> +int64_t ret  = av_index_search_timestamp(st, timestamp, flags);
>  if (ret < 0)
>  return ret;
>
>  ie = &st->index_entries[ret];
>

Re: [FFmpeg-devel] [PATCH] avformat/mp3dec: prefer "fast_seek" to TOC seek for CBR files.

2015-11-30 Thread Chris Cunningham

(Fixed CCs)

Thanks for review. Patch coming shortly.

On Thu, Nov 26, 2015 at 10:13 AM, wm4  wrote:

> On Tue, 24 Nov 2015 16:55:03 -0800
> chcunning...@chromium.org wrote:
>
> > From: Chris Cunningham 
> >
> > "Fast seek" uses linear interpolation to find the position of the
> > requested seek time. For CBR this is more direct than using the
> > mp3 TOC and bypassing the TOC avoids problems with TOC precision.
> > (see https://crbug.com/545914#c13)
> >
> > For VBR, fast seek is not precise, so continue to prefer the TOC
> > when available (the lesser of two evils).
> >
> > Also, some re-ordering of the logic in mp3_seek to simplify and
> > give usetoc=1 precedence over fastseek flag.
> > ---
> >  libavformat/mp3dec.c | 45 +++--
> >  1 file changed, 23 insertions(+), 22 deletions(-)
> >
> > diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c
> > index 32ca00c..a1f21b7 100644
> > --- a/libavformat/mp3dec.c
> > +++ b/libavformat/mp3dec.c
> > @@ -115,7 +115,8 @@ static void read_xing_toc(AVFormatContext *s,
> int64_t filesize, int64_t duration
> >  {
> >  int i;
> >  MP3DecContext *mp3 = s->priv_data;
> > -int fill_index = mp3->usetoc == 1 && duration > 0;
> > +int fast_seek = (s->flags & AVFMT_FLAG_FAST_SEEK) ? 1 : 0;
>
> Nit: the ?1:0 is not really needed.
>

Will fix.

>
> > +int fill_index = (mp3->usetoc || fast_seek) && duration > 0;
> >
> >  if (!filesize &&
> >  !(filesize = avio_size(s->pb))) {
> > @@ -344,9 +345,6 @@ static int mp3_read_header(AVFormatContext *s)
> >  int ret;
> >  int i;
> >
> > -if (mp3->usetoc < 0)
> > -mp3->usetoc = (s->flags & AVFMT_FLAG_FAST_SEEK) ? 1 : 2;
> > -
> >  st = avformat_new_stream(s, NULL);
> >  if (!st)
> >  return AVERROR(ENOMEM);
> > @@ -501,40 +499,43 @@ static int mp3_seek(AVFormatContext *s, int
> stream_index, int64_t timestamp,
> >  MP3DecContext *mp3 = s->priv_data;
> >  AVIndexEntry *ie, ie1;
> >  AVStream *st = s->streams[0];
> > -int64_t ret  = av_index_search_timestamp(st, timestamp, flags);
> > -int64_t best_pos;
> >  int fast_seek = (s->flags & AVFMT_FLAG_FAST_SEEK) ? 1 : 0;
> >  int64_t filesize = mp3->header_filesize;
> >
> > -if (mp3->usetoc == 2)
> > -return -1; // generic index code
> > -
> >  if (filesize <= 0) {
> >  int64_t size = avio_size(s->pb);
> >  if (size > 0 && size > s->internal->data_offset)
> >  filesize = size - s->internal->data_offset;
> >  }
> >
> > -if (   (mp3->is_cbr || fast_seek)
> > -&& (mp3->usetoc == 0 || !mp3->xing_toc)
> > -&& st->duration > 0
> > -&& filesize > 0) {
> > -ie = &ie1;
> > -timestamp = av_clip64(timestamp, 0, st->duration);
> > -ie->timestamp = timestamp;
> > -ie->pos   = av_rescale(timestamp, filesize, st->duration) +
> s->internal->data_offset;
> > -} else if (mp3->xing_toc) {
> > +if (mp3->xing_toc && (mp3->usetoc || (fast_seek && !mp3->is_cbr))) {
> > +av_log(s, AV_LOG_ERROR, "XING seeking. filesize = %"PRId64"\n",
> filesize);
> > +// NOTE: The MP3 TOC is not a precise lookup table. The
> precision is
> > +// worse closer to the end of the file, especially for large
> files.
> > +// Seeking to 90% of duration in file of size 4M will be off by
> roughly 1 second.
> > +if (filesize > 400)
> > +av_log(s, AV_LOG_WARNING, "Using MP3 TOC to seek; may be
> imprecise.\n");
>
> This seems like a rather questionable heuristic. And only to print a
> message? Maybe it would be better to drop it, or always print it
> regardless of file size.
>
> I'll make this always print. Hopefully this helps people avoid TOC.


> > +
> > +int64_t ret  = av_index_search_timestamp(st, timestamp, flags);
> >  if (ret < 0)
> >  return ret;
> >
> >  ie = &st->index_entries[ret];
> > +} else if (fast_seek && st->duration > 0 && filesize > 0) {
> > +if (!mp3->is_cbr)
> > +av_log(s, AV_LOG_WARNING, "Using scaling

Re: [FFmpeg-devel] [PATCH] avformat/mp3dec: prefer "fast_seek" to TOC seek for CBR files.

2015-11-30 Thread Chris Cunningham

Looks like I messed up the tab for "return best_pos;"... fix incoming.

On Mon, Nov 30, 2015 at 2:10 PM,  wrote:

> From: Chris Cunningham 
>
> "Fast seek" uses linear interpolation to find the position of the
> requested seek time. For CBR this is more direct than using the
> mp3 TOC and bypassing the TOC avoids problems with TOC precision.
> (see https://crbug.com/545914#c13)
>
> For VBR, fast seek is not precise, so continue to prefer the TOC
> when available (the lesser of two evils).
>
> Also, some re-ordering of the logic in mp3_seek to simplify and
> give usetoc=1 precedence over fastseek flag.
> ---
>  libavformat/mp3dec.c| 44 +---
>  libavformat/seek-test.c |  8 +---
>  tests/fate/seek.mak |  2 +-
>  3 files changed, 27 insertions(+), 27 deletions(-)
>
> diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c
> index 32ca00c..3c8c0ee 100644
> --- a/libavformat/mp3dec.c
> +++ b/libavformat/mp3dec.c
> @@ -115,7 +115,8 @@ static void read_xing_toc(AVFormatContext *s, int64_t
> filesize, int64_t duration
>  {
>  int i;
>  MP3DecContext *mp3 = s->priv_data;
> -int fill_index = mp3->usetoc == 1 && duration > 0;
> +int fast_seek = s->flags & AVFMT_FLAG_FAST_SEEK;
> +int fill_index = (mp3->usetoc || fast_seek) && duration > 0;
>
>  if (!filesize &&
>  !(filesize = avio_size(s->pb))) {
> @@ -344,9 +345,6 @@ static int mp3_read_header(AVFormatContext *s)
>  int ret;
>  int i;
>
> -if (mp3->usetoc < 0)
> -mp3->usetoc = (s->flags & AVFMT_FLAG_FAST_SEEK) ? 1 : 2;
> -
>  st = avformat_new_stream(s, NULL);
>  if (!st)
>  return AVERROR(ENOMEM);
> @@ -501,40 +499,40 @@ static int mp3_seek(AVFormatContext *s, int
> stream_index, int64_t timestamp,
>  MP3DecContext *mp3 = s->priv_data;
>  AVIndexEntry *ie, ie1;
>  AVStream *st = s->streams[0];
> -int64_t ret  = av_index_search_timestamp(st, timestamp, flags);
> -int64_t best_pos;
> -int fast_seek = (s->flags & AVFMT_FLAG_FAST_SEEK) ? 1 : 0;
> +int fast_seek = s->flags & AVFMT_FLAG_FAST_SEEK;
>  int64_t filesize = mp3->header_filesize;
>
> -if (mp3->usetoc == 2)
> -return -1; // generic index code
> -
>  if (filesize <= 0) {
>  int64_t size = avio_size(s->pb);
>  if (size > 0 && size > s->internal->data_offset)
>  filesize = size - s->internal->data_offset;
>  }
>
> -if (   (mp3->is_cbr || fast_seek)
> -&& (mp3->usetoc == 0 || !mp3->xing_toc)
> -&& st->duration > 0
> -&& filesize > 0) {
> -ie = &ie1;
> -timestamp = av_clip64(timestamp, 0, st->duration);
> -ie->timestamp = timestamp;
> -ie->pos   = av_rescale(timestamp, filesize, st->duration) +
> s->internal->data_offset;
> -} else if (mp3->xing_toc) {
> +if (mp3->xing_toc && (mp3->usetoc || (fast_seek && !mp3->is_cbr))) {
> +// NOTE: The MP3 TOC is not a precise lookup table. Accuracy is
> worse
> +// for bigger files.
> +av_log(s, AV_LOG_WARNING, "Using MP3 TOC to seek; may be
> imprecise.\n");
> +
> +int64_t ret  = av_index_search_timestamp(st, timestamp, flags);
>  if (ret < 0)
>  return ret;
>
>  ie = &st->index_entries[ret];
> +} else if (fast_seek && st->duration > 0 && filesize > 0) {
> +if (!mp3->is_cbr)
> +av_log(s, AV_LOG_WARNING, "Using scaling to seek VBR MP3; may
> be imprecise.\n");
> +
> +ie = &ie1;
> +timestamp = av_clip64(timestamp, 0, st->duration);
> +ie->timestamp = timestamp;
> +ie->pos   = av_rescale(timestamp, filesize, st->duration) +
> s->internal->data_offset;
>  } else {
> -return -1;
> +return -1; // generic index code
>  }
>
> -best_pos = mp3_sync(s, ie->pos, flags);
> +int64_t best_pos = mp3_sync(s, ie->pos, flags);
>  if (best_pos < 0)
> -return best_pos;
> +  return best_pos;
>
>  if (mp3->is_cbr && ie == &ie1 && mp3->frames) {
>  int frame_duration = av_rescale(st->duration, 1, mp3->frames);
> @@ -546,7 +544,7 @@ static int mp3_seek(AVFormatContext *s, int
> stream_index, int64_t timestamp,
>  }
>
>  static const AVOption options[]

Re: [FFmpeg-devel] [PATCH] avformat/mp3dec: prefer "fast_seek" to TOC seek for CBR files.

2015-12-01 Thread Chris Cunningham

Thanks wm4, next patch will fix that declaration.

Michael, any concerns?

On Mon, Nov 30, 2015 at 2:51 PM, wm4  wrote:

> On Mon, 30 Nov 2015 14:29:50 -0800
> chcunning...@chromium.org wrote:
>
> > From: Chris Cunningham 
> >
> > "Fast seek" uses linear interpolation to find the position of the
> > requested seek time. For CBR this is more direct than using the
> > mp3 TOC and bypassing the TOC avoids problems with TOC precision.
> > (see https://crbug.com/545914#c13)
> >
> > For VBR, fast seek is not precise, so continue to prefer the TOC
> > when available (the lesser of two evils).
> >
> > Also, some re-ordering of the logic in mp3_seek to simplify and
> > give usetoc=1 precedence over fastseek flag.
> > ---
> >  libavformat/mp3dec.c| 42 --
> >  libavformat/seek-test.c |  8 +---
> >  tests/fate/seek.mak |  2 +-
> >  3 files changed, 26 insertions(+), 26 deletions(-)
> >
> > diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c
> > index 32ca00c..3dc2b5c 100644
> > --- a/libavformat/mp3dec.c
> > +++ b/libavformat/mp3dec.c
> > @@ -115,7 +115,8 @@ static void read_xing_toc(AVFormatContext *s,
> int64_t filesize, int64_t duration
> >  {
> >  int i;
> >  MP3DecContext *mp3 = s->priv_data;
> > -int fill_index = mp3->usetoc == 1 && duration > 0;
> > +int fast_seek = s->flags & AVFMT_FLAG_FAST_SEEK;
> > +int fill_index = (mp3->usetoc || fast_seek) && duration > 0;
> >
> >  if (!filesize &&
> >  !(filesize = avio_size(s->pb))) {
> > @@ -344,9 +345,6 @@ static int mp3_read_header(AVFormatContext *s)
> >  int ret;
> >  int i;
> >
> > -if (mp3->usetoc < 0)
> > -mp3->usetoc = (s->flags & AVFMT_FLAG_FAST_SEEK) ? 1 : 2;
> > -
> >  st = avformat_new_stream(s, NULL);
> >  if (!st)
> >  return AVERROR(ENOMEM);
> > @@ -501,38 +499,38 @@ static int mp3_seek(AVFormatContext *s, int
> stream_index, int64_t timestamp,
> >  MP3DecContext *mp3 = s->priv_data;
> >  AVIndexEntry *ie, ie1;
> >  AVStream *st = s->streams[0];
> > -int64_t ret  = av_index_search_timestamp(st, timestamp, flags);
> > -int64_t best_pos;
> > -int fast_seek = (s->flags & AVFMT_FLAG_FAST_SEEK) ? 1 : 0;
> > +int fast_seek = s->flags & AVFMT_FLAG_FAST_SEEK;
> >  int64_t filesize = mp3->header_filesize;
> >
> > -if (mp3->usetoc == 2)
> > -return -1; // generic index code
> > -
> >  if (filesize <= 0) {
> >  int64_t size = avio_size(s->pb);
> >  if (size > 0 && size > s->internal->data_offset)
> >  filesize = size - s->internal->data_offset;
> >  }
> >
> > -if (   (mp3->is_cbr || fast_seek)
> > -&& (mp3->usetoc == 0 || !mp3->xing_toc)
> > -&& st->duration > 0
> > -&& filesize > 0) {
> > -ie = &ie1;
> > -timestamp = av_clip64(timestamp, 0, st->duration);
> > -ie->timestamp = timestamp;
> > -ie->pos   = av_rescale(timestamp, filesize, st->duration) +
> s->internal->data_offset;
> > -} else if (mp3->xing_toc) {
> > +if (mp3->xing_toc && (mp3->usetoc || (fast_seek && !mp3->is_cbr))) {
> > +// NOTE: The MP3 TOC is not a precise lookup table. Accuracy is
> worse
> > +// for bigger files.
> > +av_log(s, AV_LOG_WARNING, "Using MP3 TOC to seek; may be
> imprecise.\n");
> > +
> > +int64_t ret  = av_index_search_timestamp(st, timestamp, flags);
> >  if (ret < 0)
> >  return ret;
> >
> >  ie = &st->index_entries[ret];
> > +} else if (fast_seek && st->duration > 0 && filesize > 0) {
> > +if (!mp3->is_cbr)
> > +av_log(s, AV_LOG_WARNING, "Using scaling to seek VBR MP3;
> may be imprecise.\n");
> > +
> > +ie = &ie1;
> > +timestamp = av_clip64(timestamp, 0, st->duration);
> > +ie->timestamp = timestamp;
> > +ie->pos   = av_rescale(timestamp, filesize, st->duration) +
> s->internal->data_offset;
> >  } else {
> > -return -1;
> > +return -1; // generic index code
> >  }
> >
> > -

61 matches

Mail list logo