Apr 28, 2020, 16:07 by d...@lynne.ee:
> Apr 28, 2020, 15:59 by mattias.wad...@gmail.com:
>
>>
>>
> Well, I consider CRC checking a part of correctly parsing ogg, so I think its
> best to leave it on
> all the time.
>
Did a new version of the patches.
The first two are identical, save for some minor style tweaks.
The last one now ensures the max page size, 65k, is available for seeking and
speeds up
decoding on new/replacement streams since the offset was incorrect.
I think its worth it.
I'm also close to figuring out how to make chained Opus work, will send a patch
once I do.
>From 3f567301cc771f6192f3991b40805503ad996ff7 Mon Sep 17 00:00:00 2001
From: Lynne <d...@lynne.ee>
Date: Tue, 28 Apr 2020 12:41:34 +0100
Subject: [PATCH 1/3] oggdec: eliminate copies and extra buffers
This also makes implementing CRC checking far simpler and more robust.
---
libavformat/oggdec.c | 127 ++++++++++++++++++++-----------------------
1 file changed, 58 insertions(+), 69 deletions(-)
diff --git a/libavformat/oggdec.c b/libavformat/oggdec.c
index 95190589ab..7db26840b2 100644
--- a/libavformat/oggdec.c
+++ b/libavformat/oggdec.c
@@ -205,7 +205,7 @@ static const struct ogg_codec *ogg_find_codec(uint8_t *buf, int size)
* situation where a new audio stream spawn (identified with a new serial) and
* must replace the previous one (track switch).
*/
-static int ogg_replace_stream(AVFormatContext *s, uint32_t serial, int nsegs)
+static int ogg_replace_stream(AVFormatContext *s, uint32_t serial, int size)
{
struct ogg *ogg = s->priv_data;
struct ogg_stream *os;
@@ -214,11 +214,10 @@ static int ogg_replace_stream(AVFormatContext *s, uint32_t serial, int nsegs)
if (s->pb->seekable & AVIO_SEEKABLE_NORMAL) {
uint8_t magic[8];
- int64_t pos = avio_tell(s->pb);
- avio_skip(s->pb, nsegs);
+ avio_seek(s->pb, -size, SEEK_CUR);
if (avio_read(s->pb, magic, sizeof(magic)) != sizeof(magic))
return AVERROR_INVALIDDATA;
- avio_seek(s->pb, pos, SEEK_SET);
+ avio_seek(s->pb, size - sizeof(magic), SEEK_CUR);
codec = ogg_find_codec(magic, sizeof(magic));
if (!codec) {
av_log(s, AV_LOG_ERROR, "Cannot identify new stream\n");
@@ -303,27 +302,6 @@ static int ogg_new_stream(AVFormatContext *s, uint32_t serial)
return idx;
}
-static int ogg_new_buf(struct ogg *ogg, int idx)
-{
- struct ogg_stream *os = ogg->streams + idx;
- uint8_t *nb = av_malloc(os->bufsize + AV_INPUT_BUFFER_PADDING_SIZE);
- int size = os->bufpos - os->pstart;
-
- if (!nb)
- return AVERROR(ENOMEM);
-
- if (os->buf) {
- memcpy(nb, os->buf + os->pstart, size);
- av_free(os->buf);
- }
-
- os->buf = nb;
- os->bufpos = size;
- os->pstart = 0;
-
- return 0;
-}
-
static int data_packets_seen(const struct ogg *ogg)
{
int i;
@@ -343,8 +321,11 @@ static int ogg_read_page(AVFormatContext *s, int *sid)
int flags, nsegs;
uint64_t gp;
uint32_t serial;
- int size, idx;
+ int size = 0, idx;
+ int64_t page_pos;
uint8_t sync[4];
+ uint8_t segments[255];
+ uint8_t *readout_buf;
int sp = 0;
ret = avio_read(bc, sync, 4);
@@ -387,47 +368,73 @@ static int ogg_read_page(AVFormatContext *s, int *sid)
gp = avio_rl64(bc);
serial = avio_rl32(bc);
avio_skip(bc, 8); /* seq, crc */
- nsegs = avio_r8(bc);
+
+ nsegs = avio_r8(bc);
+ page_pos = avio_tell(bc) - 27;
+
+ ret = avio_read(bc, segments, nsegs);
+ if (ret < nsegs)
+ return ret < 0 ? ret : AVERROR_EOF;
if (avio_feof(bc))
return AVERROR_EOF;
+ for (i = 0; i < nsegs; i++)
+ size += segments[i];
+
idx = ogg_find_stream(ogg, serial);
+ if (idx >= 0) {
+ os = ogg->streams + idx;
+
+ /* Even if invalid guarantee there's enough memory to read the page */
+ if (os->bufsize - os->bufpos < size) {
+ uint8_t *nb = av_realloc(os->buf, 2*os->bufsize + AV_INPUT_BUFFER_PADDING_SIZE);
+ if (!nb)
+ return AVERROR(ENOMEM);
+ os->buf = nb;
+ os->bufsize *= 2;
+ }
+
+ readout_buf = os->buf + os->bufpos;
+ } else {
+ readout_buf = av_malloc(size);
+ }
+
+ ret = avio_read(bc, readout_buf, size);
+ if (ret < size) {
+ if (idx < 0)
+ av_free(readout_buf);
+ return ret < 0 ? ret : AVERROR_EOF;
+ }
+
if (idx < 0) {
if (data_packets_seen(ogg))
- idx = ogg_replace_stream(s, serial, nsegs);
+ idx = ogg_replace_stream(s, serial, size);
else
idx = ogg_new_stream(s, serial);
if (idx < 0) {
av_log(s, AV_LOG_ERROR, "failed to create or replace stream\n");
+ av_free(readout_buf);
return idx;
}
- }
- os = ogg->streams + idx;
- ogg->page_pos =
- os->page_pos = avio_tell(bc) - 27;
+ os = ogg->streams + idx;
- if (os->psize > 0) {
- ret = ogg_new_buf(ogg, idx);
- if (ret < 0)
- return ret;
+ memcpy(os->buf + os->bufpos, readout_buf, size);
+ av_free(readout_buf);
}
- ret = avio_read(bc, os->segments, nsegs);
- if (ret < nsegs)
- return ret < 0 ? ret : AVERROR_EOF;
-
- os->nsegs = nsegs;
- os->segp = 0;
-
- size = 0;
- for (i = 0; i < nsegs; i++)
- size += os->segments[i];
-
- if (!(flags & OGG_FLAG_BOS))
- os->got_data = 1;
+ ogg->page_pos = page_pos;
+ os->page_pos = page_pos;
+ os->nsegs = nsegs;
+ os->segp = 0;
+ os->got_data = !(flags & OGG_FLAG_BOS);
+ os->bufpos += size;
+ os->granule = gp;
+ os->flags = flags;
+ memcpy(os->segments, segments, nsegs);
+ memset(os->buf + os->bufpos, 0, AV_INPUT_BUFFER_PADDING_SIZE);
if (flags & OGG_FLAG_CONT || os->incomplete) {
if (!os->psize) {
@@ -447,26 +454,8 @@ static int ogg_read_page(AVFormatContext *s, int *sid)
os->sync_pos = os->page_pos;
}
- if (os->bufsize - os->bufpos < size) {
- uint8_t *nb = av_malloc((os->bufsize *= 2) + AV_INPUT_BUFFER_PADDING_SIZE);
- if (!nb)
- return AVERROR(ENOMEM);
- memcpy(nb, os->buf, os->bufpos);
- av_free(os->buf);
- os->buf = nb;
- }
-
- ret = avio_read(bc, os->buf + os->bufpos, size);
- if (ret < size)
- return ret < 0 ? ret : AVERROR_EOF;
-
- os->bufpos += size;
- os->granule = gp;
- os->flags = flags;
-
- memset(os->buf + os->bufpos, 0, AV_INPUT_BUFFER_PADDING_SIZE);
- if (sid)
- *sid = idx;
+ /* This function is always called with sid != NULL */
+ *sid = idx;
return 0;
}
--
2.26.2
>From 989d02631f2b47f125d3d8a3474572fe25ab1251 Mon Sep 17 00:00:00 2001
From: Lynne <d...@lynne.ee>
Date: Tue, 28 Apr 2020 12:52:11 +0100
Subject: [PATCH 2/3] oggdec: verify page checksum
This makes decoding far more robust, since OggS, the ogg magic,
can be commonly found randomly in streams, which previously made
the demuxer think there's a new stream or a change in such.
---
libavformat/oggdec.c | 46 ++++++++++++++++++++++++++++++++------------
1 file changed, 34 insertions(+), 12 deletions(-)
diff --git a/libavformat/oggdec.c b/libavformat/oggdec.c
index 7db26840b2..e0188c7c59 100644
--- a/libavformat/oggdec.c
+++ b/libavformat/oggdec.c
@@ -31,6 +31,7 @@
#include <stdio.h>
#include "libavutil/avassert.h"
#include "libavutil/intreadwrite.h"
+#include "avio_internal.h"
#include "oggdec.h"
#include "avformat.h"
#include "internal.h"
@@ -321,8 +322,9 @@ static int ogg_read_page(AVFormatContext *s, int *sid)
int flags, nsegs;
uint64_t gp;
uint32_t serial;
+ uint32_t crc, crc_tmp;
int size = 0, idx;
- int64_t page_pos;
+ int64_t version, page_pos;
uint8_t sync[4];
uint8_t segments[255];
uint8_t *readout_buf;
@@ -359,15 +361,19 @@ static int ogg_read_page(AVFormatContext *s, int *sid)
return AVERROR_INVALIDDATA;
}
- if (avio_r8(bc) != 0) { /* version */
- av_log (s, AV_LOG_ERROR, "ogg page, unsupported version\n");
- return AVERROR_INVALIDDATA;
- }
+ /* 0x4fa9b05f = av_crc(AV_CRC_32_IEEE, 0x0, "OggS", 4) */
+ ffio_init_checksum(bc, ff_crc04C11DB7_update, 0x4fa9b05f);
- flags = avio_r8(bc);
- gp = avio_rl64(bc);
- serial = avio_rl32(bc);
- avio_skip(bc, 8); /* seq, crc */
+ version = avio_r8(bc);
+ flags = avio_r8(bc);
+ gp = avio_rl64(bc);
+ serial = avio_rl32(bc);
+ avio_skip(bc, 4); /* seq */
+
+ crc_tmp = ffio_get_checksum(bc);
+ crc = avio_rb32(bc);
+ crc_tmp = ff_crc04C11DB7_update(crc_tmp, (uint8_t[4]){0}, 4);
+ ffio_init_checksum(bc, ff_crc04C11DB7_update, crc_tmp);
nsegs = avio_r8(bc);
page_pos = avio_tell(bc) - 27;
@@ -376,9 +382,6 @@ static int ogg_read_page(AVFormatContext *s, int *sid)
if (ret < nsegs)
return ret < 0 ? ret : AVERROR_EOF;
- if (avio_feof(bc))
- return AVERROR_EOF;
-
for (i = 0; i < nsegs; i++)
size += segments[i];
@@ -407,6 +410,25 @@ static int ogg_read_page(AVFormatContext *s, int *sid)
return ret < 0 ? ret : AVERROR_EOF;
}
+ if (crc ^ ffio_get_checksum(bc)) {
+ av_log(s, AV_LOG_ERROR, "CRC mismatch!\n");
+ if (idx < 0)
+ av_free(readout_buf);
+ avio_seek(bc, -size, SEEK_CUR);
+ return 0;
+ }
+
+ /* Since we're almost sure its a valid packet, checking the version after
+ * the checksum lets the demuxer be more tolerant */
+ if (version) {
+ av_log(s, AV_LOG_ERROR, "Invalid Ogg vers!\n");
+ if (idx < 0)
+ av_free(readout_buf);
+ avio_seek(bc, -size, SEEK_CUR);
+ return 0;
+ }
+
+ /* CRC is correct so we can be 99% sure there's an actual change here */
if (idx < 0) {
if (data_packets_seen(ogg))
idx = ogg_replace_stream(s, serial, size);
--
2.26.2
>From c25d21f9bf46004bcae1f2b0f54f735ba8c8d180 Mon Sep 17 00:00:00 2001
From: Lynne <d...@lynne.ee>
Date: Tue, 28 Apr 2020 12:55:17 +0100
Subject: [PATCH 3/3] oggdec: use ffio_ensure_seekback() to seek back on
incorrect data
This cleans up the code and simplifies it.
It also speeds up parsing since the old pb position was incorrect.
---
libavformat/oggdec.c | 68 ++++++++++++++++++--------------------------
1 file changed, 27 insertions(+), 41 deletions(-)
diff --git a/libavformat/oggdec.c b/libavformat/oggdec.c
index e0188c7c59..92dcafe2ed 100644
--- a/libavformat/oggdec.c
+++ b/libavformat/oggdec.c
@@ -206,59 +206,40 @@ static const struct ogg_codec *ogg_find_codec(uint8_t *buf, int size)
* situation where a new audio stream spawn (identified with a new serial) and
* must replace the previous one (track switch).
*/
-static int ogg_replace_stream(AVFormatContext *s, uint32_t serial, int size)
+static int ogg_replace_stream(AVFormatContext *s, uint32_t serial, char *magic)
{
struct ogg *ogg = s->priv_data;
struct ogg_stream *os;
const struct ogg_codec *codec;
int i = 0;
- if (s->pb->seekable & AVIO_SEEKABLE_NORMAL) {
- uint8_t magic[8];
- avio_seek(s->pb, -size, SEEK_CUR);
- if (avio_read(s->pb, magic, sizeof(magic)) != sizeof(magic))
- return AVERROR_INVALIDDATA;
- avio_seek(s->pb, size - sizeof(magic), SEEK_CUR);
- codec = ogg_find_codec(magic, sizeof(magic));
- if (!codec) {
- av_log(s, AV_LOG_ERROR, "Cannot identify new stream\n");
- return AVERROR_INVALIDDATA;
- }
- for (i = 0; i < ogg->nstreams; i++) {
- if (ogg->streams[i].codec == codec)
- break;
- }
- if (i >= ogg->nstreams)
- return ogg_new_stream(s, serial);
- } else if (ogg->nstreams != 1) {
+ if (ogg->nstreams != 1) {
avpriv_report_missing_feature(s, "Changing stream parameters in multistream ogg");
return AVERROR_PATCHWELCOME;
}
- os = &ogg->streams[i];
-
- os->serial = serial;
- return i;
+ /* Check for codecs */
+ codec = ogg_find_codec(magic, 8);
+ if (!codec) {
+ av_log(s, AV_LOG_ERROR, "Cannot identify new stream\n");
+ return AVERROR_INVALIDDATA;
+ }
-#if 0
- buf = os->buf;
- bufsize = os->bufsize;
- codec = os->codec;
+ /* If the codec matches, then we assume its a replacement */
+ for (i = 0; i < ogg->nstreams; i++) {
+ if (ogg->streams[i].codec == codec)
+ break;
+ }
- if (!ogg->state || ogg->state->streams[i].private != os->private)
- av_freep(&ogg->streams[i].private);
+ /* Otherwise, create a new stream */
+ if (i >= ogg->nstreams)
+ return ogg_new_stream(s, serial);
- /* Set Ogg stream settings similar to what is done in ogg_new_stream(). We
- * also re-use the ogg_stream allocated buffer */
- memset(os, 0, sizeof(*os));
- os->serial = serial;
- os->bufsize = bufsize;
- os->buf = buf;
- os->header = -1;
- os->codec = codec;
+ os = &ogg->streams[i];
+ os->serial = serial;
+ os->codec = codec;
return i;
-#endif
}
static int ogg_new_stream(AVFormatContext *s, uint32_t serial)
@@ -325,6 +306,7 @@ static int ogg_read_page(AVFormatContext *s, int *sid)
uint32_t crc, crc_tmp;
int size = 0, idx;
int64_t version, page_pos;
+ int64_t start_pos;
uint8_t sync[4];
uint8_t segments[255];
uint8_t *readout_buf;
@@ -364,6 +346,10 @@ static int ogg_read_page(AVFormatContext *s, int *sid)
/* 0x4fa9b05f = av_crc(AV_CRC_32_IEEE, 0x0, "OggS", 4) */
ffio_init_checksum(bc, ff_crc04C11DB7_update, 0x4fa9b05f);
+ /* To rewind if checksum is bad/check magic on switches - this is the max packet size */
+ ffio_ensure_seekback(bc, MAX_PAGE_SIZE);
+ start_pos = avio_tell(bc);
+
version = avio_r8(bc);
flags = avio_r8(bc);
gp = avio_rl64(bc);
@@ -414,7 +400,7 @@ static int ogg_read_page(AVFormatContext *s, int *sid)
av_log(s, AV_LOG_ERROR, "CRC mismatch!\n");
if (idx < 0)
av_free(readout_buf);
- avio_seek(bc, -size, SEEK_CUR);
+ avio_seek(bc, start_pos, SEEK_SET);
return 0;
}
@@ -424,14 +410,14 @@ static int ogg_read_page(AVFormatContext *s, int *sid)
av_log(s, AV_LOG_ERROR, "Invalid Ogg vers!\n");
if (idx < 0)
av_free(readout_buf);
- avio_seek(bc, -size, SEEK_CUR);
+ avio_seek(bc, start_pos, SEEK_SET);
return 0;
}
/* CRC is correct so we can be 99% sure there's an actual change here */
if (idx < 0) {
if (data_packets_seen(ogg))
- idx = ogg_replace_stream(s, serial, size);
+ idx = ogg_replace_stream(s, serial, readout_buf);
else
idx = ogg_new_stream(s, serial);
--
2.26.2
_______________________________________________
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".
