diff mbox

[FFmpeg-devel,ogg] Respect AVERROR codes returned by ogg header parsing.

Message ID CAPUDrweaLdrWqbKAiSS1cnZx7QevsrHVdHm6beYT+0MShiq6tg@mail.gmail.com
State New
Headers show

Commit Message

Dale Curtis Dec. 1, 2017, 7:16 p.m. UTC 
On Thu, Nov 30, 2017 at 5:49 PM, Michael Niedermayer <michael@niedermayer.cc
> 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

Comments

Michael Niedermayer Dec. 2, 2017, 6:06 p.m. UTC | #1 
On Fri, Dec 01, 2017 at 11:16:37AM -0800, Dale Curtis wrote:
> On Thu, Nov 30, 2017 at 5:49 PM, Michael Niedermayer <michael@niedermayer.cc
> > 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

[...]
diff mbox

Patch

From a6f5e47b54a08ac0ab2461b5e5189748059c7a9f Mon Sep 17 00:00:00 2001
From: Dale Curtis <dalecurtis@chromium.org>
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 <dalecurtis@chromium.org>
---
 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