* [FFmpeg-devel] [PATCH] avformat: add CRI USM demuxer
@ 2023-09-05 21:37 Paul B Mahol
2023-09-06 9:27 ` Andreas Rheinhardt
2023-09-10 12:02 ` Paul B Mahol
0 siblings, 2 replies; 17+ messages in thread
From: Paul B Mahol @ 2023-09-05 21:37 UTC (permalink / raw)
To: FFmpeg development discussions and patches
[-- Attachment #1: Type: text/plain, Size: 10 bytes --]
Attached.
[-- Attachment #2: 0001-avformat-add-CRI-USM-demuxer.patch --]
[-- Type: text/x-patch, Size: 11757 bytes --]
From fc0f592a04ffa99b94b1402905d0bcfb2b15270c Mon Sep 17 00:00:00 2001
From: Paul B Mahol <onemda@gmail.com>
Date: Tue, 5 Sep 2023 16:53:32 +0200
Subject: [PATCH] avformat: add CRI USM demuxer
Signed-off-by: Paul B Mahol <onemda@gmail.com>
---
libavformat/Makefile | 1 +
libavformat/allformats.c | 1 +
libavformat/usmdec.c | 327 +++++++++++++++++++++++++++++++++++++++
3 files changed, 329 insertions(+)
create mode 100644 libavformat/usmdec.c
diff --git a/libavformat/Makefile b/libavformat/Makefile
index cc1b12360a..329055ccfd 100644
--- a/libavformat/Makefile
+++ b/libavformat/Makefile
@@ -588,6 +588,7 @@ OBJS-$(CONFIG_TTY_DEMUXER) += tty.o sauce.o
OBJS-$(CONFIG_TY_DEMUXER) += ty.o
OBJS-$(CONFIG_TXD_DEMUXER) += txd.o
OBJS-$(CONFIG_UNCODEDFRAMECRC_MUXER) += uncodedframecrcenc.o framehash.o
+OBJS-$(CONFIG_USM_DEMUXER) += usmdec.o
OBJS-$(CONFIG_V210_DEMUXER) += rawvideodec.o
OBJS-$(CONFIG_V210X_DEMUXER) += rawvideodec.o
OBJS-$(CONFIG_VAG_DEMUXER) += vag.o
diff --git a/libavformat/allformats.c b/libavformat/allformats.c
index f4210e4932..d4b505a5a3 100644
--- a/libavformat/allformats.c
+++ b/libavformat/allformats.c
@@ -471,6 +471,7 @@ extern const AVInputFormat ff_txd_demuxer;
extern const AVInputFormat ff_tty_demuxer;
extern const AVInputFormat ff_ty_demuxer;
extern const FFOutputFormat ff_uncodedframecrc_muxer;
+extern const AVInputFormat ff_usm_demuxer;
extern const AVInputFormat ff_v210_demuxer;
extern const AVInputFormat ff_v210x_demuxer;
extern const AVInputFormat ff_vag_demuxer;
diff --git a/libavformat/usmdec.c b/libavformat/usmdec.c
new file mode 100644
index 0000000000..63b580b9ea
--- /dev/null
+++ b/libavformat/usmdec.c
@@ -0,0 +1,327 @@
+/*
+ * USM demuxer
+ * Copyright (c) 2023 Paul B Mahol
+ *
+ * This file is part of FFmpeg.
+ *
+ * FFmpeg is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * FFmpeg is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with FFmpeg; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+#include "libavutil/intreadwrite.h"
+#include "libavcodec/bytestream.h"
+
+#include "avformat.h"
+#include "internal.h"
+
+#define VIDEOI 0
+#define AUDIOI 1
+
+typedef struct USMChannel {
+ int index;
+ int used;
+} USMChannel;
+
+typedef struct USMDemuxContext {
+ USMChannel ch[2][256];
+ int nb_channels[2];
+ uint8_t *header;
+ unsigned header_size;
+} USMDemuxContext;
+
+static int usm_probe(const AVProbeData *p)
+{
+ if (AV_RL32(p->buf) != MKTAG('C','R','I','D'))
+ return 0;
+
+ if (AV_RL32(p->buf + 4) == 0)
+ return 0;
+
+ return AVPROBE_SCORE_MAX / 3;
+}
+
+static int usm_read_header(AVFormatContext *s)
+{
+ s->ctx_flags |= AVFMTCTX_NOHEADER;
+ return 0;
+}
+
+static int parse_utf(AVFormatContext *s, AVIOContext *pb,
+ AVStream *st, AVCodecParameters *par, int is_audio)
+{
+ USMDemuxContext *usm = s->priv_data;
+ GetByteContext gb, ugb, sgb;
+ uint32_t chunk_type, chunk_size, offset;
+ uint32_t unique_offset, string_offset;
+ uint32_t byte_offset, payload_name_offset;
+ int nb_items, unique_size, nb_dictionaries;
+ AVRational fps = { 0 };
+ int type;
+
+ chunk_type = avio_rb32(pb);
+ chunk_size = avio_rb32(pb);
+
+ if (chunk_type != MKBETAG('@','U','T','F'))
+ return AVERROR_INVALIDDATA;
+
+ av_fast_malloc(&usm->header, &usm->header_size,
+ chunk_size + AV_INPUT_BUFFER_PADDING_SIZE);
+ if (!usm->header)
+ return AVERROR(ENOMEM);
+
+ if (avio_read(pb, usm->header, chunk_size) != chunk_size)
+ return AVERROR_INVALIDDATA;
+
+ bytestream2_init(&gb, usm->header, chunk_size);
+ ugb = gb;
+ sgb = gb;
+ unique_offset = bytestream2_get_be32(&gb);
+ string_offset = bytestream2_get_be32(&gb);
+ byte_offset = bytestream2_get_be32(&gb);
+ payload_name_offset = bytestream2_get_be32(&gb);
+ nb_items = bytestream2_get_be16(&gb);
+ unique_size = bytestream2_get_be16(&gb);
+ nb_dictionaries = bytestream2_get_be32(&gb);
+ if (nb_dictionaries == 0)
+ return AVERROR_INVALIDDATA;
+
+ bytestream2_skip(&ugb, unique_offset);
+ bytestream2_skip(&sgb, string_offset);
+
+ for (int i = 0; i < nb_items; i++) {
+ GetByteContext *xgb;
+ uint8_t key[256];
+ int64_t value;
+ int n = 0;
+
+ type = bytestream2_get_byte(&gb);
+ offset = bytestream2_get_be32(&gb);
+
+ bytestream2_seek(&sgb, string_offset + offset, SEEK_SET);
+ while (bytestream2_get_bytes_left(&sgb) > 0) {
+ key[n] = bytestream2_get_byte(&sgb);
+ if (!key[n])
+ break;
+ if (n >= sizeof(key) - 1)
+ break;
+ n++;
+ }
+ key[n] = '\0';
+
+ if ((type >> 5) == 1)
+ xgb = &gb;
+ else
+ xgb = &ugb;
+
+ switch (type & 0x1F) {
+ case 0x10:
+ case 0x11:
+ value = bytestream2_get_byte(xgb);
+ break;
+ case 0x12:
+ case 0x13:
+ value = bytestream2_get_be16(xgb);
+ break;
+ case 0x14:
+ case 0x15:
+ value = bytestream2_get_be32(xgb);
+ break;
+ case 0x16:
+ case 0x17:
+ value = bytestream2_get_be64(xgb);
+ break;
+ case 0x18:
+ value = av_int2float(bytestream2_get_be32(xgb));
+ break;
+ case 0x19:
+ value = av_int2double(bytestream2_get_be64(xgb));
+ break;
+ case 0x1A:
+ break;
+ }
+
+ if (is_audio) {
+ if (!strcmp(key, "sampling_rate")) {
+ par->sample_rate = value;
+ avpriv_set_pts_info(st, 64, 1, value);
+ } else if (!strcmp(key, "num_channels")) {
+ par->ch_layout.nb_channels = value;
+ } else if (!strcmp(key, "total_samples")) {
+ st->duration = value;
+ } else if (!strcmp(key, "audio_codec")) {
+ switch (value) {
+ case 2:
+ par->codec_id = AV_CODEC_ID_ADPCM_ADX;
+ break;
+ case 4:
+ par->codec_id = AV_CODEC_ID_HCA;
+ break;
+ default:
+ av_log(s, AV_LOG_ERROR, "unsupported audio: %d\n", (int)value);
+ break;
+ }
+ }
+ } else {
+ if (!strcmp(key, "width")) {
+ par->width = value;
+ } else if (!strcmp(key, "height")) {
+ par->height = value;
+ } else if (!strcmp(key, "total_frames")) {
+ st->nb_frames = value;
+ } else if (!strcmp(key, "framerate_n")) {
+ fps.num = value;
+ } else if (!strcmp(key, "framerate_d")) {
+ fps.den = value;
+ } else if (!strcmp(key, "mpeg_codec")) {
+ switch (value) {
+ case 1:
+ par->codec_id = AV_CODEC_ID_MPEG1VIDEO;
+ break;
+ case 5:
+ par->codec_id = AV_CODEC_ID_H264;
+ break;
+ case 9:
+ par->codec_id = AV_CODEC_ID_VP9;
+ break;
+ default:
+ av_log(s, AV_LOG_ERROR, "unsupported video: %d\n", (int)value);
+ break;
+ }
+ }
+ }
+ }
+
+ if (!is_audio && fps.num && fps.den)
+ avpriv_set_pts_info(st, 64, fps.den, fps.num);
+
+ return 0;
+}
+
+static int parse_chunk(AVFormatContext *s, AVIOContext *pb,
+ uint32_t chunk_type, uint32_t chunk_size,
+ AVPacket *pkt)
+{
+ USMDemuxContext *usm = s->priv_data;
+ int64_t chunk_start;
+ int stream_index, payload_type;
+ int payload_offset, frame_time;
+ int frame_rate, padding_size, ret;
+ int is_audio = chunk_type == MKBETAG('@','S','F','A');
+
+ chunk_start = avio_tell(pb);
+ avio_skip(pb, 1);
+ payload_offset = avio_r8(pb);
+ padding_size = avio_rb16(pb);
+ stream_index = avio_r8(pb);
+ avio_skip(pb, 2);
+ payload_type = avio_r8(pb);
+ frame_time = avio_rb32(pb);
+ frame_rate = avio_rb32(pb);
+ avio_skip(pb, 8);
+
+ if (payload_type == 1) {
+ if (usm->ch[is_audio][stream_index].used == 0) {
+ AVStream *st = avformat_new_stream(s, NULL);
+ AVCodecParameters *par;
+ if (!st)
+ return AVERROR(ENOMEM);
+
+ par = st->codecpar;
+ par->codec_type = is_audio ? AVMEDIA_TYPE_AUDIO : AVMEDIA_TYPE_VIDEO;
+
+ usm->ch[is_audio][stream_index].index = st->index;
+ usm->ch[is_audio][stream_index].used = 1;
+ usm->nb_channels[is_audio]++;
+
+ ret = parse_utf(s, pb, st, par, is_audio);
+ if (ret < 0)
+ return ret;
+
+ if (!st->time_base.num || !st->time_base.den)
+ avpriv_set_pts_info(st, 64, 100, frame_rate);
+ ffstream(st)->need_parsing = AVSTREAM_PARSE_FULL_RAW;
+ }
+ } else if (payload_type == 0) {
+ if (usm->ch[is_audio][stream_index].used == 1) {
+ uint32_t pkt_size = chunk_size - (avio_tell(pb) - chunk_start);
+
+ ret = av_get_packet(pb, pkt, pkt_size);
+ if (ret < 0)
+ return ret;
+
+ pkt->stream_index = usm->ch[is_audio][stream_index].index;
+
+ return 1;
+ }
+ }
+
+ avio_skip(pb, padding_size);
+ avio_skip(pb, chunk_size - (avio_tell(pb) - chunk_start));
+
+ return 0;
+}
+
+static int usm_read_packet(AVFormatContext *s, AVPacket *pkt)
+{
+ AVIOContext *pb = s->pb;
+ int64_t ret = AVERROR_EOF;
+
+ while (!avio_feof(pb)) {
+ uint32_t chunk_type, chunk_size;
+ int got_packet = 0;
+
+ chunk_type = avio_rb32(pb);
+ chunk_size = avio_rb32(pb);
+
+ switch (chunk_type) {
+ case MKBETAG('C','R','I','D'):
+ default:
+ ret = avio_skip(pb, chunk_size);
+ break;
+ case MKBETAG('@','S','F','A'):
+ case MKBETAG('@','S','F','V'):
+ ret = parse_chunk(s, pb, chunk_type, chunk_size, pkt);
+ got_packet = ret == 1;
+ break;
+ }
+
+ if (got_packet)
+ ret = 0;
+
+ if (got_packet || ret < 0)
+ break;
+ }
+
+ return ret;
+}
+
+static int usm_read_close(AVFormatContext *s)
+{
+ USMDemuxContext *usm = s->priv_data;
+ av_freep(&usm->header);
+ usm->header_size = 0;
+ return 0;
+}
+
+const AVInputFormat ff_usm_demuxer = {
+ .name = "usm",
+ .long_name = NULL_IF_CONFIG_SMALL("CRI USM"),
+ .priv_data_size = sizeof(USMDemuxContext),
+ .read_probe = usm_probe,
+ .read_header = usm_read_header,
+ .read_packet = usm_read_packet,
+ .read_close = usm_read_close,
+ .extensions = "usm",
+ .flags = AVFMT_GENERIC_INDEX,
+};
--
2.39.1
[-- Attachment #3: Type: text/plain, Size: 251 bytes --]
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avformat: add CRI USM demuxer
2023-09-05 21:37 [FFmpeg-devel] [PATCH] avformat: add CRI USM demuxer Paul B Mahol
@ 2023-09-06 9:27 ` Andreas Rheinhardt
2023-09-06 9:57 ` Paul B Mahol
2023-09-10 12:02 ` Paul B Mahol
1 sibling, 1 reply; 17+ messages in thread
From: Andreas Rheinhardt @ 2023-09-06 9:27 UTC (permalink / raw)
To: ffmpeg-devel
Paul B Mahol:
>
> + chunk_type = avio_rb32(pb);
> + chunk_size = avio_rb32(pb);
You are not checking whether the chunk here exceeds its containing chunk.
>
> + av_fast_malloc(&usm->header, &usm->header_size,
> + chunk_size + AV_INPUT_BUFFER_PADDING_SIZE);
> + if (!usm->header)
> + return AVERROR(ENOMEM);
The bytestream2 API does not rely on the buffer being padded at all.
>
> + bytestream2_skip(&sgb, string_offset);
This is unnecessary, because you seek with an absolute offset lateron
anyway before using sgb.
>
> + bytestream2_seek(&sgb, string_offset + offset, SEEK_SET);
> + while (bytestream2_get_bytes_left(&sgb) > 0) {
> + key[n] = bytestream2_get_byte(&sgb);
> + if (!key[n])
> + break;
> + if (n >= sizeof(key) - 1)
> + break;
> + n++;
> + }
> + key[n] = '\0';
IMO this would be easier with strnlen(), avoiding sgb altogether.
You would of course need to explicitly check that you are not
overreading, but that is good practice anyway.
>
> + chunk_start = avio_tell(pb);
> + avio_skip(pb, 1);
> + payload_offset = avio_r8(pb);
> + padding_size = avio_rb16(pb);
> + stream_index = avio_r8(pb);
> + avio_skip(pb, 2);
> + payload_type = avio_r8(pb);
> + frame_time = avio_rb32(pb);
> + frame_rate = avio_rb32(pb);
> + avio_skip(pb, 8);
payload_offset and frame_time are set-but-unused; this might lead to
compiler warnings.
> + if (usm->ch[is_audio][stream_index].used == 1) {
> + uint32_t pkt_size = chunk_size - (avio_tell(pb) - chunk_start);
> +
This is unnecessary: Unless we already had a read error, pkt_size is
chunk_size - (1 + 1 + 2 + 1 + 2 + 1 + 4 + 4 + 8).
(Notice that in case padding_size is > 0, it will be part of the packet
with the current code; not sure if that is an issue.)
>
> +
> + avio_skip(pb, padding_size);
> + avio_skip(pb, chunk_size - (avio_tell(pb) - chunk_start));
> +
Simpler to just use avio_seek(pb, chunk_start + chunk_size, SEEK_SET);
- Andreas
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avformat: add CRI USM demuxer
2023-09-06 9:27 ` Andreas Rheinhardt
@ 2023-09-06 9:57 ` Paul B Mahol
2023-09-06 11:31 ` Andreas Rheinhardt
0 siblings, 1 reply; 17+ messages in thread
From: Paul B Mahol @ 2023-09-06 9:57 UTC (permalink / raw)
To: FFmpeg development discussions and patches
On Wed, Sep 6, 2023 at 11:26 AM Andreas Rheinhardt <
andreas.rheinhardt@outlook.com> wrote:
> Paul B Mahol:
> >
> > + chunk_type = avio_rb32(pb);
> > + chunk_size = avio_rb32(pb);
>
> You are not checking whether the chunk here exceeds its containing chunk.
>
> >
> > + av_fast_malloc(&usm->header, &usm->header_size,
> > + chunk_size + AV_INPUT_BUFFER_PADDING_SIZE);
> > + if (!usm->header)
> > + return AVERROR(ENOMEM);
>
> The bytestream2 API does not rely on the buffer being padded at all.
>
> >
> > + bytestream2_skip(&sgb, string_offset);
>
> This is unnecessary, because you seek with an absolute offset lateron
> anyway before using sgb.
>
> >
> > + bytestream2_seek(&sgb, string_offset + offset, SEEK_SET);
> > + while (bytestream2_get_bytes_left(&sgb) > 0) {
> > + key[n] = bytestream2_get_byte(&sgb);
> > + if (!key[n])
> > + break;
> > + if (n >= sizeof(key) - 1)
> > + break;
> > + n++;
> > + }
> > + key[n] = '\0';
>
> IMO this would be easier with strnlen(), avoiding sgb altogether.
> You would of course need to explicitly check that you are not
> overreading, but that is good practice anyway.
>
> >
> > + chunk_start = avio_tell(pb);
> > + avio_skip(pb, 1);
> > + payload_offset = avio_r8(pb);
> > + padding_size = avio_rb16(pb);
> > + stream_index = avio_r8(pb);
> > + avio_skip(pb, 2);
> > + payload_type = avio_r8(pb);
> > + frame_time = avio_rb32(pb);
> > + frame_rate = avio_rb32(pb);
> > + avio_skip(pb, 8);
>
> payload_offset and frame_time are set-but-unused; this might lead to
> compiler warnings.
>
> > + if (usm->ch[is_audio][stream_index].used == 1) {
> > + uint32_t pkt_size = chunk_size - (avio_tell(pb) -
> chunk_start);
> > +
>
> This is unnecessary: Unless we already had a read error, pkt_size is
> chunk_size - (1 + 1 + 2 + 1 + 2 + 1 + 4 + 4 + 8).
>
> (Notice that in case padding_size is > 0, it will be part of the packet
> with the current code; not sure if that is an issue.)
>
> >
> > +
> > + avio_skip(pb, padding_size);
> > + avio_skip(pb, chunk_size - (avio_tell(pb) - chunk_start));
> > +
>
> Simpler to just use avio_seek(pb, chunk_start + chunk_size, SEEK_SET);
>
But input might not be seekable.
>
> - Andreas
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@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-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avformat: add CRI USM demuxer
2023-09-06 9:57 ` Paul B Mahol
@ 2023-09-06 11:31 ` Andreas Rheinhardt
2023-09-06 12:01 ` Paul B Mahol
0 siblings, 1 reply; 17+ messages in thread
From: Andreas Rheinhardt @ 2023-09-06 11:31 UTC (permalink / raw)
To: ffmpeg-devel
Paul B Mahol:
> On Wed, Sep 6, 2023 at 11:26 AM Andreas Rheinhardt <
> andreas.rheinhardt@outlook.com> wrote:
>
>> Paul B Mahol:
>>>
>>> + chunk_type = avio_rb32(pb);
>>> + chunk_size = avio_rb32(pb);
>>
>> You are not checking whether the chunk here exceeds its containing chunk.
>>
>>>
>>> + av_fast_malloc(&usm->header, &usm->header_size,
>>> + chunk_size + AV_INPUT_BUFFER_PADDING_SIZE);
>>> + if (!usm->header)
>>> + return AVERROR(ENOMEM);
>>
>> The bytestream2 API does not rely on the buffer being padded at all.
>>
>>>
>>> + bytestream2_skip(&sgb, string_offset);
>>
>> This is unnecessary, because you seek with an absolute offset lateron
>> anyway before using sgb.
>>
>>>
>>> + bytestream2_seek(&sgb, string_offset + offset, SEEK_SET);
>>> + while (bytestream2_get_bytes_left(&sgb) > 0) {
>>> + key[n] = bytestream2_get_byte(&sgb);
>>> + if (!key[n])
>>> + break;
>>> + if (n >= sizeof(key) - 1)
>>> + break;
>>> + n++;
>>> + }
>>> + key[n] = '\0';
>>
>> IMO this would be easier with strnlen(), avoiding sgb altogether.
>> You would of course need to explicitly check that you are not
>> overreading, but that is good practice anyway.
>>
>>>
>>> + chunk_start = avio_tell(pb);
>>> + avio_skip(pb, 1);
>>> + payload_offset = avio_r8(pb);
>>> + padding_size = avio_rb16(pb);
>>> + stream_index = avio_r8(pb);
>>> + avio_skip(pb, 2);
>>> + payload_type = avio_r8(pb);
>>> + frame_time = avio_rb32(pb);
>>> + frame_rate = avio_rb32(pb);
>>> + avio_skip(pb, 8);
>>
>> payload_offset and frame_time are set-but-unused; this might lead to
>> compiler warnings.
>>
>>> + if (usm->ch[is_audio][stream_index].used == 1) {
>>> + uint32_t pkt_size = chunk_size - (avio_tell(pb) -
>> chunk_start);
>>> +
>>
>> This is unnecessary: Unless we already had a read error, pkt_size is
>> chunk_size - (1 + 1 + 2 + 1 + 2 + 1 + 4 + 4 + 8).
>>
>> (Notice that in case padding_size is > 0, it will be part of the packet
>> with the current code; not sure if that is an issue.)
>>
>>>
>>> +
>>> + avio_skip(pb, padding_size);
>>> + avio_skip(pb, chunk_size - (avio_tell(pb) - chunk_start));
>>> +
>>
>> Simpler to just use avio_seek(pb, chunk_start + chunk_size, SEEK_SET);
>>
>
> But input might not be seekable.
>
And? You know that avio_skip(pb, offset) is just avio_seek(pb, offset,
SEEK_CUR)?
- Andreas
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avformat: add CRI USM demuxer
2023-09-06 11:31 ` Andreas Rheinhardt
@ 2023-09-06 12:01 ` Paul B Mahol
2023-09-10 12:00 ` Andreas Rheinhardt
0 siblings, 1 reply; 17+ messages in thread
From: Paul B Mahol @ 2023-09-06 12:01 UTC (permalink / raw)
To: FFmpeg development discussions and patches
On Wed, Sep 6, 2023 at 1:30 PM Andreas Rheinhardt <
andreas.rheinhardt@outlook.com> wrote:
> Paul B Mahol:
> > On Wed, Sep 6, 2023 at 11:26 AM Andreas Rheinhardt <
> > andreas.rheinhardt@outlook.com> wrote:
> >
> >> Paul B Mahol:
> >>>
> >>> + chunk_type = avio_rb32(pb);
> >>> + chunk_size = avio_rb32(pb);
> >>
> >> You are not checking whether the chunk here exceeds its containing
> chunk.
> >>
> >>>
> >>> + av_fast_malloc(&usm->header, &usm->header_size,
> >>> + chunk_size + AV_INPUT_BUFFER_PADDING_SIZE);
> >>> + if (!usm->header)
> >>> + return AVERROR(ENOMEM);
> >>
> >> The bytestream2 API does not rely on the buffer being padded at all.
> >>
> >>>
> >>> + bytestream2_skip(&sgb, string_offset);
> >>
> >> This is unnecessary, because you seek with an absolute offset lateron
> >> anyway before using sgb.
> >>
> >>>
> >>> + bytestream2_seek(&sgb, string_offset + offset, SEEK_SET);
> >>> + while (bytestream2_get_bytes_left(&sgb) > 0) {
> >>> + key[n] = bytestream2_get_byte(&sgb);
> >>> + if (!key[n])
> >>> + break;
> >>> + if (n >= sizeof(key) - 1)
> >>> + break;
> >>> + n++;
> >>> + }
> >>> + key[n] = '\0';
> >>
> >> IMO this would be easier with strnlen(), avoiding sgb altogether.
> >> You would of course need to explicitly check that you are not
> >> overreading, but that is good practice anyway.
> >>
> >>>
> >>> + chunk_start = avio_tell(pb);
> >>> + avio_skip(pb, 1);
> >>> + payload_offset = avio_r8(pb);
> >>> + padding_size = avio_rb16(pb);
> >>> + stream_index = avio_r8(pb);
> >>> + avio_skip(pb, 2);
> >>> + payload_type = avio_r8(pb);
> >>> + frame_time = avio_rb32(pb);
> >>> + frame_rate = avio_rb32(pb);
> >>> + avio_skip(pb, 8);
> >>
> >> payload_offset and frame_time are set-but-unused; this might lead to
> >> compiler warnings.
> >>
> >>> + if (usm->ch[is_audio][stream_index].used == 1) {
> >>> + uint32_t pkt_size = chunk_size - (avio_tell(pb) -
> >> chunk_start);
> >>> +
> >>
> >> This is unnecessary: Unless we already had a read error, pkt_size is
> >> chunk_size - (1 + 1 + 2 + 1 + 2 + 1 + 4 + 4 + 8).
> >>
> >> (Notice that in case padding_size is > 0, it will be part of the packet
> >> with the current code; not sure if that is an issue.)
> >>
> >>>
> >>> +
> >>> + avio_skip(pb, padding_size);
> >>> + avio_skip(pb, chunk_size - (avio_tell(pb) - chunk_start));
> >>> +
> >>
> >> Simpler to just use avio_seek(pb, chunk_start + chunk_size, SEEK_SET);
> >>
> >
> > But input might not be seekable.
> >
>
> And? You know that avio_skip(pb, offset) is just avio_seek(pb, offset,
> SEEK_CUR)?
>
And? Do you know that SEEK_SET is different from SEEK_CUR with positive
argument.
>
> - Andreas
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@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-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avformat: add CRI USM demuxer
2023-09-06 12:01 ` Paul B Mahol
@ 2023-09-10 12:00 ` Andreas Rheinhardt
2023-09-10 12:10 ` Paul B Mahol
0 siblings, 1 reply; 17+ messages in thread
From: Andreas Rheinhardt @ 2023-09-10 12:00 UTC (permalink / raw)
To: ffmpeg-devel
Paul B Mahol:
> On Wed, Sep 6, 2023 at 1:30 PM Andreas Rheinhardt <
> andreas.rheinhardt@outlook.com> wrote:
>
>> Paul B Mahol:
>>> On Wed, Sep 6, 2023 at 11:26 AM Andreas Rheinhardt <
>>> andreas.rheinhardt@outlook.com> wrote:
>>>
>>>> Paul B Mahol:
>>>>>
>>>>> + chunk_type = avio_rb32(pb);
>>>>> + chunk_size = avio_rb32(pb);
>>>>
>>>> You are not checking whether the chunk here exceeds its containing
>> chunk.
>>>>
>>>>>
>>>>> + av_fast_malloc(&usm->header, &usm->header_size,
>>>>> + chunk_size + AV_INPUT_BUFFER_PADDING_SIZE);
>>>>> + if (!usm->header)
>>>>> + return AVERROR(ENOMEM);
>>>>
>>>> The bytestream2 API does not rely on the buffer being padded at all.
>>>>
>>>>>
>>>>> + bytestream2_skip(&sgb, string_offset);
>>>>
>>>> This is unnecessary, because you seek with an absolute offset lateron
>>>> anyway before using sgb.
>>>>
>>>>>
>>>>> + bytestream2_seek(&sgb, string_offset + offset, SEEK_SET);
>>>>> + while (bytestream2_get_bytes_left(&sgb) > 0) {
>>>>> + key[n] = bytestream2_get_byte(&sgb);
>>>>> + if (!key[n])
>>>>> + break;
>>>>> + if (n >= sizeof(key) - 1)
>>>>> + break;
>>>>> + n++;
>>>>> + }
>>>>> + key[n] = '\0';
>>>>
>>>> IMO this would be easier with strnlen(), avoiding sgb altogether.
>>>> You would of course need to explicitly check that you are not
>>>> overreading, but that is good practice anyway.
>>>>
>>>>>
>>>>> + chunk_start = avio_tell(pb);
>>>>> + avio_skip(pb, 1);
>>>>> + payload_offset = avio_r8(pb);
>>>>> + padding_size = avio_rb16(pb);
>>>>> + stream_index = avio_r8(pb);
>>>>> + avio_skip(pb, 2);
>>>>> + payload_type = avio_r8(pb);
>>>>> + frame_time = avio_rb32(pb);
>>>>> + frame_rate = avio_rb32(pb);
>>>>> + avio_skip(pb, 8);
>>>>
>>>> payload_offset and frame_time are set-but-unused; this might lead to
>>>> compiler warnings.
>>>>
>>>>> + if (usm->ch[is_audio][stream_index].used == 1) {
>>>>> + uint32_t pkt_size = chunk_size - (avio_tell(pb) -
>>>> chunk_start);
>>>>> +
>>>>
>>>> This is unnecessary: Unless we already had a read error, pkt_size is
>>>> chunk_size - (1 + 1 + 2 + 1 + 2 + 1 + 4 + 4 + 8).
>>>>
>>>> (Notice that in case padding_size is > 0, it will be part of the packet
>>>> with the current code; not sure if that is an issue.)
>>>>
>>>>>
>>>>> +
>>>>> + avio_skip(pb, padding_size);
>>>>> + avio_skip(pb, chunk_size - (avio_tell(pb) - chunk_start));
>>>>> +
>>>>
>>>> Simpler to just use avio_seek(pb, chunk_start + chunk_size, SEEK_SET);
>>>>
>>>
>>> But input might not be seekable.
>>>
>>
>> And? You know that avio_skip(pb, offset) is just avio_seek(pb, offset,
>> SEEK_CUR)?
>>
>
> And? Do you know that SEEK_SET is different from SEEK_CUR with positive
> argument.
>
You are using SEEK_CUR with -avio_tell(pb), which effectively makes it
an absolute seek.
- Andreas
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avformat: add CRI USM demuxer
2023-09-05 21:37 [FFmpeg-devel] [PATCH] avformat: add CRI USM demuxer Paul B Mahol
2023-09-06 9:27 ` Andreas Rheinhardt
@ 2023-09-10 12:02 ` Paul B Mahol
2023-09-16 12:19 ` Paul B Mahol
1 sibling, 1 reply; 17+ messages in thread
From: Paul B Mahol @ 2023-09-10 12:02 UTC (permalink / raw)
To: FFmpeg development discussions and patches
[-- Attachment #1: Type: text/plain, Size: 320 bytes --]
New version attached:
- fixed VP9 demuxing
- added support for alpha streams
- added support for subtitle streams
- numerous fixes and improvements
Can't get seeking to behave correctly with ADPCM_ADX audio streams.
Once one seek to start of file audio is no longer demuxed and video packets
are filling all queue.
[-- Attachment #2: 0001-avformat-add-CRI-USM-demuxer.patch --]
[-- Type: text/x-patch, Size: 13469 bytes --]
From 5c32d4a9edb4f87ac2909909c732841bb6871e48 Mon Sep 17 00:00:00 2001
From: Paul B Mahol <onemda@gmail.com>
Date: Tue, 5 Sep 2023 16:53:32 +0200
Subject: [PATCH] avformat: add CRI USM demuxer
Signed-off-by: Paul B Mahol <onemda@gmail.com>
---
libavformat/Makefile | 1 +
libavformat/allformats.c | 1 +
libavformat/usmdec.c | 379 +++++++++++++++++++++++++++++++++++++++
3 files changed, 381 insertions(+)
create mode 100644 libavformat/usmdec.c
diff --git a/libavformat/Makefile b/libavformat/Makefile
index cc1b12360a..329055ccfd 100644
--- a/libavformat/Makefile
+++ b/libavformat/Makefile
@@ -588,6 +588,7 @@ OBJS-$(CONFIG_TTY_DEMUXER) += tty.o sauce.o
OBJS-$(CONFIG_TY_DEMUXER) += ty.o
OBJS-$(CONFIG_TXD_DEMUXER) += txd.o
OBJS-$(CONFIG_UNCODEDFRAMECRC_MUXER) += uncodedframecrcenc.o framehash.o
+OBJS-$(CONFIG_USM_DEMUXER) += usmdec.o
OBJS-$(CONFIG_V210_DEMUXER) += rawvideodec.o
OBJS-$(CONFIG_V210X_DEMUXER) += rawvideodec.o
OBJS-$(CONFIG_VAG_DEMUXER) += vag.o
diff --git a/libavformat/allformats.c b/libavformat/allformats.c
index f4210e4932..d4b505a5a3 100644
--- a/libavformat/allformats.c
+++ b/libavformat/allformats.c
@@ -471,6 +471,7 @@ extern const AVInputFormat ff_txd_demuxer;
extern const AVInputFormat ff_tty_demuxer;
extern const AVInputFormat ff_ty_demuxer;
extern const FFOutputFormat ff_uncodedframecrc_muxer;
+extern const AVInputFormat ff_usm_demuxer;
extern const AVInputFormat ff_v210_demuxer;
extern const AVInputFormat ff_v210x_demuxer;
extern const AVInputFormat ff_vag_demuxer;
diff --git a/libavformat/usmdec.c b/libavformat/usmdec.c
new file mode 100644
index 0000000000..f8bfe1a123
--- /dev/null
+++ b/libavformat/usmdec.c
@@ -0,0 +1,379 @@
+/*
+ * USM demuxer
+ * Copyright (c) 2023 Paul B Mahol
+ *
+ * This file is part of FFmpeg.
+ *
+ * FFmpeg is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * FFmpeg is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with FFmpeg; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+#include "libavutil/intreadwrite.h"
+#include "libavcodec/bytestream.h"
+
+#include "avformat.h"
+#include "internal.h"
+
+#define VIDEOI 0
+#define AUDIOI 1
+#define ALPHAI 2
+#define SUBTTI 3
+
+typedef struct USMChannel {
+ int index;
+ int used;
+} USMChannel;
+
+typedef struct USMDemuxContext {
+ USMChannel ch[4][256];
+ int nb_channels[4];
+ uint8_t *header;
+ unsigned header_size;
+} USMDemuxContext;
+
+static int usm_probe(const AVProbeData *p)
+{
+ if (AV_RL32(p->buf) != MKTAG('C','R','I','D'))
+ return 0;
+
+ if (AV_RN32(p->buf + 4) == 0)
+ return 0;
+
+ return AVPROBE_SCORE_MAX / 3;
+}
+
+static int usm_read_header(AVFormatContext *s)
+{
+ s->ctx_flags |= AVFMTCTX_NOHEADER;
+ return 0;
+}
+
+static int parse_utf(AVFormatContext *s, AVIOContext *pb,
+ AVStream *st, AVCodecParameters *par, int ch_type,
+ uint32_t parent_chunk_size)
+{
+ USMDemuxContext *usm = s->priv_data;
+ GetByteContext gb, ugb, sgb;
+ uint32_t chunk_type, chunk_size, offset;
+ uint32_t unique_offset, string_offset;
+ int nb_items, unique_size, nb_dictionaries;
+ AVRational fps = { 0 };
+ int type;
+
+ chunk_type = avio_rb32(pb);
+ chunk_size = avio_rb32(pb);
+
+ if (chunk_type != MKBETAG('@','U','T','F'))
+ return AVERROR_INVALIDDATA;
+
+ if (!chunk_size || chunk_size >= parent_chunk_size)
+ return AVERROR_INVALIDDATA;
+
+ av_fast_malloc(&usm->header, &usm->header_size, chunk_size);
+ if (!usm->header)
+ return AVERROR(ENOMEM);
+
+ if (avio_read(pb, usm->header, chunk_size) != chunk_size)
+ return AVERROR_EOF;
+
+ bytestream2_init(&gb, usm->header, chunk_size);
+ ugb = gb;
+ sgb = gb;
+ unique_offset = bytestream2_get_be32(&gb);
+ string_offset = bytestream2_get_be32(&gb);
+ /*byte_offset =*/ bytestream2_get_be32(&gb);
+ /*payload_name_offset =*/ bytestream2_get_be32(&gb);
+ nb_items = bytestream2_get_be16(&gb);
+ unique_size = bytestream2_get_be16(&gb);
+ nb_dictionaries = bytestream2_get_be32(&gb);
+ if (nb_dictionaries == 0)
+ return AVERROR_INVALIDDATA;
+
+ bytestream2_skip(&ugb, unique_offset);
+ if (bytestream2_get_bytes_left(&ugb) < unique_size)
+ return AVERROR_INVALIDDATA;
+ bytestream2_init(&ugb, ugb.buffer, unique_size);
+
+ bytestream2_skip(&sgb, string_offset);
+
+ for (int i = 0; i < nb_items; i++) {
+ GetByteContext *xgb;
+ uint8_t key[256];
+ int64_t value;
+ int n = 0;
+
+ type = bytestream2_get_byte(&gb);
+ offset = bytestream2_get_be32(&gb);
+
+ bytestream2_seek(&sgb, string_offset + offset, SEEK_SET);
+ while (bytestream2_get_bytes_left(&sgb) > 0) {
+ key[n] = bytestream2_get_byte(&sgb);
+ if (!key[n])
+ break;
+ if (n >= sizeof(key) - 1)
+ break;
+ n++;
+ }
+ key[n] = '\0';
+
+ if ((type >> 5) == 1)
+ xgb = &gb;
+ else
+ xgb = &ugb;
+
+ switch (type & 0x1F) {
+ case 0x10:
+ case 0x11:
+ value = bytestream2_get_byte(xgb);
+ break;
+ case 0x12:
+ case 0x13:
+ value = bytestream2_get_be16(xgb);
+ break;
+ case 0x14:
+ case 0x15:
+ value = bytestream2_get_be32(xgb);
+ break;
+ case 0x16:
+ case 0x17:
+ value = bytestream2_get_be64(xgb);
+ break;
+ case 0x18:
+ value = av_int2float(bytestream2_get_be32(xgb));
+ break;
+ case 0x19:
+ value = av_int2double(bytestream2_get_be64(xgb));
+ break;
+ case 0x1A:
+ break;
+ }
+
+ if (ch_type == AUDIOI) {
+ if (!strcmp(key, "sampling_rate")) {
+ par->sample_rate = value;
+ avpriv_set_pts_info(st, 64, 1, value);
+ } else if (!strcmp(key, "num_channels")) {
+ par->ch_layout.nb_channels = value;
+ } else if (!strcmp(key, "total_samples")) {
+ st->duration = value;
+ } else if (!strcmp(key, "audio_codec")) {
+ switch (value) {
+ case 2:
+ par->codec_id = AV_CODEC_ID_ADPCM_ADX;
+ break;
+ case 4:
+ par->codec_id = AV_CODEC_ID_HCA;
+ break;
+ default:
+ av_log(s, AV_LOG_ERROR, "unsupported audio: %d\n", (int)value);
+ break;
+ }
+ }
+ } else if (ch_type == VIDEOI || ch_type == ALPHAI) {
+ if (!strcmp(key, "width")) {
+ par->width = value;
+ } else if (!strcmp(key, "height")) {
+ par->height = value;
+ } else if (!strcmp(key, "total_frames")) {
+ st->nb_frames = value;
+ } else if (!strcmp(key, "framerate_n")) {
+ fps.num = value;
+ } else if (!strcmp(key, "framerate_d")) {
+ fps.den = value;
+ } else if (!strcmp(key, "mpeg_codec")) {
+ switch (value) {
+ case 1:
+ par->codec_id = AV_CODEC_ID_MPEG1VIDEO;
+ break;
+ case 5:
+ par->codec_id = AV_CODEC_ID_H264;
+ break;
+ case 9:
+ par->codec_id = AV_CODEC_ID_VP9;
+ break;
+ default:
+ av_log(s, AV_LOG_ERROR, "unsupported video: %d\n", (int)value);
+ break;
+ }
+ }
+ }
+ }
+
+ if (ch_type == VIDEOI && fps.num && fps.den)
+ avpriv_set_pts_info(st, 64, fps.den, fps.num);
+
+ return 0;
+}
+
+static int64_t parse_chunk(AVFormatContext *s, AVIOContext *pb,
+ uint32_t chunk_type, uint32_t chunk_size,
+ AVPacket *pkt)
+{
+ const int is_audio = chunk_type == MKBETAG('@','S','F','A');
+ const int is_alpha = chunk_type == MKBETAG('@','A','L','P');
+ const int is_subtt = chunk_type == MKBETAG('@','S','B','T');
+ USMDemuxContext *usm = s->priv_data;
+ int64_t chunk_start, ret;
+ int stream_index, payload_type;
+ int payload_offset;
+ int frame_rate, padding_size;
+ const int ch_type = is_subtt ? SUBTTI : is_audio ? AUDIOI : is_alpha ? ALPHAI : VIDEOI;
+
+ ret = avio_tell(pb);
+ if (ret < 0)
+ return ret;
+ chunk_start = ret;
+ avio_skip(pb, 1);
+ payload_offset = avio_r8(pb);
+ padding_size = avio_rb16(pb);
+ stream_index = avio_r8(pb);
+ avio_skip(pb, 2);
+ payload_type = avio_r8(pb);
+ /*frame_time =*/ avio_rb32(pb);
+ frame_rate = avio_rb32(pb);
+ avio_skip(pb, 8);
+ ret = avio_tell(pb);
+ if (ret < 0)
+ return ret;
+ ret = avio_skip(pb, FFMAX(0, (ret - chunk_start) - payload_offset));
+ if (ret < 0)
+ return ret;
+
+ if (payload_type == 1) {
+ if (usm->ch[ch_type][stream_index].used == 0) {
+ AVStream *st = avformat_new_stream(s, NULL);
+ AVCodecParameters *par;
+ if (!st)
+ return AVERROR(ENOMEM);
+
+ par = st->codecpar;
+ switch (ch_type) {
+ case ALPHAI:
+ case VIDEOI:
+ par->codec_type = AVMEDIA_TYPE_VIDEO;
+ break;
+ case AUDIOI:
+ par->codec_type = AVMEDIA_TYPE_AUDIO;
+ break;
+ case SUBTTI:
+ par->codec_type = AVMEDIA_TYPE_SUBTITLE;
+ break;
+ }
+ st->start_time = 0;
+
+ usm->ch[ch_type][stream_index].index = st->index;
+ usm->ch[ch_type][stream_index].used = 1;
+ usm->nb_channels[ch_type]++;
+
+ ret = parse_utf(s, pb, st, par, ch_type, chunk_size);
+ if (ret < 0)
+ return ret;
+
+ if (!st->time_base.num || !st->time_base.den)
+ avpriv_set_pts_info(st, 64, 100, frame_rate);
+ ffstream(st)->need_parsing = AVSTREAM_PARSE_TIMESTAMPS;
+ }
+ } else if (payload_type == 0) {
+ if (usm->ch[ch_type][stream_index].used == 1) {
+ uint32_t pkt_size;
+
+ ret = avio_tell(pb);
+ if (ret < 0)
+ return ret;
+
+ pkt_size = chunk_size - (ret - chunk_start) - padding_size;
+ ret = av_get_packet(pb, pkt, pkt_size);
+ if (ret < 0)
+ return ret;
+
+ pkt->stream_index = usm->ch[ch_type][stream_index].index;
+
+ avio_skip(pb, padding_size);
+
+ if (ret != pkt_size)
+ return AVERROR_EOF;
+ return ret;
+ }
+ }
+
+ ret = avio_tell(pb);
+ if (ret < 0)
+ return ret;
+ ret = avio_skip(pb, FFMAX(0, chunk_size - (ret - chunk_start)));
+ if (ret < 0)
+ return ret;
+ return 0;
+}
+
+static int usm_read_packet(AVFormatContext *s, AVPacket *pkt)
+{
+ AVIOContext *pb = s->pb;
+ int64_t ret = AVERROR_EOF;
+
+ while (!avio_feof(pb)) {
+ uint32_t chunk_type, chunk_size;
+ int got_packet = 0;
+ int64_t pos;
+
+ pos = avio_tell(pb);
+ if (pos < 0)
+ return pos;
+ chunk_type = avio_rb32(pb);
+ chunk_size = avio_rb32(pb);
+ if (!chunk_size)
+ return AVERROR_INVALIDDATA;
+
+ switch (chunk_type) {
+ case MKBETAG('C','R','I','D'):
+ default:
+ ret = avio_skip(pb, chunk_size);
+ break;
+ case MKBETAG('@','A','L','P'):
+ case MKBETAG('@','S','B','T'):
+ case MKBETAG('@','S','F','A'):
+ case MKBETAG('@','S','F','V'):
+ ret = parse_chunk(s, pb, chunk_type, chunk_size, pkt);
+ got_packet = ret > 0;
+ break;
+ }
+
+ if (got_packet)
+ pkt->pos = pos;
+
+ if (got_packet || ret < 0)
+ break;
+ }
+
+ return ret;
+}
+
+static int usm_read_close(AVFormatContext *s)
+{
+ USMDemuxContext *usm = s->priv_data;
+ av_freep(&usm->header);
+ usm->header_size = 0;
+ return 0;
+}
+
+const AVInputFormat ff_usm_demuxer = {
+ .name = "usm",
+ .long_name = NULL_IF_CONFIG_SMALL("CRI USM"),
+ .priv_data_size = sizeof(USMDemuxContext),
+ .read_probe = usm_probe,
+ .read_header = usm_read_header,
+ .read_packet = usm_read_packet,
+ .read_close = usm_read_close,
+ .extensions = "usm",
+ .flags = AVFMT_GENERIC_INDEX | AVFMT_NO_BYTE_SEEK | AVFMT_NOBINSEARCH,
+};
--
2.39.1
[-- Attachment #3: Type: text/plain, Size: 251 bytes --]
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avformat: add CRI USM demuxer
2023-09-10 12:10 ` Paul B Mahol
@ 2023-09-10 12:07 ` Andreas Rheinhardt
2023-09-10 12:15 ` Paul B Mahol
0 siblings, 1 reply; 17+ messages in thread
From: Andreas Rheinhardt @ 2023-09-10 12:07 UTC (permalink / raw)
To: ffmpeg-devel
Paul B Mahol:
> On Sun, Sep 10, 2023 at 1:59 PM Andreas Rheinhardt <
> andreas.rheinhardt@outlook.com> wrote:
>
>> Paul B Mahol:
>>> On Wed, Sep 6, 2023 at 1:30 PM Andreas Rheinhardt <
>>> andreas.rheinhardt@outlook.com> wrote:
>>>
>>>> Paul B Mahol:
>>>>> On Wed, Sep 6, 2023 at 11:26 AM Andreas Rheinhardt <
>>>>> andreas.rheinhardt@outlook.com> wrote:
>>>>>
>>>>>> Paul B Mahol:
>>>>>>>
>>>>>>> + chunk_type = avio_rb32(pb);
>>>>>>> + chunk_size = avio_rb32(pb);
>>>>>>
>>>>>> You are not checking whether the chunk here exceeds its containing
>>>> chunk.
>>>>>>
>>>>>>>
>>>>>>> + av_fast_malloc(&usm->header, &usm->header_size,
>>>>>>> + chunk_size + AV_INPUT_BUFFER_PADDING_SIZE);
>>>>>>> + if (!usm->header)
>>>>>>> + return AVERROR(ENOMEM);
>>>>>>
>>>>>> The bytestream2 API does not rely on the buffer being padded at all.
>>>>>>
>>>>>>>
>>>>>>> + bytestream2_skip(&sgb, string_offset);
>>>>>>
>>>>>> This is unnecessary, because you seek with an absolute offset lateron
>>>>>> anyway before using sgb.
>>>>>>
>>>>>>>
>>>>>>> + bytestream2_seek(&sgb, string_offset + offset, SEEK_SET);
>>>>>>> + while (bytestream2_get_bytes_left(&sgb) > 0) {
>>>>>>> + key[n] = bytestream2_get_byte(&sgb);
>>>>>>> + if (!key[n])
>>>>>>> + break;
>>>>>>> + if (n >= sizeof(key) - 1)
>>>>>>> + break;
>>>>>>> + n++;
>>>>>>> + }
>>>>>>> + key[n] = '\0';
>>>>>>
>>>>>> IMO this would be easier with strnlen(), avoiding sgb altogether.
>>>>>> You would of course need to explicitly check that you are not
>>>>>> overreading, but that is good practice anyway.
>>>>>>
>>>>>>>
>>>>>>> + chunk_start = avio_tell(pb);
>>>>>>> + avio_skip(pb, 1);
>>>>>>> + payload_offset = avio_r8(pb);
>>>>>>> + padding_size = avio_rb16(pb);
>>>>>>> + stream_index = avio_r8(pb);
>>>>>>> + avio_skip(pb, 2);
>>>>>>> + payload_type = avio_r8(pb);
>>>>>>> + frame_time = avio_rb32(pb);
>>>>>>> + frame_rate = avio_rb32(pb);
>>>>>>> + avio_skip(pb, 8);
>>>>>>
>>>>>> payload_offset and frame_time are set-but-unused; this might lead to
>>>>>> compiler warnings.
>>>>>>
>>>>>>> + if (usm->ch[is_audio][stream_index].used == 1) {
>>>>>>> + uint32_t pkt_size = chunk_size - (avio_tell(pb) -
>>>>>> chunk_start);
>>>>>>> +
>>>>>>
>>>>>> This is unnecessary: Unless we already had a read error, pkt_size is
>>>>>> chunk_size - (1 + 1 + 2 + 1 + 2 + 1 + 4 + 4 + 8).
>>>>>>
>>>>>> (Notice that in case padding_size is > 0, it will be part of the
>> packet
>>>>>> with the current code; not sure if that is an issue.)
>>>>>>
>>>>>>>
>>>>>>> +
>>>>>>> + avio_skip(pb, padding_size);
>>>>>>> + avio_skip(pb, chunk_size - (avio_tell(pb) - chunk_start));
>>>>>>> +
>>>>>>
>>>>>> Simpler to just use avio_seek(pb, chunk_start + chunk_size, SEEK_SET);
>>>>>>
>>>>>
>>>>> But input might not be seekable.
>>>>>
>>>>
>>>> And? You know that avio_skip(pb, offset) is just avio_seek(pb, offset,
>>>> SEEK_CUR)?
>>>>
>>>
>>> And? Do you know that SEEK_SET is different from SEEK_CUR with positive
>>> argument.
>>>
>>
>> You are using SEEK_CUR with -avio_tell(pb), which effectively makes it
>> an absolute seek.
>>
>
> Nope, I skip data only, not seeking backward ever.
>
avio_seek() internally translates any avio_seek() with SEEK_CUR to an
absolute seek (as required by the seek callbacks) and does not care
about whether it is seeking forward or backward.
- Andreas
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avformat: add CRI USM demuxer
2023-09-10 12:00 ` Andreas Rheinhardt
@ 2023-09-10 12:10 ` Paul B Mahol
2023-09-10 12:07 ` Andreas Rheinhardt
0 siblings, 1 reply; 17+ messages in thread
From: Paul B Mahol @ 2023-09-10 12:10 UTC (permalink / raw)
To: FFmpeg development discussions and patches
On Sun, Sep 10, 2023 at 1:59 PM Andreas Rheinhardt <
andreas.rheinhardt@outlook.com> wrote:
> Paul B Mahol:
> > On Wed, Sep 6, 2023 at 1:30 PM Andreas Rheinhardt <
> > andreas.rheinhardt@outlook.com> wrote:
> >
> >> Paul B Mahol:
> >>> On Wed, Sep 6, 2023 at 11:26 AM Andreas Rheinhardt <
> >>> andreas.rheinhardt@outlook.com> wrote:
> >>>
> >>>> Paul B Mahol:
> >>>>>
> >>>>> + chunk_type = avio_rb32(pb);
> >>>>> + chunk_size = avio_rb32(pb);
> >>>>
> >>>> You are not checking whether the chunk here exceeds its containing
> >> chunk.
> >>>>
> >>>>>
> >>>>> + av_fast_malloc(&usm->header, &usm->header_size,
> >>>>> + chunk_size + AV_INPUT_BUFFER_PADDING_SIZE);
> >>>>> + if (!usm->header)
> >>>>> + return AVERROR(ENOMEM);
> >>>>
> >>>> The bytestream2 API does not rely on the buffer being padded at all.
> >>>>
> >>>>>
> >>>>> + bytestream2_skip(&sgb, string_offset);
> >>>>
> >>>> This is unnecessary, because you seek with an absolute offset lateron
> >>>> anyway before using sgb.
> >>>>
> >>>>>
> >>>>> + bytestream2_seek(&sgb, string_offset + offset, SEEK_SET);
> >>>>> + while (bytestream2_get_bytes_left(&sgb) > 0) {
> >>>>> + key[n] = bytestream2_get_byte(&sgb);
> >>>>> + if (!key[n])
> >>>>> + break;
> >>>>> + if (n >= sizeof(key) - 1)
> >>>>> + break;
> >>>>> + n++;
> >>>>> + }
> >>>>> + key[n] = '\0';
> >>>>
> >>>> IMO this would be easier with strnlen(), avoiding sgb altogether.
> >>>> You would of course need to explicitly check that you are not
> >>>> overreading, but that is good practice anyway.
> >>>>
> >>>>>
> >>>>> + chunk_start = avio_tell(pb);
> >>>>> + avio_skip(pb, 1);
> >>>>> + payload_offset = avio_r8(pb);
> >>>>> + padding_size = avio_rb16(pb);
> >>>>> + stream_index = avio_r8(pb);
> >>>>> + avio_skip(pb, 2);
> >>>>> + payload_type = avio_r8(pb);
> >>>>> + frame_time = avio_rb32(pb);
> >>>>> + frame_rate = avio_rb32(pb);
> >>>>> + avio_skip(pb, 8);
> >>>>
> >>>> payload_offset and frame_time are set-but-unused; this might lead to
> >>>> compiler warnings.
> >>>>
> >>>>> + if (usm->ch[is_audio][stream_index].used == 1) {
> >>>>> + uint32_t pkt_size = chunk_size - (avio_tell(pb) -
> >>>> chunk_start);
> >>>>> +
> >>>>
> >>>> This is unnecessary: Unless we already had a read error, pkt_size is
> >>>> chunk_size - (1 + 1 + 2 + 1 + 2 + 1 + 4 + 4 + 8).
> >>>>
> >>>> (Notice that in case padding_size is > 0, it will be part of the
> packet
> >>>> with the current code; not sure if that is an issue.)
> >>>>
> >>>>>
> >>>>> +
> >>>>> + avio_skip(pb, padding_size);
> >>>>> + avio_skip(pb, chunk_size - (avio_tell(pb) - chunk_start));
> >>>>> +
> >>>>
> >>>> Simpler to just use avio_seek(pb, chunk_start + chunk_size, SEEK_SET);
> >>>>
> >>>
> >>> But input might not be seekable.
> >>>
> >>
> >> And? You know that avio_skip(pb, offset) is just avio_seek(pb, offset,
> >> SEEK_CUR)?
> >>
> >
> > And? Do you know that SEEK_SET is different from SEEK_CUR with positive
> > argument.
> >
>
> You are using SEEK_CUR with -avio_tell(pb), which effectively makes it
> an absolute seek.
>
Nope, I skip data only, not seeking backward ever.
>
> - Andreas
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@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-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avformat: add CRI USM demuxer
2023-09-10 12:07 ` Andreas Rheinhardt
@ 2023-09-10 12:15 ` Paul B Mahol
2023-09-10 12:19 ` Andreas Rheinhardt
0 siblings, 1 reply; 17+ messages in thread
From: Paul B Mahol @ 2023-09-10 12:15 UTC (permalink / raw)
To: FFmpeg development discussions and patches
On Sun, Sep 10, 2023 at 2:06 PM Andreas Rheinhardt <
andreas.rheinhardt@outlook.com> wrote:
> Paul B Mahol:
> > On Sun, Sep 10, 2023 at 1:59 PM Andreas Rheinhardt <
> > andreas.rheinhardt@outlook.com> wrote:
> >
> >> Paul B Mahol:
> >>> On Wed, Sep 6, 2023 at 1:30 PM Andreas Rheinhardt <
> >>> andreas.rheinhardt@outlook.com> wrote:
> >>>
> >>>> Paul B Mahol:
> >>>>> On Wed, Sep 6, 2023 at 11:26 AM Andreas Rheinhardt <
> >>>>> andreas.rheinhardt@outlook.com> wrote:
> >>>>>
> >>>>>> Paul B Mahol:
> >>>>>>>
> >>>>>>> + chunk_type = avio_rb32(pb);
> >>>>>>> + chunk_size = avio_rb32(pb);
> >>>>>>
> >>>>>> You are not checking whether the chunk here exceeds its containing
> >>>> chunk.
> >>>>>>
> >>>>>>>
> >>>>>>> + av_fast_malloc(&usm->header, &usm->header_size,
> >>>>>>> + chunk_size + AV_INPUT_BUFFER_PADDING_SIZE);
> >>>>>>> + if (!usm->header)
> >>>>>>> + return AVERROR(ENOMEM);
> >>>>>>
> >>>>>> The bytestream2 API does not rely on the buffer being padded at all.
> >>>>>>
> >>>>>>>
> >>>>>>> + bytestream2_skip(&sgb, string_offset);
> >>>>>>
> >>>>>> This is unnecessary, because you seek with an absolute offset
> lateron
> >>>>>> anyway before using sgb.
> >>>>>>
> >>>>>>>
> >>>>>>> + bytestream2_seek(&sgb, string_offset + offset, SEEK_SET);
> >>>>>>> + while (bytestream2_get_bytes_left(&sgb) > 0) {
> >>>>>>> + key[n] = bytestream2_get_byte(&sgb);
> >>>>>>> + if (!key[n])
> >>>>>>> + break;
> >>>>>>> + if (n >= sizeof(key) - 1)
> >>>>>>> + break;
> >>>>>>> + n++;
> >>>>>>> + }
> >>>>>>> + key[n] = '\0';
> >>>>>>
> >>>>>> IMO this would be easier with strnlen(), avoiding sgb altogether.
> >>>>>> You would of course need to explicitly check that you are not
> >>>>>> overreading, but that is good practice anyway.
> >>>>>>
> >>>>>>>
> >>>>>>> + chunk_start = avio_tell(pb);
> >>>>>>> + avio_skip(pb, 1);
> >>>>>>> + payload_offset = avio_r8(pb);
> >>>>>>> + padding_size = avio_rb16(pb);
> >>>>>>> + stream_index = avio_r8(pb);
> >>>>>>> + avio_skip(pb, 2);
> >>>>>>> + payload_type = avio_r8(pb);
> >>>>>>> + frame_time = avio_rb32(pb);
> >>>>>>> + frame_rate = avio_rb32(pb);
> >>>>>>> + avio_skip(pb, 8);
> >>>>>>
> >>>>>> payload_offset and frame_time are set-but-unused; this might lead to
> >>>>>> compiler warnings.
> >>>>>>
> >>>>>>> + if (usm->ch[is_audio][stream_index].used == 1) {
> >>>>>>> + uint32_t pkt_size = chunk_size - (avio_tell(pb) -
> >>>>>> chunk_start);
> >>>>>>> +
> >>>>>>
> >>>>>> This is unnecessary: Unless we already had a read error, pkt_size is
> >>>>>> chunk_size - (1 + 1 + 2 + 1 + 2 + 1 + 4 + 4 + 8).
> >>>>>>
> >>>>>> (Notice that in case padding_size is > 0, it will be part of the
> >> packet
> >>>>>> with the current code; not sure if that is an issue.)
> >>>>>>
> >>>>>>>
> >>>>>>> +
> >>>>>>> + avio_skip(pb, padding_size);
> >>>>>>> + avio_skip(pb, chunk_size - (avio_tell(pb) - chunk_start));
> >>>>>>> +
> >>>>>>
> >>>>>> Simpler to just use avio_seek(pb, chunk_start + chunk_size,
> SEEK_SET);
> >>>>>>
> >>>>>
> >>>>> But input might not be seekable.
> >>>>>
> >>>>
> >>>> And? You know that avio_skip(pb, offset) is just avio_seek(pb, offset,
> >>>> SEEK_CUR)?
> >>>>
> >>>
> >>> And? Do you know that SEEK_SET is different from SEEK_CUR with positive
> >>> argument.
> >>>
> >>
> >> You are using SEEK_CUR with -avio_tell(pb), which effectively makes it
> >> an absolute seek.
> >>
> >
> > Nope, I skip data only, not seeking backward ever.
> >
>
> avio_seek() internally translates any avio_seek() with SEEK_CUR to an
> absolute seek (as required by the seek callbacks) and does not care
> about whether it is seeking forward or backward.
>
Irrelevant. Seeking needs enough cache to work on non-seekable input.
>
> - Andreas
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@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-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avformat: add CRI USM demuxer
2023-09-10 12:15 ` Paul B Mahol
@ 2023-09-10 12:19 ` Andreas Rheinhardt
2023-09-10 12:29 ` Paul B Mahol
0 siblings, 1 reply; 17+ messages in thread
From: Andreas Rheinhardt @ 2023-09-10 12:19 UTC (permalink / raw)
To: ffmpeg-devel
Paul B Mahol:
> On Sun, Sep 10, 2023 at 2:06 PM Andreas Rheinhardt <
> andreas.rheinhardt@outlook.com> wrote:
>
>> Paul B Mahol:
>>> On Sun, Sep 10, 2023 at 1:59 PM Andreas Rheinhardt <
>>> andreas.rheinhardt@outlook.com> wrote:
>>>
>>>> Paul B Mahol:
>>>>> On Wed, Sep 6, 2023 at 1:30 PM Andreas Rheinhardt <
>>>>> andreas.rheinhardt@outlook.com> wrote:
>>>>>
>>>>>> Paul B Mahol:
>>>>>>> On Wed, Sep 6, 2023 at 11:26 AM Andreas Rheinhardt <
>>>>>>> andreas.rheinhardt@outlook.com> wrote:
>>>>>>>
>>>>>>>> Paul B Mahol:
>>>>>>>>>
>>>>>>>>> + chunk_type = avio_rb32(pb);
>>>>>>>>> + chunk_size = avio_rb32(pb);
>>>>>>>>
>>>>>>>> You are not checking whether the chunk here exceeds its containing
>>>>>> chunk.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> + av_fast_malloc(&usm->header, &usm->header_size,
>>>>>>>>> + chunk_size + AV_INPUT_BUFFER_PADDING_SIZE);
>>>>>>>>> + if (!usm->header)
>>>>>>>>> + return AVERROR(ENOMEM);
>>>>>>>>
>>>>>>>> The bytestream2 API does not rely on the buffer being padded at all.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> + bytestream2_skip(&sgb, string_offset);
>>>>>>>>
>>>>>>>> This is unnecessary, because you seek with an absolute offset
>> lateron
>>>>>>>> anyway before using sgb.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> + bytestream2_seek(&sgb, string_offset + offset, SEEK_SET);
>>>>>>>>> + while (bytestream2_get_bytes_left(&sgb) > 0) {
>>>>>>>>> + key[n] = bytestream2_get_byte(&sgb);
>>>>>>>>> + if (!key[n])
>>>>>>>>> + break;
>>>>>>>>> + if (n >= sizeof(key) - 1)
>>>>>>>>> + break;
>>>>>>>>> + n++;
>>>>>>>>> + }
>>>>>>>>> + key[n] = '\0';
>>>>>>>>
>>>>>>>> IMO this would be easier with strnlen(), avoiding sgb altogether.
>>>>>>>> You would of course need to explicitly check that you are not
>>>>>>>> overreading, but that is good practice anyway.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> + chunk_start = avio_tell(pb);
>>>>>>>>> + avio_skip(pb, 1);
>>>>>>>>> + payload_offset = avio_r8(pb);
>>>>>>>>> + padding_size = avio_rb16(pb);
>>>>>>>>> + stream_index = avio_r8(pb);
>>>>>>>>> + avio_skip(pb, 2);
>>>>>>>>> + payload_type = avio_r8(pb);
>>>>>>>>> + frame_time = avio_rb32(pb);
>>>>>>>>> + frame_rate = avio_rb32(pb);
>>>>>>>>> + avio_skip(pb, 8);
>>>>>>>>
>>>>>>>> payload_offset and frame_time are set-but-unused; this might lead to
>>>>>>>> compiler warnings.
>>>>>>>>
>>>>>>>>> + if (usm->ch[is_audio][stream_index].used == 1) {
>>>>>>>>> + uint32_t pkt_size = chunk_size - (avio_tell(pb) -
>>>>>>>> chunk_start);
>>>>>>>>> +
>>>>>>>>
>>>>>>>> This is unnecessary: Unless we already had a read error, pkt_size is
>>>>>>>> chunk_size - (1 + 1 + 2 + 1 + 2 + 1 + 4 + 4 + 8).
>>>>>>>>
>>>>>>>> (Notice that in case padding_size is > 0, it will be part of the
>>>> packet
>>>>>>>> with the current code; not sure if that is an issue.)
>>>>>>>>
>>>>>>>>>
>>>>>>>>> +
>>>>>>>>> + avio_skip(pb, padding_size);
>>>>>>>>> + avio_skip(pb, chunk_size - (avio_tell(pb) - chunk_start));
>>>>>>>>> +
>>>>>>>>
>>>>>>>> Simpler to just use avio_seek(pb, chunk_start + chunk_size,
>> SEEK_SET);
>>>>>>>>
>>>>>>>
>>>>>>> But input might not be seekable.
>>>>>>>
>>>>>>
>>>>>> And? You know that avio_skip(pb, offset) is just avio_seek(pb, offset,
>>>>>> SEEK_CUR)?
>>>>>>
>>>>>
>>>>> And? Do you know that SEEK_SET is different from SEEK_CUR with positive
>>>>> argument.
>>>>>
>>>>
>>>> You are using SEEK_CUR with -avio_tell(pb), which effectively makes it
>>>> an absolute seek.
>>>>
>>>
>>> Nope, I skip data only, not seeking backward ever.
>>>
>>
>> avio_seek() internally translates any avio_seek() with SEEK_CUR to an
>> absolute seek (as required by the seek callbacks) and does not care
>> about whether it is seeking forward or backward.
>>
>
> Irrelevant. Seeking needs enough cache to work on non-seekable input.
>
Given that avio_skip() is just a wrapper around avio_seek(), the
behaviour will not change even when the input is non-seekable.
- Andreas
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avformat: add CRI USM demuxer
2023-09-10 12:19 ` Andreas Rheinhardt
@ 2023-09-10 12:29 ` Paul B Mahol
2023-09-10 12:55 ` Andreas Rheinhardt
0 siblings, 1 reply; 17+ messages in thread
From: Paul B Mahol @ 2023-09-10 12:29 UTC (permalink / raw)
To: FFmpeg development discussions and patches
On Sun, Sep 10, 2023 at 2:18 PM Andreas Rheinhardt <
andreas.rheinhardt@outlook.com> wrote:
> Paul B Mahol:
> > On Sun, Sep 10, 2023 at 2:06 PM Andreas Rheinhardt <
> > andreas.rheinhardt@outlook.com> wrote:
> >
> >> Paul B Mahol:
> >>> On Sun, Sep 10, 2023 at 1:59 PM Andreas Rheinhardt <
> >>> andreas.rheinhardt@outlook.com> wrote:
> >>>
> >>>> Paul B Mahol:
> >>>>> On Wed, Sep 6, 2023 at 1:30 PM Andreas Rheinhardt <
> >>>>> andreas.rheinhardt@outlook.com> wrote:
> >>>>>
> >>>>>> Paul B Mahol:
> >>>>>>> On Wed, Sep 6, 2023 at 11:26 AM Andreas Rheinhardt <
> >>>>>>> andreas.rheinhardt@outlook.com> wrote:
> >>>>>>>
> >>>>>>>> Paul B Mahol:
> >>>>>>>>>
> >>>>>>>>> + chunk_type = avio_rb32(pb);
> >>>>>>>>> + chunk_size = avio_rb32(pb);
> >>>>>>>>
> >>>>>>>> You are not checking whether the chunk here exceeds its containing
> >>>>>> chunk.
> >>>>>>>>
> >>>>>>>>>
> >>>>>>>>> + av_fast_malloc(&usm->header, &usm->header_size,
> >>>>>>>>> + chunk_size + AV_INPUT_BUFFER_PADDING_SIZE);
> >>>>>>>>> + if (!usm->header)
> >>>>>>>>> + return AVERROR(ENOMEM);
> >>>>>>>>
> >>>>>>>> The bytestream2 API does not rely on the buffer being padded at
> all.
> >>>>>>>>
> >>>>>>>>>
> >>>>>>>>> + bytestream2_skip(&sgb, string_offset);
> >>>>>>>>
> >>>>>>>> This is unnecessary, because you seek with an absolute offset
> >> lateron
> >>>>>>>> anyway before using sgb.
> >>>>>>>>
> >>>>>>>>>
> >>>>>>>>> + bytestream2_seek(&sgb, string_offset + offset,
> SEEK_SET);
> >>>>>>>>> + while (bytestream2_get_bytes_left(&sgb) > 0) {
> >>>>>>>>> + key[n] = bytestream2_get_byte(&sgb);
> >>>>>>>>> + if (!key[n])
> >>>>>>>>> + break;
> >>>>>>>>> + if (n >= sizeof(key) - 1)
> >>>>>>>>> + break;
> >>>>>>>>> + n++;
> >>>>>>>>> + }
> >>>>>>>>> + key[n] = '\0';
> >>>>>>>>
> >>>>>>>> IMO this would be easier with strnlen(), avoiding sgb altogether.
> >>>>>>>> You would of course need to explicitly check that you are not
> >>>>>>>> overreading, but that is good practice anyway.
> >>>>>>>>
> >>>>>>>>>
> >>>>>>>>> + chunk_start = avio_tell(pb);
> >>>>>>>>> + avio_skip(pb, 1);
> >>>>>>>>> + payload_offset = avio_r8(pb);
> >>>>>>>>> + padding_size = avio_rb16(pb);
> >>>>>>>>> + stream_index = avio_r8(pb);
> >>>>>>>>> + avio_skip(pb, 2);
> >>>>>>>>> + payload_type = avio_r8(pb);
> >>>>>>>>> + frame_time = avio_rb32(pb);
> >>>>>>>>> + frame_rate = avio_rb32(pb);
> >>>>>>>>> + avio_skip(pb, 8);
> >>>>>>>>
> >>>>>>>> payload_offset and frame_time are set-but-unused; this might lead
> to
> >>>>>>>> compiler warnings.
> >>>>>>>>
> >>>>>>>>> + if (usm->ch[is_audio][stream_index].used == 1) {
> >>>>>>>>> + uint32_t pkt_size = chunk_size - (avio_tell(pb) -
> >>>>>>>> chunk_start);
> >>>>>>>>> +
> >>>>>>>>
> >>>>>>>> This is unnecessary: Unless we already had a read error, pkt_size
> is
> >>>>>>>> chunk_size - (1 + 1 + 2 + 1 + 2 + 1 + 4 + 4 + 8).
> >>>>>>>>
> >>>>>>>> (Notice that in case padding_size is > 0, it will be part of the
> >>>> packet
> >>>>>>>> with the current code; not sure if that is an issue.)
> >>>>>>>>
> >>>>>>>>>
> >>>>>>>>> +
> >>>>>>>>> + avio_skip(pb, padding_size);
> >>>>>>>>> + avio_skip(pb, chunk_size - (avio_tell(pb) - chunk_start));
> >>>>>>>>> +
> >>>>>>>>
> >>>>>>>> Simpler to just use avio_seek(pb, chunk_start + chunk_size,
> >> SEEK_SET);
> >>>>>>>>
> >>>>>>>
> >>>>>>> But input might not be seekable.
> >>>>>>>
> >>>>>>
> >>>>>> And? You know that avio_skip(pb, offset) is just avio_seek(pb,
> offset,
> >>>>>> SEEK_CUR)?
> >>>>>>
> >>>>>
> >>>>> And? Do you know that SEEK_SET is different from SEEK_CUR with
> positive
> >>>>> argument.
> >>>>>
> >>>>
> >>>> You are using SEEK_CUR with -avio_tell(pb), which effectively makes it
> >>>> an absolute seek.
> >>>>
> >>>
> >>> Nope, I skip data only, not seeking backward ever.
> >>>
> >>
> >> avio_seek() internally translates any avio_seek() with SEEK_CUR to an
> >> absolute seek (as required by the seek callbacks) and does not care
> >> about whether it is seeking forward or backward.
> >>
> >
> > Irrelevant. Seeking needs enough cache to work on non-seekable input.
> >
>
> Given that avio_skip() is just a wrapper around avio_seek(), the
> behaviour will not change even when the input is non-seekable.
>
What you are trying to sell?
That seeking works with unseekable input?
>
> - Andreas
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@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-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avformat: add CRI USM demuxer
2023-09-10 12:29 ` Paul B Mahol
@ 2023-09-10 12:55 ` Andreas Rheinhardt
2023-09-10 13:16 ` Paul B Mahol
0 siblings, 1 reply; 17+ messages in thread
From: Andreas Rheinhardt @ 2023-09-10 12:55 UTC (permalink / raw)
To: ffmpeg-devel
Paul B Mahol:
> On Sun, Sep 10, 2023 at 2:18 PM Andreas Rheinhardt <
> andreas.rheinhardt@outlook.com> wrote:
>
>> Paul B Mahol:
>>> On Sun, Sep 10, 2023 at 2:06 PM Andreas Rheinhardt <
>>> andreas.rheinhardt@outlook.com> wrote:
>>>
>>>> Paul B Mahol:
>>>>> On Sun, Sep 10, 2023 at 1:59 PM Andreas Rheinhardt <
>>>>> andreas.rheinhardt@outlook.com> wrote:
>>>>>
>>>>>> Paul B Mahol:
>>>>>>> On Wed, Sep 6, 2023 at 1:30 PM Andreas Rheinhardt <
>>>>>>> andreas.rheinhardt@outlook.com> wrote:
>>>>>>>
>>>>>>>> Paul B Mahol:
>>>>>>>>> On Wed, Sep 6, 2023 at 11:26 AM Andreas Rheinhardt <
>>>>>>>>> andreas.rheinhardt@outlook.com> wrote:
>>>>>>>>>
>>>>>>>>>> Paul B Mahol:
>>>>>>>>>>>
>>>>>>>>>>> + chunk_type = avio_rb32(pb);
>>>>>>>>>>> + chunk_size = avio_rb32(pb);
>>>>>>>>>>
>>>>>>>>>> You are not checking whether the chunk here exceeds its containing
>>>>>>>> chunk.
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> + av_fast_malloc(&usm->header, &usm->header_size,
>>>>>>>>>>> + chunk_size + AV_INPUT_BUFFER_PADDING_SIZE);
>>>>>>>>>>> + if (!usm->header)
>>>>>>>>>>> + return AVERROR(ENOMEM);
>>>>>>>>>>
>>>>>>>>>> The bytestream2 API does not rely on the buffer being padded at
>> all.
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> + bytestream2_skip(&sgb, string_offset);
>>>>>>>>>>
>>>>>>>>>> This is unnecessary, because you seek with an absolute offset
>>>> lateron
>>>>>>>>>> anyway before using sgb.
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> + bytestream2_seek(&sgb, string_offset + offset,
>> SEEK_SET);
>>>>>>>>>>> + while (bytestream2_get_bytes_left(&sgb) > 0) {
>>>>>>>>>>> + key[n] = bytestream2_get_byte(&sgb);
>>>>>>>>>>> + if (!key[n])
>>>>>>>>>>> + break;
>>>>>>>>>>> + if (n >= sizeof(key) - 1)
>>>>>>>>>>> + break;
>>>>>>>>>>> + n++;
>>>>>>>>>>> + }
>>>>>>>>>>> + key[n] = '\0';
>>>>>>>>>>
>>>>>>>>>> IMO this would be easier with strnlen(), avoiding sgb altogether.
>>>>>>>>>> You would of course need to explicitly check that you are not
>>>>>>>>>> overreading, but that is good practice anyway.
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> + chunk_start = avio_tell(pb);
>>>>>>>>>>> + avio_skip(pb, 1);
>>>>>>>>>>> + payload_offset = avio_r8(pb);
>>>>>>>>>>> + padding_size = avio_rb16(pb);
>>>>>>>>>>> + stream_index = avio_r8(pb);
>>>>>>>>>>> + avio_skip(pb, 2);
>>>>>>>>>>> + payload_type = avio_r8(pb);
>>>>>>>>>>> + frame_time = avio_rb32(pb);
>>>>>>>>>>> + frame_rate = avio_rb32(pb);
>>>>>>>>>>> + avio_skip(pb, 8);
>>>>>>>>>>
>>>>>>>>>> payload_offset and frame_time are set-but-unused; this might lead
>> to
>>>>>>>>>> compiler warnings.
>>>>>>>>>>
>>>>>>>>>>> + if (usm->ch[is_audio][stream_index].used == 1) {
>>>>>>>>>>> + uint32_t pkt_size = chunk_size - (avio_tell(pb) -
>>>>>>>>>> chunk_start);
>>>>>>>>>>> +
>>>>>>>>>>
>>>>>>>>>> This is unnecessary: Unless we already had a read error, pkt_size
>> is
>>>>>>>>>> chunk_size - (1 + 1 + 2 + 1 + 2 + 1 + 4 + 4 + 8).
>>>>>>>>>>
>>>>>>>>>> (Notice that in case padding_size is > 0, it will be part of the
>>>>>> packet
>>>>>>>>>> with the current code; not sure if that is an issue.)
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> +
>>>>>>>>>>> + avio_skip(pb, padding_size);
>>>>>>>>>>> + avio_skip(pb, chunk_size - (avio_tell(pb) - chunk_start));
>>>>>>>>>>> +
>>>>>>>>>>
>>>>>>>>>> Simpler to just use avio_seek(pb, chunk_start + chunk_size,
>>>> SEEK_SET);
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> But input might not be seekable.
>>>>>>>>>
>>>>>>>>
>>>>>>>> And? You know that avio_skip(pb, offset) is just avio_seek(pb,
>> offset,
>>>>>>>> SEEK_CUR)?
>>>>>>>>
>>>>>>>
>>>>>>> And? Do you know that SEEK_SET is different from SEEK_CUR with
>> positive
>>>>>>> argument.
>>>>>>>
>>>>>>
>>>>>> You are using SEEK_CUR with -avio_tell(pb), which effectively makes it
>>>>>> an absolute seek.
>>>>>>
>>>>>
>>>>> Nope, I skip data only, not seeking backward ever.
>>>>>
>>>>
>>>> avio_seek() internally translates any avio_seek() with SEEK_CUR to an
>>>> absolute seek (as required by the seek callbacks) and does not care
>>>> about whether it is seeking forward or backward.
>>>>
>>>
>>> Irrelevant. Seeking needs enough cache to work on non-seekable input.
>>>
>>
>> Given that avio_skip() is just a wrapper around avio_seek(), the
>> behaviour will not change even when the input is non-seekable.
>>
>
> What you are trying to sell?
>
> That seeking works with unseekable input?
>
Seeking forward works for unseekable input by reading data and
discarding it (that is how avio_seek() works). But actually we don't
need that, all we need is that avio_skip() is just a wrapper around
avio_seek(), so that the two forms are equivalent.
- Andreas
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avformat: add CRI USM demuxer
2023-09-10 13:16 ` Paul B Mahol
@ 2023-09-10 13:14 ` Andreas Rheinhardt
2023-09-10 13:24 ` Paul B Mahol
0 siblings, 1 reply; 17+ messages in thread
From: Andreas Rheinhardt @ 2023-09-10 13:14 UTC (permalink / raw)
To: ffmpeg-devel
Paul B Mahol:
> On Sun, Sep 10, 2023 at 2:54 PM Andreas Rheinhardt <
> andreas.rheinhardt@outlook.com> wrote:
>
>> Paul B Mahol:
>>> On Sun, Sep 10, 2023 at 2:18 PM Andreas Rheinhardt <
>>> andreas.rheinhardt@outlook.com> wrote:
>>>
>>>> Paul B Mahol:
>>>>> On Sun, Sep 10, 2023 at 2:06 PM Andreas Rheinhardt <
>>>>> andreas.rheinhardt@outlook.com> wrote:
>>>>>
>>>>>> Paul B Mahol:
>>>>>>> On Sun, Sep 10, 2023 at 1:59 PM Andreas Rheinhardt <
>>>>>>> andreas.rheinhardt@outlook.com> wrote:
>>>>>>>
>>>>>>>> Paul B Mahol:
>>>>>>>>> On Wed, Sep 6, 2023 at 1:30 PM Andreas Rheinhardt <
>>>>>>>>> andreas.rheinhardt@outlook.com> wrote:
>>>>>>>>>
>>>>>>>>>> Paul B Mahol:
>>>>>>>>>>> On Wed, Sep 6, 2023 at 11:26 AM Andreas Rheinhardt <
>>>>>>>>>>> andreas.rheinhardt@outlook.com> wrote:
>>>>>>>>>>>
>>>>>>>>>>>> Paul B Mahol:
>>>>>>>>>>>>>
>>>>>>>>>>>>> + chunk_type = avio_rb32(pb);
>>>>>>>>>>>>> + chunk_size = avio_rb32(pb);
>>>>>>>>>>>>
>>>>>>>>>>>> You are not checking whether the chunk here exceeds its
>> containing
>>>>>>>>>> chunk.
>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> + av_fast_malloc(&usm->header, &usm->header_size,
>>>>>>>>>>>>> + chunk_size + AV_INPUT_BUFFER_PADDING_SIZE);
>>>>>>>>>>>>> + if (!usm->header)
>>>>>>>>>>>>> + return AVERROR(ENOMEM);
>>>>>>>>>>>>
>>>>>>>>>>>> The bytestream2 API does not rely on the buffer being padded at
>>>> all.
>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> + bytestream2_skip(&sgb, string_offset);
>>>>>>>>>>>>
>>>>>>>>>>>> This is unnecessary, because you seek with an absolute offset
>>>>>> lateron
>>>>>>>>>>>> anyway before using sgb.
>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> + bytestream2_seek(&sgb, string_offset + offset,
>>>> SEEK_SET);
>>>>>>>>>>>>> + while (bytestream2_get_bytes_left(&sgb) > 0) {
>>>>>>>>>>>>> + key[n] = bytestream2_get_byte(&sgb);
>>>>>>>>>>>>> + if (!key[n])
>>>>>>>>>>>>> + break;
>>>>>>>>>>>>> + if (n >= sizeof(key) - 1)
>>>>>>>>>>>>> + break;
>>>>>>>>>>>>> + n++;
>>>>>>>>>>>>> + }
>>>>>>>>>>>>> + key[n] = '\0';
>>>>>>>>>>>>
>>>>>>>>>>>> IMO this would be easier with strnlen(), avoiding sgb
>> altogether.
>>>>>>>>>>>> You would of course need to explicitly check that you are not
>>>>>>>>>>>> overreading, but that is good practice anyway.
>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> + chunk_start = avio_tell(pb);
>>>>>>>>>>>>> + avio_skip(pb, 1);
>>>>>>>>>>>>> + payload_offset = avio_r8(pb);
>>>>>>>>>>>>> + padding_size = avio_rb16(pb);
>>>>>>>>>>>>> + stream_index = avio_r8(pb);
>>>>>>>>>>>>> + avio_skip(pb, 2);
>>>>>>>>>>>>> + payload_type = avio_r8(pb);
>>>>>>>>>>>>> + frame_time = avio_rb32(pb);
>>>>>>>>>>>>> + frame_rate = avio_rb32(pb);
>>>>>>>>>>>>> + avio_skip(pb, 8);
>>>>>>>>>>>>
>>>>>>>>>>>> payload_offset and frame_time are set-but-unused; this might
>> lead
>>>> to
>>>>>>>>>>>> compiler warnings.
>>>>>>>>>>>>
>>>>>>>>>>>>> + if (usm->ch[is_audio][stream_index].used == 1) {
>>>>>>>>>>>>> + uint32_t pkt_size = chunk_size - (avio_tell(pb) -
>>>>>>>>>>>> chunk_start);
>>>>>>>>>>>>> +
>>>>>>>>>>>>
>>>>>>>>>>>> This is unnecessary: Unless we already had a read error,
>> pkt_size
>>>> is
>>>>>>>>>>>> chunk_size - (1 + 1 + 2 + 1 + 2 + 1 + 4 + 4 + 8).
>>>>>>>>>>>>
>>>>>>>>>>>> (Notice that in case padding_size is > 0, it will be part of the
>>>>>>>> packet
>>>>>>>>>>>> with the current code; not sure if that is an issue.)
>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> +
>>>>>>>>>>>>> + avio_skip(pb, padding_size);
>>>>>>>>>>>>> + avio_skip(pb, chunk_size - (avio_tell(pb) - chunk_start));
>>>>>>>>>>>>> +
>>>>>>>>>>>>
>>>>>>>>>>>> Simpler to just use avio_seek(pb, chunk_start + chunk_size,
>>>>>> SEEK_SET);
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> But input might not be seekable.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> And? You know that avio_skip(pb, offset) is just avio_seek(pb,
>>>> offset,
>>>>>>>>>> SEEK_CUR)?
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> And? Do you know that SEEK_SET is different from SEEK_CUR with
>>>> positive
>>>>>>>>> argument.
>>>>>>>>>
>>>>>>>>
>>>>>>>> You are using SEEK_CUR with -avio_tell(pb), which effectively makes
>> it
>>>>>>>> an absolute seek.
>>>>>>>>
>>>>>>>
>>>>>>> Nope, I skip data only, not seeking backward ever.
>>>>>>>
>>>>>>
>>>>>> avio_seek() internally translates any avio_seek() with SEEK_CUR to an
>>>>>> absolute seek (as required by the seek callbacks) and does not care
>>>>>> about whether it is seeking forward or backward.
>>>>>>
>>>>>
>>>>> Irrelevant. Seeking needs enough cache to work on non-seekable input.
>>>>>
>>>>
>>>> Given that avio_skip() is just a wrapper around avio_seek(), the
>>>> behaviour will not change even when the input is non-seekable.
>>>>
>>>
>>> What you are trying to sell?
>>>
>>> That seeking works with unseekable input?
>>>
>>
>> Seeking forward works for unseekable input by reading data and
>> discarding it (that is how avio_seek() works). But actually we don't
>> need that, all we need is that avio_skip() is just a wrapper around
>> avio_seek(), so that the two forms are equivalent.
>>
>
> Patches welcome, just do not break unseekable demuxing.
>
? You want me to send a separate patch avoiding the unnecessary
avio_seek() and switching to avio_seek() from avio_skip()? Why not apply
it now?
- Andreas
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avformat: add CRI USM demuxer
2023-09-10 12:55 ` Andreas Rheinhardt
@ 2023-09-10 13:16 ` Paul B Mahol
2023-09-10 13:14 ` Andreas Rheinhardt
0 siblings, 1 reply; 17+ messages in thread
From: Paul B Mahol @ 2023-09-10 13:16 UTC (permalink / raw)
To: FFmpeg development discussions and patches
On Sun, Sep 10, 2023 at 2:54 PM Andreas Rheinhardt <
andreas.rheinhardt@outlook.com> wrote:
> Paul B Mahol:
> > On Sun, Sep 10, 2023 at 2:18 PM Andreas Rheinhardt <
> > andreas.rheinhardt@outlook.com> wrote:
> >
> >> Paul B Mahol:
> >>> On Sun, Sep 10, 2023 at 2:06 PM Andreas Rheinhardt <
> >>> andreas.rheinhardt@outlook.com> wrote:
> >>>
> >>>> Paul B Mahol:
> >>>>> On Sun, Sep 10, 2023 at 1:59 PM Andreas Rheinhardt <
> >>>>> andreas.rheinhardt@outlook.com> wrote:
> >>>>>
> >>>>>> Paul B Mahol:
> >>>>>>> On Wed, Sep 6, 2023 at 1:30 PM Andreas Rheinhardt <
> >>>>>>> andreas.rheinhardt@outlook.com> wrote:
> >>>>>>>
> >>>>>>>> Paul B Mahol:
> >>>>>>>>> On Wed, Sep 6, 2023 at 11:26 AM Andreas Rheinhardt <
> >>>>>>>>> andreas.rheinhardt@outlook.com> wrote:
> >>>>>>>>>
> >>>>>>>>>> Paul B Mahol:
> >>>>>>>>>>>
> >>>>>>>>>>> + chunk_type = avio_rb32(pb);
> >>>>>>>>>>> + chunk_size = avio_rb32(pb);
> >>>>>>>>>>
> >>>>>>>>>> You are not checking whether the chunk here exceeds its
> containing
> >>>>>>>> chunk.
> >>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> + av_fast_malloc(&usm->header, &usm->header_size,
> >>>>>>>>>>> + chunk_size + AV_INPUT_BUFFER_PADDING_SIZE);
> >>>>>>>>>>> + if (!usm->header)
> >>>>>>>>>>> + return AVERROR(ENOMEM);
> >>>>>>>>>>
> >>>>>>>>>> The bytestream2 API does not rely on the buffer being padded at
> >> all.
> >>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> + bytestream2_skip(&sgb, string_offset);
> >>>>>>>>>>
> >>>>>>>>>> This is unnecessary, because you seek with an absolute offset
> >>>> lateron
> >>>>>>>>>> anyway before using sgb.
> >>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> + bytestream2_seek(&sgb, string_offset + offset,
> >> SEEK_SET);
> >>>>>>>>>>> + while (bytestream2_get_bytes_left(&sgb) > 0) {
> >>>>>>>>>>> + key[n] = bytestream2_get_byte(&sgb);
> >>>>>>>>>>> + if (!key[n])
> >>>>>>>>>>> + break;
> >>>>>>>>>>> + if (n >= sizeof(key) - 1)
> >>>>>>>>>>> + break;
> >>>>>>>>>>> + n++;
> >>>>>>>>>>> + }
> >>>>>>>>>>> + key[n] = '\0';
> >>>>>>>>>>
> >>>>>>>>>> IMO this would be easier with strnlen(), avoiding sgb
> altogether.
> >>>>>>>>>> You would of course need to explicitly check that you are not
> >>>>>>>>>> overreading, but that is good practice anyway.
> >>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> + chunk_start = avio_tell(pb);
> >>>>>>>>>>> + avio_skip(pb, 1);
> >>>>>>>>>>> + payload_offset = avio_r8(pb);
> >>>>>>>>>>> + padding_size = avio_rb16(pb);
> >>>>>>>>>>> + stream_index = avio_r8(pb);
> >>>>>>>>>>> + avio_skip(pb, 2);
> >>>>>>>>>>> + payload_type = avio_r8(pb);
> >>>>>>>>>>> + frame_time = avio_rb32(pb);
> >>>>>>>>>>> + frame_rate = avio_rb32(pb);
> >>>>>>>>>>> + avio_skip(pb, 8);
> >>>>>>>>>>
> >>>>>>>>>> payload_offset and frame_time are set-but-unused; this might
> lead
> >> to
> >>>>>>>>>> compiler warnings.
> >>>>>>>>>>
> >>>>>>>>>>> + if (usm->ch[is_audio][stream_index].used == 1) {
> >>>>>>>>>>> + uint32_t pkt_size = chunk_size - (avio_tell(pb) -
> >>>>>>>>>> chunk_start);
> >>>>>>>>>>> +
> >>>>>>>>>>
> >>>>>>>>>> This is unnecessary: Unless we already had a read error,
> pkt_size
> >> is
> >>>>>>>>>> chunk_size - (1 + 1 + 2 + 1 + 2 + 1 + 4 + 4 + 8).
> >>>>>>>>>>
> >>>>>>>>>> (Notice that in case padding_size is > 0, it will be part of the
> >>>>>> packet
> >>>>>>>>>> with the current code; not sure if that is an issue.)
> >>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> +
> >>>>>>>>>>> + avio_skip(pb, padding_size);
> >>>>>>>>>>> + avio_skip(pb, chunk_size - (avio_tell(pb) - chunk_start));
> >>>>>>>>>>> +
> >>>>>>>>>>
> >>>>>>>>>> Simpler to just use avio_seek(pb, chunk_start + chunk_size,
> >>>> SEEK_SET);
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> But input might not be seekable.
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> And? You know that avio_skip(pb, offset) is just avio_seek(pb,
> >> offset,
> >>>>>>>> SEEK_CUR)?
> >>>>>>>>
> >>>>>>>
> >>>>>>> And? Do you know that SEEK_SET is different from SEEK_CUR with
> >> positive
> >>>>>>> argument.
> >>>>>>>
> >>>>>>
> >>>>>> You are using SEEK_CUR with -avio_tell(pb), which effectively makes
> it
> >>>>>> an absolute seek.
> >>>>>>
> >>>>>
> >>>>> Nope, I skip data only, not seeking backward ever.
> >>>>>
> >>>>
> >>>> avio_seek() internally translates any avio_seek() with SEEK_CUR to an
> >>>> absolute seek (as required by the seek callbacks) and does not care
> >>>> about whether it is seeking forward or backward.
> >>>>
> >>>
> >>> Irrelevant. Seeking needs enough cache to work on non-seekable input.
> >>>
> >>
> >> Given that avio_skip() is just a wrapper around avio_seek(), the
> >> behaviour will not change even when the input is non-seekable.
> >>
> >
> > What you are trying to sell?
> >
> > That seeking works with unseekable input?
> >
>
> Seeking forward works for unseekable input by reading data and
> discarding it (that is how avio_seek() works). But actually we don't
> need that, all we need is that avio_skip() is just a wrapper around
> avio_seek(), so that the two forms are equivalent.
>
Patches welcome, just do not break unseekable demuxing.
>
> - Andreas
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@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-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avformat: add CRI USM demuxer
2023-09-10 13:14 ` Andreas Rheinhardt
@ 2023-09-10 13:24 ` Paul B Mahol
0 siblings, 0 replies; 17+ messages in thread
From: Paul B Mahol @ 2023-09-10 13:24 UTC (permalink / raw)
To: FFmpeg development discussions and patches
On Sun, Sep 10, 2023 at 3:13 PM Andreas Rheinhardt <
andreas.rheinhardt@outlook.com> wrote:
> Paul B Mahol:
> > On Sun, Sep 10, 2023 at 2:54 PM Andreas Rheinhardt <
> > andreas.rheinhardt@outlook.com> wrote:
> >
> >> Paul B Mahol:
> >>> On Sun, Sep 10, 2023 at 2:18 PM Andreas Rheinhardt <
> >>> andreas.rheinhardt@outlook.com> wrote:
> >>>
> >>>> Paul B Mahol:
> >>>>> On Sun, Sep 10, 2023 at 2:06 PM Andreas Rheinhardt <
> >>>>> andreas.rheinhardt@outlook.com> wrote:
> >>>>>
> >>>>>> Paul B Mahol:
> >>>>>>> On Sun, Sep 10, 2023 at 1:59 PM Andreas Rheinhardt <
> >>>>>>> andreas.rheinhardt@outlook.com> wrote:
> >>>>>>>
> >>>>>>>> Paul B Mahol:
> >>>>>>>>> On Wed, Sep 6, 2023 at 1:30 PM Andreas Rheinhardt <
> >>>>>>>>> andreas.rheinhardt@outlook.com> wrote:
> >>>>>>>>>
> >>>>>>>>>> Paul B Mahol:
> >>>>>>>>>>> On Wed, Sep 6, 2023 at 11:26 AM Andreas Rheinhardt <
> >>>>>>>>>>> andreas.rheinhardt@outlook.com> wrote:
> >>>>>>>>>>>
> >>>>>>>>>>>> Paul B Mahol:
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> + chunk_type = avio_rb32(pb);
> >>>>>>>>>>>>> + chunk_size = avio_rb32(pb);
> >>>>>>>>>>>>
> >>>>>>>>>>>> You are not checking whether the chunk here exceeds its
> >> containing
> >>>>>>>>>> chunk.
> >>>>>>>>>>>>
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> + av_fast_malloc(&usm->header, &usm->header_size,
> >>>>>>>>>>>>> + chunk_size +
> AV_INPUT_BUFFER_PADDING_SIZE);
> >>>>>>>>>>>>> + if (!usm->header)
> >>>>>>>>>>>>> + return AVERROR(ENOMEM);
> >>>>>>>>>>>>
> >>>>>>>>>>>> The bytestream2 API does not rely on the buffer being padded
> at
> >>>> all.
> >>>>>>>>>>>>
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> + bytestream2_skip(&sgb, string_offset);
> >>>>>>>>>>>>
> >>>>>>>>>>>> This is unnecessary, because you seek with an absolute offset
> >>>>>> lateron
> >>>>>>>>>>>> anyway before using sgb.
> >>>>>>>>>>>>
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> + bytestream2_seek(&sgb, string_offset + offset,
> >>>> SEEK_SET);
> >>>>>>>>>>>>> + while (bytestream2_get_bytes_left(&sgb) > 0) {
> >>>>>>>>>>>>> + key[n] = bytestream2_get_byte(&sgb);
> >>>>>>>>>>>>> + if (!key[n])
> >>>>>>>>>>>>> + break;
> >>>>>>>>>>>>> + if (n >= sizeof(key) - 1)
> >>>>>>>>>>>>> + break;
> >>>>>>>>>>>>> + n++;
> >>>>>>>>>>>>> + }
> >>>>>>>>>>>>> + key[n] = '\0';
> >>>>>>>>>>>>
> >>>>>>>>>>>> IMO this would be easier with strnlen(), avoiding sgb
> >> altogether.
> >>>>>>>>>>>> You would of course need to explicitly check that you are not
> >>>>>>>>>>>> overreading, but that is good practice anyway.
> >>>>>>>>>>>>
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> + chunk_start = avio_tell(pb);
> >>>>>>>>>>>>> + avio_skip(pb, 1);
> >>>>>>>>>>>>> + payload_offset = avio_r8(pb);
> >>>>>>>>>>>>> + padding_size = avio_rb16(pb);
> >>>>>>>>>>>>> + stream_index = avio_r8(pb);
> >>>>>>>>>>>>> + avio_skip(pb, 2);
> >>>>>>>>>>>>> + payload_type = avio_r8(pb);
> >>>>>>>>>>>>> + frame_time = avio_rb32(pb);
> >>>>>>>>>>>>> + frame_rate = avio_rb32(pb);
> >>>>>>>>>>>>> + avio_skip(pb, 8);
> >>>>>>>>>>>>
> >>>>>>>>>>>> payload_offset and frame_time are set-but-unused; this might
> >> lead
> >>>> to
> >>>>>>>>>>>> compiler warnings.
> >>>>>>>>>>>>
> >>>>>>>>>>>>> + if (usm->ch[is_audio][stream_index].used == 1) {
> >>>>>>>>>>>>> + uint32_t pkt_size = chunk_size - (avio_tell(pb)
> -
> >>>>>>>>>>>> chunk_start);
> >>>>>>>>>>>>> +
> >>>>>>>>>>>>
> >>>>>>>>>>>> This is unnecessary: Unless we already had a read error,
> >> pkt_size
> >>>> is
> >>>>>>>>>>>> chunk_size - (1 + 1 + 2 + 1 + 2 + 1 + 4 + 4 + 8).
> >>>>>>>>>>>>
> >>>>>>>>>>>> (Notice that in case padding_size is > 0, it will be part of
> the
> >>>>>>>> packet
> >>>>>>>>>>>> with the current code; not sure if that is an issue.)
> >>>>>>>>>>>>
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> +
> >>>>>>>>>>>>> + avio_skip(pb, padding_size);
> >>>>>>>>>>>>> + avio_skip(pb, chunk_size - (avio_tell(pb) -
> chunk_start));
> >>>>>>>>>>>>> +
> >>>>>>>>>>>>
> >>>>>>>>>>>> Simpler to just use avio_seek(pb, chunk_start + chunk_size,
> >>>>>> SEEK_SET);
> >>>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> But input might not be seekable.
> >>>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> And? You know that avio_skip(pb, offset) is just avio_seek(pb,
> >>>> offset,
> >>>>>>>>>> SEEK_CUR)?
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> And? Do you know that SEEK_SET is different from SEEK_CUR with
> >>>> positive
> >>>>>>>>> argument.
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> You are using SEEK_CUR with -avio_tell(pb), which effectively
> makes
> >> it
> >>>>>>>> an absolute seek.
> >>>>>>>>
> >>>>>>>
> >>>>>>> Nope, I skip data only, not seeking backward ever.
> >>>>>>>
> >>>>>>
> >>>>>> avio_seek() internally translates any avio_seek() with SEEK_CUR to
> an
> >>>>>> absolute seek (as required by the seek callbacks) and does not care
> >>>>>> about whether it is seeking forward or backward.
> >>>>>>
> >>>>>
> >>>>> Irrelevant. Seeking needs enough cache to work on non-seekable input.
> >>>>>
> >>>>
> >>>> Given that avio_skip() is just a wrapper around avio_seek(), the
> >>>> behaviour will not change even when the input is non-seekable.
> >>>>
> >>>
> >>> What you are trying to sell?
> >>>
> >>> That seeking works with unseekable input?
> >>>
> >>
> >> Seeking forward works for unseekable input by reading data and
> >> discarding it (that is how avio_seek() works). But actually we don't
> >> need that, all we need is that avio_skip() is just a wrapper around
> >> avio_seek(), so that the two forms are equivalent.
> >>
> >
> > Patches welcome, just do not break unseekable demuxing.
> >
>
> ? You want me to send a separate patch avoiding the unnecessary
> avio_seek() and switching to avio_seek() from avio_skip()? Why not apply
> it now?
>
I prefer avio_skip() solution.
And would prefer to stay that way,
It is irrelevant how underlying code call is implemented. Otherwise I'm not
code nazi.
>
> - Andreas
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@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-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avformat: add CRI USM demuxer
2023-09-10 12:02 ` Paul B Mahol
@ 2023-09-16 12:19 ` Paul B Mahol
0 siblings, 0 replies; 17+ messages in thread
From: Paul B Mahol @ 2023-09-16 12:19 UTC (permalink / raw)
To: FFmpeg development discussions and patches
Will push soon.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2023-09-16 12:12 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-09-05 21:37 [FFmpeg-devel] [PATCH] avformat: add CRI USM demuxer Paul B Mahol
2023-09-06 9:27 ` Andreas Rheinhardt
2023-09-06 9:57 ` Paul B Mahol
2023-09-06 11:31 ` Andreas Rheinhardt
2023-09-06 12:01 ` Paul B Mahol
2023-09-10 12:00 ` Andreas Rheinhardt
2023-09-10 12:10 ` Paul B Mahol
2023-09-10 12:07 ` Andreas Rheinhardt
2023-09-10 12:15 ` Paul B Mahol
2023-09-10 12:19 ` Andreas Rheinhardt
2023-09-10 12:29 ` Paul B Mahol
2023-09-10 12:55 ` Andreas Rheinhardt
2023-09-10 13:16 ` Paul B Mahol
2023-09-10 13:14 ` Andreas Rheinhardt
2023-09-10 13:24 ` Paul B Mahol
2023-09-10 12:02 ` Paul B Mahol
2023-09-16 12:19 ` Paul B Mahol
Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
This inbox may be cloned and mirrored by anyone:
git clone --mirror https://master.gitmailbox.com/ffmpegdev/0 ffmpegdev/git/0.git
# If you have public-inbox 1.1+ installed, you may
# initialize and index your mirror using the following commands:
public-inbox-init -V2 ffmpegdev ffmpegdev/ https://master.gitmailbox.com/ffmpegdev \
ffmpegdev@gitmailbox.com
public-inbox-index ffmpegdev
Example config snippet for mirrors.
AGPL code for this site: git clone https://public-inbox.org/public-inbox.git