Re: [FFmpeg-devel] [ogg] Respect AVERROR codes returned by ogg header parsing.
On Fri, Dec 01, 2017 at 11:16:37AM -0800, Dale Curtis wrote: > On Thu, Nov 30, 2017 at 5:49 PM, Michael Niedermayer > wrote: > > > I dont see anything really wrong with the file > > > > For kicks, I tried running it through oggz-validate, but it doesn't know > how to handle ogm. Which TIL, is distinct from ogv. > > > > > > it seems what happens is that theres a data packet in one stream that > > preceeds the headers on the other streams, technically that data packet > > likely contains the video headers so its kind of a header too. > > > > The demuxer stops header parsing prematurly due to this. > > from there on it gets confused reading the same header twice > > while it determines the duration of the file which triggers the error > > > > There are a few differnt ways to fix this, iam not sure which is the > > simplest but i dont think the demuxer should fail in this case > > > > > Applying a workaround similar to what you did here works: > http://git.videolan.org/?p=ffmpeg.git;a=commitdiff;h=c04c43b3e423d0426162828e7b180e4d0014a3f7 > > I.e. condition AVERROR_INVALIDATA based on priv->vp being incomplete here: > http://git.videolan.org/?p=ffmpeg.git;a=blob;f=libavformat/oggparsevorbis.c;h=65b1998a02d92d0fcaeb740d8f4523641502dbea;hb=HEAD#l319 > > WDYT? Here's a patch to do this and fail on AVERROR w/o the AV_EF_EXPLODE > restriction. will apply thanks [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB "You are 36 times more likely to die in a bathtub than at the hands of a terrorist. Also, you are 2.5 times more likely to become a president and 2 times more likely to become an astronaut, than to die in a terrorist attack." -- Thoughty2 signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [ogg] Respect AVERROR codes returned by ogg header parsing.
On Thu, Nov 30, 2017 at 5:49 PM, Michael Niedermayer wrote:
> I dont see anything really wrong with the file
>
For kicks, I tried running it through oggz-validate, but it doesn't know
how to handle ogm. Which TIL, is distinct from ogv.
>
> it seems what happens is that theres a data packet in one stream that
> preceeds the headers on the other streams, technically that data packet
> likely contains the video headers so its kind of a header too.
>
> The demuxer stops header parsing prematurly due to this.
> from there on it gets confused reading the same header twice
> while it determines the duration of the file which triggers the error
>
> There are a few differnt ways to fix this, iam not sure which is the
> simplest but i dont think the demuxer should fail in this case
>
>
Applying a workaround similar to what you did here works:
http://git.videolan.org/?p=ffmpeg.git;a=commitdiff;h=c04c43b3e423d0426162828e7b180e4d0014a3f7
I.e. condition AVERROR_INVALIDATA based on priv->vp being incomplete here:
http://git.videolan.org/?p=ffmpeg.git;a=blob;f=libavformat/oggparsevorbis.c;h=65b1998a02d92d0fcaeb740d8f4523641502dbea;hb=HEAD#l319
WDYT? Here's a patch to do this and fail on AVERROR w/o the AV_EF_EXPLODE
restriction.
- dale
From a6f5e47b54a08ac0ab2461b5e5189748059c7a9f Mon Sep 17 00:00:00 2001
From: Dale Curtis
Date: Tue, 28 Nov 2017 13:40:20 -0800
Subject: [PATCH] Respect AVERROR codes returned by ogg parsers.
Fixes ticket #6804. All of the ogg header and packet parsers may
return standard AVERROR codes; these return values should not be
treated as success.
Additionally changes oggparsevorbis, to not give up too early
with certain types of poorly muxed files.
Signed-off-by: Dale Curtis
---
libavformat/oggdec.c | 14 +++---
libavformat/oggparsevorbis.c | 2 +-
2 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/libavformat/oggdec.c b/libavformat/oggdec.c
index 193a286e43..38f60653f9 100644
--- a/libavformat/oggdec.c
+++ b/libavformat/oggdec.c
@@ -543,7 +543,11 @@ static int ogg_packet(AVFormatContext *s, int *sid, int *dstart, int *dsize,
os->incomplete = 0;
if (os->header) {
-os->header = os->codec->header(s, idx);
+if ((ret = os->codec->header(s, idx)) < 0) {
+av_log(s, AV_LOG_ERROR, "Header processing failed: %s\n", av_err2str(ret));
+return ret;
+}
+os->header = ret;
if (!os->header) {
os->segp = segp;
os->psize = psize;
@@ -574,8 +578,12 @@ static int ogg_packet(AVFormatContext *s, int *sid, int *dstart, int *dsize,
} else {
os->pflags= 0;
os->pduration = 0;
-if (os->codec && os->codec->packet)
-os->codec->packet(s, idx);
+if (os->codec && os->codec->packet) {
+if ((ret = os->codec->packet(s, idx)) < 0) {
+av_log(s, AV_LOG_ERROR, "Packet processing failed: %s\n", av_err2str(ret));
+return ret;
+}
+}
if (sid)
*sid = idx;
if (dstart)
diff --git a/libavformat/oggparsevorbis.c b/libavformat/oggparsevorbis.c
index 65b1998a02..29b1ab514e 100644
--- a/libavformat/oggparsevorbis.c
+++ b/libavformat/oggparsevorbis.c
@@ -317,7 +317,7 @@ static int vorbis_header(AVFormatContext *s, int idx)
if (priv->packet[pkt_type >> 1])
return AVERROR_INVALIDDATA;
if (pkt_type > 1 && !priv->packet[0] || pkt_type > 3 && !priv->packet[1])
-return AVERROR_INVALIDDATA;
+return priv->vp ? 0 : AVERROR_INVALIDDATA;
priv->len[pkt_type >> 1]= os->psize;
priv->packet[pkt_type >> 1] = av_mallocz(os->psize);
--
2.15.0.531.g2ccb3012c9-goog
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [ogg] Respect AVERROR codes returned by ogg header parsing.
On Thu, Nov 30, 2017 at 12:19:10PM -0800, Dale Curtis wrote: > On Wed, Nov 29, 2017 at 5:58 PM, Michael Niedermayer > wrote: > > > > > AV_EF_EXPLODE appears to be a possibility, yes > > in general demuxers should try to extract as much as possible from a > > file and not give up if some broken packet is found > > > > I don't think this is a good idea; some of these errors seem fairly fatal > to the overall process. I think instead it's better to write off the sample > you have as being invalid. That said, I defer to your judgement here, so > I've produced both versions of the patch and leave it to your decision. > Chrome is now always using AV_EF_EXPLODE, so either works for our usage. Ive taken a deeper look as this doesnt sound right I dont see anything really wrong with the file it seems what happens is that theres a data packet in one stream that preceeds the headers on the other streams, technically that data packet likely contains the video headers so its kind of a header too. The demuxer stops header parsing prematurly due to this. from there on it gets confused reading the same header twice while it determines the duration of the file which triggers the error There are a few differnt ways to fix this, iam not sure which is the simplest but i dont think the demuxer should fail in this case [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB "Nothing to hide" only works if the folks in power share the values of you and everyone you know entirely and always will -- Tom Scott signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [ogg] Respect AVERROR codes returned by ogg header parsing.
On Wed, Nov 29, 2017 at 5:58 PM, Michael Niedermayer wrote:
>
> AV_EF_EXPLODE appears to be a possibility, yes
> in general demuxers should try to extract as much as possible from a
> file and not give up if some broken packet is found
>
I don't think this is a good idea; some of these errors seem fairly fatal
to the overall process. I think instead it's better to write off the sample
you have as being invalid. That said, I defer to your judgement here, so
I've produced both versions of the patch and leave it to your decision.
Chrome is now always using AV_EF_EXPLODE, so either works for our usage.
- dale
From e47bd6161b4e062c669dad1989d2289fd1e3a8dc Mon Sep 17 00:00:00 2001
From: Dale Curtis
Date: Tue, 28 Nov 2017 13:40:20 -0800
Subject: [PATCH] Respect AVERROR codes returned by ogg parsers.
Fixes ticket #6804. All of the ogg header and packet parsers may
return standard AVERROR codes; these return values should not be
treated as success. Despite returning some fairly serious error
codes, this version of the patch puts these failures behind an
error_recognition flag of AV_EF_EXPLODE.
Signed-off-by: Dale Curtis
---
libavformat/oggdec.c | 14 +++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/libavformat/oggdec.c b/libavformat/oggdec.c
index 193a286e43..03053bef7a 100644
--- a/libavformat/oggdec.c
+++ b/libavformat/oggdec.c
@@ -543,7 +543,11 @@ static int ogg_packet(AVFormatContext *s, int *sid, int *dstart, int *dsize,
os->incomplete = 0;
if (os->header) {
-os->header = os->codec->header(s, idx);
+if ((ret = os->codec->header(s, idx)) < 0 && (s->error_recognition & AV_EF_EXPLODE)) {
+av_log(s, AV_LOG_ERROR, "Header processing failed: %d\n", ret);
+return ret;
+}
+os->header = ret;
if (!os->header) {
os->segp = segp;
os->psize = psize;
@@ -574,8 +578,12 @@ static int ogg_packet(AVFormatContext *s, int *sid, int *dstart, int *dsize,
} else {
os->pflags= 0;
os->pduration = 0;
-if (os->codec && os->codec->packet)
-os->codec->packet(s, idx);
+if (os->codec && os->codec->packet) {
+if ((ret = os->codec->packet(s, idx)) < 0 && (s->error_recognition & AV_EF_EXPLODE)) {
+av_log(s, AV_LOG_ERROR, "Packet processing failed: %d\n", ret);
+return ret;
+}
+}
if (sid)
*sid = idx;
if (dstart)
--
2.15.0.531.g2ccb3012c9-goog
From 80975de4c72f81d65ad9d3fa0c45ea2903a52434 Mon Sep 17 00:00:00 2001
From: Dale Curtis
Date: Tue, 28 Nov 2017 13:40:20 -0800
Subject: [PATCH] Respect AVERROR codes returned by ogg parsers.
Fixes ticket #6804. All of the ogg header and packet parsers may
return standard AVERROR codes; these return values should not be
treated as success.
Signed-off-by: Dale Curtis
---
libavformat/oggdec.c | 14 +++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/libavformat/oggdec.c b/libavformat/oggdec.c
index 193a286e43..a55743db0d 100644
--- a/libavformat/oggdec.c
+++ b/libavformat/oggdec.c
@@ -543,7 +543,11 @@ static int ogg_packet(AVFormatContext *s, int *sid, int *dstart, int *dsize,
os->incomplete = 0;
if (os->header) {
-os->header = os->codec->header(s, idx);
+if ((ret = os->codec->header(s, idx)) < 0) {
+av_log(s, AV_LOG_ERROR, "Header processing failed: %d\n", ret);
+return ret;
+}
+os->header = ret;
if (!os->header) {
os->segp = segp;
os->psize = psize;
@@ -574,8 +578,12 @@ static int ogg_packet(AVFormatContext *s, int *sid, int *dstart, int *dsize,
} else {
os->pflags= 0;
os->pduration = 0;
-if (os->codec && os->codec->packet)
-os->codec->packet(s, idx);
+if (os->codec && os->codec->packet) {
+if ((ret = os->codec->packet(s, idx)) < 0) {
+av_log(s, AV_LOG_ERROR, "Packet processing failed: %d\n", ret);
+return ret;
+}
+}
if (sid)
*sid = idx;
if (dstart)
--
2.15.0.531.g2ccb3012c9-goog
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [ogg] Respect AVERROR codes returned by ogg header parsing.
On Wed, Nov 29, 2017 at 11:57:17AM -0800, Dale Curtis wrote: > On Tue, Nov 28, 2017 at 4:32 PM, Michael Niedermayer > wrote: > > > > > > This breaks converting of this: > > ./ffmpeg -i bgc.sub.dub.ogm -vf subtitles=bgc.sub.dub.ogm -an file.avi > > > > https://samples.ffmpeg.org/ogg/bgc.sub.dub.ogm > > > > > That's due to the vorbis parser returning AVERROR_INVALIDDATA here, with > pkt_type == 5 and priv->packet[0] == priv_packet[1] == null. > http://git.videolan.org/?p=ffmpeg.git;a=blob;f=libavformat/oggparsevorbis.c;h=65b1998a02d92d0fcaeb740d8f4523641502dbea;hb=HEAD#l319 > > > Which were added in 2010 here: > http://git.videolan.org/?p=ffmpeg.git;a=commitdiff;h=73c44cb2869bfdbea829942eb35efa6d4c4e2f91 > > It seems that file has invalid vorbis streams if the check is to be > believed. What do you want to do here? Restrict this error behind > AV_EF_EXPLODE? AV_EF_EXPLODE appears to be a possibility, yes in general demuxers should try to extract as much as possible from a file and not give up if some broken packet is found [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Old school: Use the lowest level language in which you can solve the problem conveniently. New school: Use the highest level language in which the latest supercomputer can solve the problem without the user falling asleep waiting. signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [ogg] Respect AVERROR codes returned by ogg header parsing.
On Tue, Nov 28, 2017 at 4:32 PM, Michael Niedermayer wrote: > > > This breaks converting of this: > ./ffmpeg -i bgc.sub.dub.ogm -vf subtitles=bgc.sub.dub.ogm -an file.avi > > https://samples.ffmpeg.org/ogg/bgc.sub.dub.ogm > > That's due to the vorbis parser returning AVERROR_INVALIDDATA here, with pkt_type == 5 and priv->packet[0] == priv_packet[1] == null. http://git.videolan.org/?p=ffmpeg.git;a=blob;f=libavformat/oggparsevorbis.c;h=65b1998a02d92d0fcaeb740d8f4523641502dbea;hb=HEAD#l319 Which were added in 2010 here: http://git.videolan.org/?p=ffmpeg.git;a=commitdiff;h=73c44cb2869bfdbea829942eb35efa6d4c4e2f91 It seems that file has invalid vorbis streams if the check is to be believed. What do you want to do here? Restrict this error behind AV_EF_EXPLODE? - dale ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [ogg] Respect AVERROR codes returned by ogg header parsing.
On Tue, Nov 28, 2017 at 02:17:21PM -0800, Dale Curtis wrote: > Fixes bad parens in the above patch. > > - dale > > On Tue, Nov 28, 2017 at 2:03 PM, Dale Curtis > wrote: > > > Actually packet() was broken too, updated the patch to fix this case too. > > > > - dale > > > > On Tue, Nov 28, 2017 at 1:41 PM, Dale Curtis > > wrote: > > > >> Fixes ticket #6804. All of the ogg header parsers may return > >> standard AVERROR codes; these return values should not be > >> treated as success. > >> > >> Signed-off-by: Dale Curtis > >> > >> > >> > > > oggdec.c | 10 +++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > 07e15d08be7fcce839206cdd699eea804b1520ff fix_ogg_v3.patch > From 8d3b21ab59d835430f057cfb40a63318b25e7014 Mon Sep 17 00:00:00 2001 > From: Dale Curtis > Date: Tue, 28 Nov 2017 13:40:20 -0800 > Subject: [PATCH] Respect AVERROR codes returned by ogg parsers. > > Fixes ticket #6804. All of the ogg header and packet parsers may > return standard AVERROR codes; these return values should not be > treated as success. > > Signed-off-by: Dale Curtis > --- > libavformat/oggdec.c | 10 +++--- > 1 file changed, 7 insertions(+), 3 deletions(-) This breaks converting of this: ./ffmpeg -i bgc.sub.dub.ogm -vf subtitles=bgc.sub.dub.ogm -an file.avi https://samples.ffmpeg.org/ogg/bgc.sub.dub.ogm [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB If you think the mosad wants you dead since a long time then you are either wrong or dead since a long time. signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [ogg] Respect AVERROR codes returned by ogg header parsing.
Fixes bad parens in the above patch.
- dale
On Tue, Nov 28, 2017 at 2:03 PM, Dale Curtis
wrote:
> Actually packet() was broken too, updated the patch to fix this case too.
>
> - dale
>
> On Tue, Nov 28, 2017 at 1:41 PM, Dale Curtis
> wrote:
>
>> Fixes ticket #6804. All of the ogg header parsers may return
>> standard AVERROR codes; these return values should not be
>> treated as success.
>>
>> Signed-off-by: Dale Curtis
>>
>>
>>
>
From 8d3b21ab59d835430f057cfb40a63318b25e7014 Mon Sep 17 00:00:00 2001
From: Dale Curtis
Date: Tue, 28 Nov 2017 13:40:20 -0800
Subject: [PATCH] Respect AVERROR codes returned by ogg parsers.
Fixes ticket #6804. All of the ogg header and packet parsers may
return standard AVERROR codes; these return values should not be
treated as success.
Signed-off-by: Dale Curtis
---
libavformat/oggdec.c | 10 +++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/libavformat/oggdec.c b/libavformat/oggdec.c
index 193a286e43..281eba44cf 100644
--- a/libavformat/oggdec.c
+++ b/libavformat/oggdec.c
@@ -543,7 +543,9 @@ static int ogg_packet(AVFormatContext *s, int *sid, int *dstart, int *dsize,
os->incomplete = 0;
if (os->header) {
-os->header = os->codec->header(s, idx);
+if ((ret = os->codec->header(s, idx)) < 0)
+return ret;
+os->header = ret;
if (!os->header) {
os->segp = segp;
os->psize = psize;
@@ -574,8 +576,10 @@ static int ogg_packet(AVFormatContext *s, int *sid, int *dstart, int *dsize,
} else {
os->pflags= 0;
os->pduration = 0;
-if (os->codec && os->codec->packet)
-os->codec->packet(s, idx);
+if (os->codec && os->codec->packet) {
+if ((ret = os->codec->packet(s, idx)) < 0)
+return ret;
+}
if (sid)
*sid = idx;
if (dstart)
--
2.15.0.417.g466bffb3ac-goog
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [ogg] Respect AVERROR codes returned by ogg header parsing.
Actually packet() was broken too, updated the patch to fix this case too.
- dale
On Tue, Nov 28, 2017 at 1:41 PM, Dale Curtis
wrote:
> Fixes ticket #6804. All of the ogg header parsers may return
> standard AVERROR codes; these return values should not be
> treated as success.
>
> Signed-off-by: Dale Curtis
>
>
>
From 986e24ed45c5eb222eb87f2aa6ca703a371a267a Mon Sep 17 00:00:00 2001
From: Dale Curtis
Date: Tue, 28 Nov 2017 13:40:20 -0800
Subject: [PATCH] Respect AVERROR codes returned by ogg parsers.
Fixes ticket #6804. All of the ogg header and packet parsers may
return standard AVERROR codes; these return values should not be
treated as success.
Signed-off-by: Dale Curtis
---
libavformat/oggdec.c | 10 +++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/libavformat/oggdec.c b/libavformat/oggdec.c
index 193a286e43..b1f318bfb2 100644
--- a/libavformat/oggdec.c
+++ b/libavformat/oggdec.c
@@ -543,7 +543,9 @@ static int ogg_packet(AVFormatContext *s, int *sid, int *dstart, int *dsize,
os->incomplete = 0;
if (os->header) {
-os->header = os->codec->header(s, idx);
+if (ret = os->codec->header(s, idx) < 0)
+return ret;
+os->header = ret;
if (!os->header) {
os->segp = segp;
os->psize = psize;
@@ -574,8 +576,10 @@ static int ogg_packet(AVFormatContext *s, int *sid, int *dstart, int *dsize,
} else {
os->pflags= 0;
os->pduration = 0;
-if (os->codec && os->codec->packet)
-os->codec->packet(s, idx);
+if (os->codec && os->codec->packet) {
+if (ret = os->codec->packet(s, idx) < 0)
+return ret;
+}
if (sid)
*sid = idx;
if (dstart)
--
2.15.0.417.g466bffb3ac-goog
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
