Re: [FFmpeg-devel] [PATCH] matroskadec: execute seekheads recursively
On Oct 21, 2014, at 19:19, Carl Eugen Hoyos ceho...@ag.or.at wrote: Rodger Combs rodger.combs at gmail.com writes: This fixes https://trac.ffmpeg.org/ticket/3934 but I'm not sure if there was a good reason for this to be here to begin with. Perhaps a protection against infinite recursion (though I believe EBML_MAX_DEPTH serves that purpose to some degree)? Is all this related to ticket #2263? Carl Eugen ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel Yes. That sample (seekhead-at-end.mkv) seeks slowly because we don't execute seekheads recursively, and is fixed by this patch. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] matroskadec: execute seekheads recursively
On Oct 19, 2014, at 23:34, Rodger Combs rodger.co...@gmail.com wrote: On Oct 17, 2014, at 17:40, Michael Niedermayer michae...@gmx.at mailto:michae...@gmx.at wrote: On Fri, Oct 17, 2014 at 03:27:49PM +0200, Michael Niedermayer wrote: On Fri, Oct 17, 2014 at 01:55:35AM -0500, Rodger Combs wrote: On Oct 17, 2014, at 01:52, Rodger Combs rodger.co...@gmail.com mailto:rodger.co...@gmail.com wrote: On Oct 17, 2014, at 01:16, Rodger Combs rodger.co...@gmail.com mailto:rodger.co...@gmail.com mailto:rodger.co...@gmail.com mailto:rodger.co...@gmail.com wrote: This fixes https://trac.ffmpeg.org/ticket/3934 https://trac.ffmpeg.org/ticket/3934 https://trac.ffmpeg.org/ticket/3934 https://trac.ffmpeg.org/ticket/3934, but I'm not sure if there was a good reason for this to be here to begin with. Perhaps a protection against infinite recursion (though I believe EBML_MAX_DEPTH serves that purpose to some degree)? 0001-matroskadec-execute-seekheads-recursively.patch Evidently either I or my mail client screwed up and the patch didn't get attached. Whoops. 0001-matroskadec-execute-seekheads-recursively.patch Let's try that again... Welp, apparently my email client's borked badly in some way. Here's a gist link instead: https://gist.github.com/08f111e72b8b5ddba078 https://gist.github.com/08f111e72b8b5ddba078 copy and pasted so our archives dont depend on external links as well as for easy revieweing From 4cf14a9d117da69b64c267e6f982931cfa60a300 Mon Sep 17 00:00:00 2001 From: Rodger Combs rodger.co...@gmail.com mailto:rodger.co...@gmail.com Date: Fri, 17 Oct 2014 00:35:12 -0500 Subject: [PATCH] matroskadec: execute seekheads recursively Fixes https://trac.ffmpeg.org/ticket/3934 https://trac.ffmpeg.org/ticket/3934 --- libavformat/matroskadec.c | 1 - 1 file changed, 1 deletion(-) diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c index b742319..b437e74 100644 --- a/libavformat/matroskadec.c +++ b/libavformat/matroskadec.c @@ -1368,7 +1368,6 @@ static int matroska_parse_seekhead_entry(MatroskaDemuxContext *matroska, int ret = 0; if (idx = seekhead_list-nb_elem|| -seekhead[idx].id == MATROSKA_ID_SEEKHEAD || seekhead[idx].id == MATROSKA_ID_CLUSTER) return 0; ebml_parse() that gets called as a result of this change does not succeed and causes the one and only seekhead entry to return failure so i think this doesnt execute seekheads recursively [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB It is dangerous to be right in matters on which the established authorities are wrong. -- Voltaire ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org mailto:ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel http://ffmpeg.org/mailman/listinfo/ffmpeg-devel Just stepped through this; I also see ebml_parse() failing with the clip I originally uploaded, but not with the original source file, which I've uploaded to Dropbox: https://www.dropbox.com/s/32ve4hp567ukikt/Maken-Ki%21%20-%20S01E02.mkv?dl=0 https://www.dropbox.com/s/32ve4hp567ukikt/Maken-Ki%21%20-%20S01E02.mkv?dl=0 On IRC, the consensus seems to be: 1. This works as expected 2. We're not sure if it could lead to an infinite loop if a SeekHead pointed at itself, or at another SeekHead pointing to it; could someone with more experience with matroskadec.c confirm? 3. matroska_read_seek currently returns 0 and fails to seek if the file has no Cues; it should return -1 and let libavformat fallback on legacy behavior (as it does if it encounters an invalid SeekHead or Cues). This is fixed by this patch: From acd730174bbbc2f95be1e697ed4fb6bf33c30f05 Mon Sep 17 00:00:00 2001 From: Rodger Combs rodger.co...@gmail.com Date: Tue, 21 Oct 2014 01:33:08 -0500 Subject: [PATCH] matroskadec: fail matroska_read_seek if no index was parsed --- libavformat/matroskadec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c index b742319..a50e9ea 100644 --- a/libavformat/matroskadec.c +++ b/libavformat/matroskadec.c @@ -2917,7 +2917,7 @@ static int matroska_read_seek(AVFormatContext *s, int stream_index, matroska_parse_cues(matroska); } -if (!st-nb_index_entries) +if (!st-nb_index_entries || !matroska-index.nb_elem) goto err; timestamp = FFMAX(timestamp, st-index_entries[0].timestamp); -- 1.9.1 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] matroskadec: execute seekheads recursively
Rodger Combs rodger.combs at gmail.com writes: This fixes https://trac.ffmpeg.org/ticket/3934 but I'm not sure if there was a good reason for this to be here to begin with. Perhaps a protection against infinite recursion (though I believe EBML_MAX_DEPTH serves that purpose to some degree)? Is all this related to ticket #2263? Carl Eugen ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] matroskadec: execute seekheads recursively
On Tue, Oct 21, 2014 at 01:34:16AM -0500, Rodger Combs wrote: On Oct 19, 2014, at 23:34, Rodger Combs rodger.co...@gmail.com wrote: On Oct 17, 2014, at 17:40, Michael Niedermayer michae...@gmx.at mailto:michae...@gmx.at wrote: On Fri, Oct 17, 2014 at 03:27:49PM +0200, Michael Niedermayer wrote: On Fri, Oct 17, 2014 at 01:55:35AM -0500, Rodger Combs wrote: On Oct 17, 2014, at 01:52, Rodger Combs rodger.co...@gmail.com mailto:rodger.co...@gmail.com wrote: On Oct 17, 2014, at 01:16, Rodger Combs rodger.co...@gmail.com mailto:rodger.co...@gmail.com mailto:rodger.co...@gmail.com mailto:rodger.co...@gmail.com wrote: This fixes https://trac.ffmpeg.org/ticket/3934 https://trac.ffmpeg.org/ticket/3934 https://trac.ffmpeg.org/ticket/3934 https://trac.ffmpeg.org/ticket/3934, but I'm not sure if there was a good reason for this to be here to begin with. Perhaps a protection against infinite recursion (though I believe EBML_MAX_DEPTH serves that purpose to some degree)? 0001-matroskadec-execute-seekheads-recursively.patch Evidently either I or my mail client screwed up and the patch didn't get attached. Whoops. 0001-matroskadec-execute-seekheads-recursively.patch Let's try that again... Welp, apparently my email client's borked badly in some way. Here's a gist link instead: https://gist.github.com/08f111e72b8b5ddba078 https://gist.github.com/08f111e72b8b5ddba078 copy and pasted so our archives dont depend on external links as well as for easy revieweing From 4cf14a9d117da69b64c267e6f982931cfa60a300 Mon Sep 17 00:00:00 2001 From: Rodger Combs rodger.co...@gmail.com mailto:rodger.co...@gmail.com Date: Fri, 17 Oct 2014 00:35:12 -0500 Subject: [PATCH] matroskadec: execute seekheads recursively Fixes https://trac.ffmpeg.org/ticket/3934 https://trac.ffmpeg.org/ticket/3934 --- libavformat/matroskadec.c | 1 - 1 file changed, 1 deletion(-) diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c index b742319..b437e74 100644 --- a/libavformat/matroskadec.c +++ b/libavformat/matroskadec.c @@ -1368,7 +1368,6 @@ static int matroska_parse_seekhead_entry(MatroskaDemuxContext *matroska, int ret = 0; if (idx = seekhead_list-nb_elem|| -seekhead[idx].id == MATROSKA_ID_SEEKHEAD || seekhead[idx].id == MATROSKA_ID_CLUSTER) return 0; ebml_parse() that gets called as a result of this change does not succeed and causes the one and only seekhead entry to return failure so i think this doesnt execute seekheads recursively [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB It is dangerous to be right in matters on which the established authorities are wrong. -- Voltaire ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org mailto:ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel http://ffmpeg.org/mailman/listinfo/ffmpeg-devel Just stepped through this; I also see ebml_parse() failing with the clip I originally uploaded, but not with the original source file, which I've uploaded to Dropbox: https://www.dropbox.com/s/32ve4hp567ukikt/Maken-Ki%21%20-%20S01E02.mkv?dl=0 https://www.dropbox.com/s/32ve4hp567ukikt/Maken-Ki%21%20-%20S01E02.mkv?dl=0 On IRC, the consensus seems to be: 1. This works as expected 2. We're not sure if it could lead to an infinite loop if a SeekHead pointed at itself, or at another SeekHead pointing to it; could someone with more experience with matroskadec.c confirm? 3. matroska_read_seek currently returns 0 and fails to seek if the file has no Cues; it should return -1 and let libavformat fallback on legacy behavior (as it does if it encounters an invalid SeekHead or Cues). This is fixed by this patch: From acd730174bbbc2f95be1e697ed4fb6bf33c30f05 Mon Sep 17 00:00:00 2001 From: Rodger Combs rodger.co...@gmail.com Date: Tue, 21 Oct 2014 01:33:08 -0500 Subject: [PATCH] matroskadec: fail matroska_read_seek if no index was parsed --- libavformat/matroskadec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c index b742319..a50e9ea 100644 --- a/libavformat/matroskadec.c +++ b/libavformat/matroskadec.c @@ -2917,7 +2917,7 @@ static int matroska_read_seek(AVFormatContext *s, int stream_index, matroska_parse_cues(matroska); } -if (!st-nb_index_entries) +if (!st-nb_index_entries || !matroska-index.nb_elem) goto err; timestamp = FFMAX(timestamp, st-index_entries[0].timestamp); posted a different patch to fix this, please review/comment [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The greatest way to live with honor in this world is to be what we pretend to be. -- Socrates signature.asc
Re: [FFmpeg-devel] [PATCH] matroskadec: execute seekheads recursively
On Oct 17, 2014, at 17:40, Michael Niedermayer michae...@gmx.at wrote: On Fri, Oct 17, 2014 at 03:27:49PM +0200, Michael Niedermayer wrote: On Fri, Oct 17, 2014 at 01:55:35AM -0500, Rodger Combs wrote: On Oct 17, 2014, at 01:52, Rodger Combs rodger.co...@gmail.com wrote: On Oct 17, 2014, at 01:16, Rodger Combs rodger.co...@gmail.com mailto:rodger.co...@gmail.com wrote: This fixes https://trac.ffmpeg.org/ticket/3934 https://trac.ffmpeg.org/ticket/3934, but I'm not sure if there was a good reason for this to be here to begin with. Perhaps a protection against infinite recursion (though I believe EBML_MAX_DEPTH serves that purpose to some degree)? 0001-matroskadec-execute-seekheads-recursively.patch Evidently either I or my mail client screwed up and the patch didn't get attached. Whoops. 0001-matroskadec-execute-seekheads-recursively.patch Let's try that again... Welp, apparently my email client's borked badly in some way. Here's a gist link instead: https://gist.github.com/08f111e72b8b5ddba078 copy and pasted so our archives dont depend on external links as well as for easy revieweing From 4cf14a9d117da69b64c267e6f982931cfa60a300 Mon Sep 17 00:00:00 2001 From: Rodger Combs rodger.co...@gmail.com Date: Fri, 17 Oct 2014 00:35:12 -0500 Subject: [PATCH] matroskadec: execute seekheads recursively Fixes https://trac.ffmpeg.org/ticket/3934 --- libavformat/matroskadec.c | 1 - 1 file changed, 1 deletion(-) diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c index b742319..b437e74 100644 --- a/libavformat/matroskadec.c +++ b/libavformat/matroskadec.c @@ -1368,7 +1368,6 @@ static int matroska_parse_seekhead_entry(MatroskaDemuxContext *matroska, int ret = 0; if (idx = seekhead_list-nb_elem|| -seekhead[idx].id == MATROSKA_ID_SEEKHEAD || seekhead[idx].id == MATROSKA_ID_CLUSTER) return 0; ebml_parse() that gets called as a result of this change does not succeed and causes the one and only seekhead entry to return failure so i think this doesnt execute seekheads recursively [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB It is dangerous to be right in matters on which the established authorities are wrong. -- Voltaire ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org mailto:ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel http://ffmpeg.org/mailman/listinfo/ffmpeg-devel Just stepped through this; I also see ebml_parse() failing with the clip I originally uploaded, but not with the original source file, which I've uploaded to Dropbox: https://www.dropbox.com/s/32ve4hp567ukikt/Maken-Ki%21%20-%20S01E02.mkv?dl=0 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] matroskadec: execute seekheads recursively
On Oct 17, 2014, at 01:16, Rodger Combs rodger.co...@gmail.com wrote: This fixes https://trac.ffmpeg.org/ticket/3934 https://trac.ffmpeg.org/ticket/3934, but I'm not sure if there was a good reason for this to be here to begin with. Perhaps a protection against infinite recursion (though I believe EBML_MAX_DEPTH serves that purpose to some degree)? 0001-matroskadec-execute-seekheads-recursively.patch Evidently either I or my mail client screwed up and the patch didn't get attached. Whoops. Let's try that again... ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] matroskadec: execute seekheads recursively
On Oct 17, 2014, at 01:52, Rodger Combs rodger.co...@gmail.com wrote: On Oct 17, 2014, at 01:16, Rodger Combs rodger.co...@gmail.com mailto:rodger.co...@gmail.com wrote: This fixes https://trac.ffmpeg.org/ticket/3934 https://trac.ffmpeg.org/ticket/3934, but I'm not sure if there was a good reason for this to be here to begin with. Perhaps a protection against infinite recursion (though I believe EBML_MAX_DEPTH serves that purpose to some degree)? 0001-matroskadec-execute-seekheads-recursively.patch Evidently either I or my mail client screwed up and the patch didn't get attached. Whoops. 0001-matroskadec-execute-seekheads-recursively.patch Let's try that again... Welp, apparently my email client's borked badly in some way. Here's a gist link instead: https://gist.github.com/08f111e72b8b5ddba078 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] matroskadec: execute seekheads recursively
On Fri, 17 Oct 2014 01:55:35 -0500 Rodger Combs rodger.co...@gmail.com wrote: On Oct 17, 2014, at 01:52, Rodger Combs rodger.co...@gmail.com wrote: On Oct 17, 2014, at 01:16, Rodger Combs rodger.co...@gmail.com mailto:rodger.co...@gmail.com wrote: This fixes https://trac.ffmpeg.org/ticket/3934 https://trac.ffmpeg.org/ticket/3934, but I'm not sure if there was a good reason for this to be here to begin with. Perhaps a protection against infinite recursion (though I believe EBML_MAX_DEPTH serves that purpose to some degree)? 0001-matroskadec-execute-seekheads-recursively.patch Evidently either I or my mail client screwed up and the patch didn't get attached. Whoops. 0001-matroskadec-execute-seekheads-recursively.patch Let's try that again... Welp, apparently my email client's borked badly in some way. Here's a gist link instead: https://gist.github.com/08f111e72b8b5ddba078 Yes, seekheads can reference other seekheads. IMO it should perhaps keep a list of elements to read (and which have already been read), which includes the seakhead. Since it'd be a list, it'd be safe against recursion. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] matroskadec: execute seekheads recursively
On Fri, Oct 17, 2014 at 01:55:35AM -0500, Rodger Combs wrote: On Oct 17, 2014, at 01:52, Rodger Combs rodger.co...@gmail.com wrote: On Oct 17, 2014, at 01:16, Rodger Combs rodger.co...@gmail.com mailto:rodger.co...@gmail.com wrote: This fixes https://trac.ffmpeg.org/ticket/3934 https://trac.ffmpeg.org/ticket/3934, but I'm not sure if there was a good reason for this to be here to begin with. Perhaps a protection against infinite recursion (though I believe EBML_MAX_DEPTH serves that purpose to some degree)? 0001-matroskadec-execute-seekheads-recursively.patch Evidently either I or my mail client screwed up and the patch didn't get attached. Whoops. 0001-matroskadec-execute-seekheads-recursively.patch Let's try that again... Welp, apparently my email client's borked badly in some way. Here's a gist link instead: https://gist.github.com/08f111e72b8b5ddba078 copy and pasted so our archives dont depend on external links as well as for easy revieweing From 4cf14a9d117da69b64c267e6f982931cfa60a300 Mon Sep 17 00:00:00 2001 From: Rodger Combs rodger.co...@gmail.com Date: Fri, 17 Oct 2014 00:35:12 -0500 Subject: [PATCH] matroskadec: execute seekheads recursively Fixes https://trac.ffmpeg.org/ticket/3934 --- libavformat/matroskadec.c | 1 - 1 file changed, 1 deletion(-) diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c index b742319..b437e74 100644 --- a/libavformat/matroskadec.c +++ b/libavformat/matroskadec.c @@ -1368,7 +1368,6 @@ static int matroska_parse_seekhead_entry(MatroskaDemuxContext *matroska, int ret = 0; if (idx = seekhead_list-nb_elem|| -seekhead[idx].id == MATROSKA_ID_SEEKHEAD || seekhead[idx].id == MATROSKA_ID_CLUSTER) return 0; -- 1.9.1 [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Breaking DRM is a little like attempting to break through a door even though the window is wide open and the only thing in the house is a bunch of things you dont want and which you would get tomorrow for free anyway signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] matroskadec: execute seekheads recursively
On Fri, Oct 17, 2014 at 03:27:49PM +0200, Michael Niedermayer wrote: On Fri, Oct 17, 2014 at 01:55:35AM -0500, Rodger Combs wrote: On Oct 17, 2014, at 01:52, Rodger Combs rodger.co...@gmail.com wrote: On Oct 17, 2014, at 01:16, Rodger Combs rodger.co...@gmail.com mailto:rodger.co...@gmail.com wrote: This fixes https://trac.ffmpeg.org/ticket/3934 https://trac.ffmpeg.org/ticket/3934, but I'm not sure if there was a good reason for this to be here to begin with. Perhaps a protection against infinite recursion (though I believe EBML_MAX_DEPTH serves that purpose to some degree)? 0001-matroskadec-execute-seekheads-recursively.patch Evidently either I or my mail client screwed up and the patch didn't get attached. Whoops. 0001-matroskadec-execute-seekheads-recursively.patch Let's try that again... Welp, apparently my email client's borked badly in some way. Here's a gist link instead: https://gist.github.com/08f111e72b8b5ddba078 copy and pasted so our archives dont depend on external links as well as for easy revieweing From 4cf14a9d117da69b64c267e6f982931cfa60a300 Mon Sep 17 00:00:00 2001 From: Rodger Combs rodger.co...@gmail.com Date: Fri, 17 Oct 2014 00:35:12 -0500 Subject: [PATCH] matroskadec: execute seekheads recursively Fixes https://trac.ffmpeg.org/ticket/3934 --- libavformat/matroskadec.c | 1 - 1 file changed, 1 deletion(-) diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c index b742319..b437e74 100644 --- a/libavformat/matroskadec.c +++ b/libavformat/matroskadec.c @@ -1368,7 +1368,6 @@ static int matroska_parse_seekhead_entry(MatroskaDemuxContext *matroska, int ret = 0; if (idx = seekhead_list-nb_elem|| -seekhead[idx].id == MATROSKA_ID_SEEKHEAD || seekhead[idx].id == MATROSKA_ID_CLUSTER) return 0; ebml_parse() that gets called as a result of this change does not succeed and causes the one and only seekhead entry to return failure so i think this doesnt execute seekheads recursively [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB It is dangerous to be right in matters on which the established authorities are wrong. -- Voltaire signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
