| Message ID | 20191001170512.13762-1-rzumer@tebako.net |
|---|---|
| State | New |
| Headers | show |
On 10/1/2019 2:05 PM, Raphaël Zumer wrote: > Signed-off-by: Raphaël Zumer <rzumer@tebako.net> > --- > libavformat/ivfdec.c | 3 ++- > libavformat/ivfenc.c | 5 +++-- > 2 files changed, 5 insertions(+), 3 deletions(-) > > diff --git a/libavformat/ivfdec.c b/libavformat/ivfdec.c > index 40ae464b76..2fdb6f5a04 100644 > --- a/libavformat/ivfdec.c > +++ b/libavformat/ivfdec.c > @@ -53,7 +53,8 @@ static int read_header(AVFormatContext *s) > st->codecpar->height = avio_rl16(s->pb); > time_base.den = avio_rl32(s->pb); > time_base.num = avio_rl32(s->pb); > - st->duration = avio_rl64(s->pb); > + st->duration = avio_rl32(s->pb); > + avio_rl32(s->pb); // unused avio_skip(s->pb, 4); This part is good either way. > > st->need_parsing = AVSTREAM_PARSE_HEADERS; > > diff --git a/libavformat/ivfenc.c b/libavformat/ivfenc.c > index adf72117e9..e135a78213 100644 > --- a/libavformat/ivfenc.c > +++ b/libavformat/ivfenc.c > @@ -53,7 +53,7 @@ static int ivf_write_header(AVFormatContext *s) > avio_wl16(pb, par->height); > avio_wl32(pb, s->streams[0]->time_base.den); > avio_wl32(pb, s->streams[0]->time_base.num); > - avio_wl64(pb, 0xFFFFFFFFFFFFFFFFULL); > + avio_wl64(pb, 0xFFFFFFFFFFFFFFFFULL); // length is overwritten at the end of muxing > > return 0; > } > @@ -83,7 +83,8 @@ static int ivf_write_trailer(AVFormatContext *s) > size_t end = avio_tell(pb); > > avio_seek(pb, 24, SEEK_SET); > - avio_wl64(pb, ctx->frame_cnt * ctx->sum_delta_pts / (ctx->frame_cnt - 1)); > + // overwrite the "length" field (duration) > + avio_wl32(pb, ctx->frame_cnt * ctx->sum_delta_pts / (ctx->frame_cnt - 1)); The value in the unused field will be 0xFFFFFFFF after this change instead of 0, since you're writing 32 bits as duration instead of 64 where the high 32 bits (corresponding to the unused field) are zeroed. That means the ivf demuxer prior to this patch will read bogus duration values from ivf files created after this patch. Just leave the muxer as is. > avio_seek(pb, end, SEEK_SET); > } > >
Thank you for the review. I have left the encoded value as 64 bits and split the patch into two in the v2 just sent: one for the decoder change in field size, and one for the encoder comments. On Tue, 2019-10-01 at 14:25 -0300, James Almer wrote: > On 10/1/2019 2:05 PM, Raphaël Zumer wrote: > > Signed-off-by: Raphaël Zumer <rzumer@tebako.net> > > --- > > libavformat/ivfdec.c | 3 ++- > > libavformat/ivfenc.c | 5 +++-- > > 2 files changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/libavformat/ivfdec.c b/libavformat/ivfdec.c > > index 40ae464b76..2fdb6f5a04 100644 > > --- a/libavformat/ivfdec.c > > +++ b/libavformat/ivfdec.c > > @@ -53,7 +53,8 @@ static int read_header(AVFormatContext *s) > > st->codecpar->height = avio_rl16(s->pb); > > time_base.den = avio_rl32(s->pb); > > time_base.num = avio_rl32(s->pb); > > - st->duration = avio_rl64(s->pb); > > + st->duration = avio_rl32(s->pb); > > + avio_rl32(s->pb); // unused > > avio_skip(s->pb, 4); > > This part is good either way. > > > > > st->need_parsing = AVSTREAM_PARSE_HEADERS; > > > > diff --git a/libavformat/ivfenc.c b/libavformat/ivfenc.c > > index adf72117e9..e135a78213 100644 > > --- a/libavformat/ivfenc.c > > +++ b/libavformat/ivfenc.c > > @@ -53,7 +53,7 @@ static int ivf_write_header(AVFormatContext *s) > > avio_wl16(pb, par->height); > > avio_wl32(pb, s->streams[0]->time_base.den); > > avio_wl32(pb, s->streams[0]->time_base.num); > > - avio_wl64(pb, 0xFFFFFFFFFFFFFFFFULL); > > + avio_wl64(pb, 0xFFFFFFFFFFFFFFFFULL); // length is overwritten > > at the end of muxing > > > > return 0; > > } > > @@ -83,7 +83,8 @@ static int ivf_write_trailer(AVFormatContext *s) > > size_t end = avio_tell(pb); > > > > avio_seek(pb, 24, SEEK_SET); > > - avio_wl64(pb, ctx->frame_cnt * ctx->sum_delta_pts / (ctx- > > >frame_cnt - 1)); > > + // overwrite the "length" field (duration) > > + avio_wl32(pb, ctx->frame_cnt * ctx->sum_delta_pts / (ctx- > > >frame_cnt - 1)); > > The value in the unused field will be 0xFFFFFFFF after this change > instead of 0, since you're writing 32 bits as duration instead of 64 > where the high 32 bits (corresponding to the unused field) are > zeroed. > That means the ivf demuxer prior to this patch will read bogus > duration > values from ivf files created after this patch. > > Just leave the muxer as is. > > > avio_seek(pb, end, SEEK_SET); > > } > > > > > > _______________________________________________ > 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".
On 01/10/2019 18:25, James Almer wrote: > The value in the unused field will be 0xFFFFFFFF after this change > instead of 0, since you're writing 32 bits as duration instead of 64 > where the high 32 bits (corresponding to the unused field) are zeroed. > That means the ivf demuxer prior to this patch will read bogus duration > values from ivf files created after this patch. > > Just leave the muxer as is. Why not just write zero? It's, to me, worse to leave a bogus 64-bit write to appease bugs in our own demuxer. It's confusing and misleading for any readers of the code. - Derek
On Tue, 2019-10-01 at 20:09 +0100, Derek Buitenhuis wrote: > On 01/10/2019 18:25, James Almer wrote: > > The value in the unused field will be 0xFFFFFFFF after this change > > instead of 0, since you're writing 32 bits as duration instead of > > 64 > > where the high 32 bits (corresponding to the unused field) are > > zeroed. > > That means the ivf demuxer prior to this patch will read bogus > > duration > > values from ivf files created after this patch. > > > > Just leave the muxer as is. > > Why not just write zero? > > It's, to me, worse to leave a bogus 64-bit write to appease bugs in > our > own demuxer. It's confusing and misleading for any readers of the > code. In that case I would prefer changing the initial written value to 0 rather than 0xFFFFFFFFFFFFFFFFULL. Writing over the unused bytes twice to get around an old error is a bit odd as well.
Am Di., 1. Okt. 2019 um 21:35 Uhr schrieb Raphaël Zumer <rzumer@tebako.net>: > > On Tue, 2019-10-01 at 20:09 +0100, Derek Buitenhuis wrote: > > On 01/10/2019 18:25, James Almer wrote: > > > The value in the unused field will be 0xFFFFFFFF after this change > > > instead of 0, since you're writing 32 bits as duration instead of > > > 64 > > > where the high 32 bits (corresponding to the unused field) are > > > zeroed. > > > That means the ivf demuxer prior to this patch will read bogus > > > duration > > > values from ivf files created after this patch. > > > > > > Just leave the muxer as is. > > > > Why not just write zero? > > > > It's, to me, worse to leave a bogus 64-bit write to appease bugs in > > our > > own demuxer. It's confusing and misleading for any readers of the > > code. > > In that case I would prefer changing the initial written value to 0 > rather than 0xFFFFFFFFFFFFFFFFULL. Writing over the unused bytes twice > to get around an old error is a bit odd as well. That may needlessly break non-seekable output. Carl Eugen
On Tue, 2019-10-01 at 21:41 +0200, Carl Eugen Hoyos wrote: > Am Di., 1. Okt. 2019 um 21:35 Uhr schrieb Raphaël Zumer < > rzumer@tebako.net>: > > On Tue, 2019-10-01 at 20:09 +0100, Derek Buitenhuis wrote: > > > On 01/10/2019 18:25, James Almer wrote: > > > > The value in the unused field will be 0xFFFFFFFF after this > > > > change > > > > instead of 0, since you're writing 32 bits as duration instead > > > > of > > > > 64 > > > > where the high 32 bits (corresponding to the unused field) are > > > > zeroed. > > > > That means the ivf demuxer prior to this patch will read bogus > > > > duration > > > > values from ivf files created after this patch. > > > > > > > > Just leave the muxer as is. > > > > > > Why not just write zero? > > > > > > It's, to me, worse to leave a bogus 64-bit write to appease bugs > > > in > > > our > > > own demuxer. It's confusing and misleading for any readers of the > > > code. > > > > In that case I would prefer changing the initial written value to 0 > > rather than 0xFFFFFFFFFFFFFFFFULL. Writing over the unused bytes > > twice > > to get around an old error is a bit odd as well. > > That may needlessly break non-seekable output. Writing a 0 as the initial value is consistent with the behaviour of libvpx. libvpx writes 0 initially: https://github.com/webmproject/libvpx/blob/v1.8.1/vpxenc.c#L1191 then updates afterwards with the length (if output is seekable): https://github.com/webmproject/libvpx/blob/v1.8.1/vpxenc.c#L1209 (for reference, the ivf_write_file_header function is here: https://github.com/webmproject/libvpx/blob/v1.8.1/ivfenc.c#L16 ) So we need to make sure that ffmpeg can handle 0 values in this field regardless.
On Tue, 2019-10-01 at 15:57 -0400, Calvin Walton wrote: > On Tue, 2019-10-01 at 21:41 +0200, Carl Eugen Hoyos wrote: > > Am Di., 1. Okt. 2019 um 21:35 Uhr schrieb Raphaël Zumer < > > rzumer@tebako.net>: > > > On Tue, 2019-10-01 at 20:09 +0100, Derek Buitenhuis wrote: > > > > Why not just write zero? > > > > > > > > It's, to me, worse to leave a bogus 64-bit write to appease > > > > bugs > > > > in > > > > our > > > > own demuxer. It's confusing and misleading for any readers of > > > > the > > > > code. > > > > > > In that case I would prefer changing the initial written value to > > > 0 > > > rather than 0xFFFFFFFFFFFFFFFFULL. Writing over the unused bytes > > > twice > > > to get around an old error is a bit odd as well. > > > > That may needlessly break non-seekable output. > > Writing a 0 as the initial value is consistent with the behaviour of > libvpx. > > libvpx writes 0 initially: > https://github.com/webmproject/libvpx/blob/v1.8.1/vpxenc.c#L1191 > then updates afterwards with the length (if output is seekable): > https://github.com/webmproject/libvpx/blob/v1.8.1/vpxenc.c#L1209 > > (for reference, the ivf_write_file_header function is here: > https://github.com/webmproject/libvpx/blob/v1.8.1/ivfenc.c#L16 ) > > So we need to make sure that ffmpeg can handle 0 values in this field > regardless. > For now I sent a patch in reply to Derek's message, which writes the length field as a 32-bit value explicitly, and zeroes out the unused bytes. But I agree that if ffmpeg cannot decode a zero length field properly, and this is how libvpx codes unseekable files, that is another problem.
diff --git a/libavformat/ivfdec.c b/libavformat/ivfdec.c index 40ae464b76..2fdb6f5a04 100644 --- a/libavformat/ivfdec.c +++ b/libavformat/ivfdec.c @@ -53,7 +53,8 @@ static int read_header(AVFormatContext *s) st->codecpar->height = avio_rl16(s->pb); time_base.den = avio_rl32(s->pb); time_base.num = avio_rl32(s->pb); - st->duration = avio_rl64(s->pb); + st->duration = avio_rl32(s->pb); + avio_rl32(s->pb); // unused st->need_parsing = AVSTREAM_PARSE_HEADERS; diff --git a/libavformat/ivfenc.c b/libavformat/ivfenc.c index adf72117e9..e135a78213 100644 --- a/libavformat/ivfenc.c +++ b/libavformat/ivfenc.c @@ -53,7 +53,7 @@ static int ivf_write_header(AVFormatContext *s) avio_wl16(pb, par->height); avio_wl32(pb, s->streams[0]->time_base.den); avio_wl32(pb, s->streams[0]->time_base.num); - avio_wl64(pb, 0xFFFFFFFFFFFFFFFFULL); + avio_wl64(pb, 0xFFFFFFFFFFFFFFFFULL); // length is overwritten at the end of muxing return 0; } @@ -83,7 +83,8 @@ static int ivf_write_trailer(AVFormatContext *s) size_t end = avio_tell(pb); avio_seek(pb, 24, SEEK_SET); - avio_wl64(pb, ctx->frame_cnt * ctx->sum_delta_pts / (ctx->frame_cnt - 1)); + // overwrite the "length" field (duration) + avio_wl32(pb, ctx->frame_cnt * ctx->sum_delta_pts / (ctx->frame_cnt - 1)); avio_seek(pb, end, SEEK_SET); }
Signed-off-by: Raphaël Zumer <rzumer@tebako.net> --- libavformat/ivfdec.c | 3 ++- libavformat/ivfenc.c | 5 +++-- 2 files changed, 5 insertions(+), 3 deletions(-)