| Message ID | 20161007060536.8136-1-jamrial@gmail.com |
|---|---|
| State | Accepted |
| Commit | c45ba265fcbb57fcacf82f66cb2a2643210308e1 |
| Headers | show |
On 10/7/2016 3:05 AM, James Almer wrote: > The dynamic AVIOContext would get closed pointing to the wrong position > in the buffer. > This is a regression since 650e17d88b63b5aca6e0a43483e89e64b0f7d2dd. > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > Example: > ./ffmpeg -i fate-samples/vorbis/vorbis_chapter_extension_demo.ogg -c:a copy -metadata:c key=value out.mka > > The output is corrupt before this patch. > > The refactoring isn't necessary per se, but it comes in handy for the next patch, > where attachments will not be treated as tracks anymore when writting tags. > > libavformat/matroskaenc.c | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c > index 0878cb5..286b8b4 100644 > --- a/libavformat/matroskaenc.c > +++ b/libavformat/matroskaenc.c > @@ -2264,17 +2264,19 @@ static int mkv_write_trailer(AVFormatContext *s) > end_ebml_master_crc32(pb, &mkv->info_bc, mkv, mkv->info); > > // update stream durations > - if (mkv->stream_durations) { > + if (!mkv->is_live && mkv->stream_durations) { > int i; > + int64_t curr = avio_tell(mkv->tags_bc); > for (i = 0; i < s->nb_streams; ++i) { > AVStream *st = s->streams[i]; > - double duration_sec = mkv->stream_durations[i] * av_q2d(st->time_base); > - char duration_string[20] = ""; > > - av_log(s, AV_LOG_DEBUG, "stream %d end duration = %" PRIu64 "\n", i, > - mkv->stream_durations[i]); > + if (mkv->stream_duration_offsets[i] > 0) { > + double duration_sec = mkv->stream_durations[i] * av_q2d(st->time_base); > + char duration_string[20] = ""; > + > + av_log(s, AV_LOG_DEBUG, "stream %d end duration = %" PRIu64 "\n", i, > + mkv->stream_durations[i]); > > - if (!mkv->is_live && mkv->stream_duration_offsets[i] > 0) { > avio_seek(mkv->tags_bc, mkv->stream_duration_offsets[i], SEEK_SET); > > snprintf(duration_string, 20, "%02d:%02d:%012.9f", > @@ -2284,6 +2286,7 @@ static int mkv_write_trailer(AVFormatContext *s) > put_ebml_binary(mkv->tags_bc, MATROSKA_ID_TAGSTRING, duration_string, 20); > } > } > + avio_seek(mkv->tags_bc, curr, SEEK_SET); > } > if (mkv->tags.pos && !mkv->is_live) { > avio_seek(pb, mkv->tags.pos, SEEK_SET); Ping. It's a recent regression on a muxer, so I'd like to solve it asap. I'll push it later today if nobody comments.
> On Oct 7, 2016, at 10:30 AM, James Almer <jamrial@gmail.com> wrote: > > On 10/7/2016 3:05 AM, James Almer wrote: >> The dynamic AVIOContext would get closed pointing to the wrong position >> in the buffer. >> This is a regression since 650e17d88b63b5aca6e0a43483e89e64b0f7d2dd. >> >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> Example: >> ./ffmpeg -i fate-samples/vorbis/vorbis_chapter_extension_demo.ogg -c:a copy -metadata:c key=value out.mka >> >> The output is corrupt before this patch. >> >> The refactoring isn't necessary per se, but it comes in handy for the next patch, >> where attachments will not be treated as tracks anymore when writting tags. >> >> libavformat/matroskaenc.c | 15 +++++++++------ >> 1 file changed, 9 insertions(+), 6 deletions(-) >> >> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c >> index 0878cb5..286b8b4 100644 >> --- a/libavformat/matroskaenc.c >> +++ b/libavformat/matroskaenc.c >> @@ -2264,17 +2264,19 @@ static int mkv_write_trailer(AVFormatContext *s) >> end_ebml_master_crc32(pb, &mkv->info_bc, mkv, mkv->info); >> >> // update stream durations >> - if (mkv->stream_durations) { >> + if (!mkv->is_live && mkv->stream_durations) { >> int i; >> + int64_t curr = avio_tell(mkv->tags_bc); >> for (i = 0; i < s->nb_streams; ++i) { >> AVStream *st = s->streams[i]; >> - double duration_sec = mkv->stream_durations[i] * av_q2d(st->time_base); >> - char duration_string[20] = ""; >> >> - av_log(s, AV_LOG_DEBUG, "stream %d end duration = %" PRIu64 "\n", i, >> - mkv->stream_durations[i]); >> + if (mkv->stream_duration_offsets[i] > 0) { >> + double duration_sec = mkv->stream_durations[i] * av_q2d(st->time_base); >> + char duration_string[20] = ""; >> + >> + av_log(s, AV_LOG_DEBUG, "stream %d end duration = %" PRIu64 "\n", i, >> + mkv->stream_durations[i]); >> >> - if (!mkv->is_live && mkv->stream_duration_offsets[i] > 0) { >> avio_seek(mkv->tags_bc, mkv->stream_duration_offsets[i], SEEK_SET); >> >> snprintf(duration_string, 20, "%02d:%02d:%012.9f", >> @@ -2284,6 +2286,7 @@ static int mkv_write_trailer(AVFormatContext *s) >> put_ebml_binary(mkv->tags_bc, MATROSKA_ID_TAGSTRING, duration_string, 20); >> } >> } >> + avio_seek(mkv->tags_bc, curr, SEEK_SET); >> } >> if (mkv->tags.pos && !mkv->is_live) { >> avio_seek(pb, mkv->tags.pos, SEEK_SET); > > > Ping. It's a recent regression on a muxer, so I'd like to solve it asap. > I'll push it later today if nobody comments. I tested this and it does fix the issue so that the added "key" tag is properly formed, whereas the tag is only null bytes without the patch. Thanks! Dave Rice
On 10/7/2016 3:29 PM, Dave Rice wrote: > >> On Oct 7, 2016, at 10:30 AM, James Almer <jamrial@gmail.com> wrote: >> >> On 10/7/2016 3:05 AM, James Almer wrote: >>> The dynamic AVIOContext would get closed pointing to the wrong position >>> in the buffer. >>> This is a regression since 650e17d88b63b5aca6e0a43483e89e64b0f7d2dd. >>> >>> Signed-off-by: James Almer <jamrial@gmail.com> >>> --- >>> Example: >>> ./ffmpeg -i fate-samples/vorbis/vorbis_chapter_extension_demo.ogg -c:a copy -metadata:c key=value out.mka >>> >>> The output is corrupt before this patch. >>> >>> The refactoring isn't necessary per se, but it comes in handy for the next patch, >>> where attachments will not be treated as tracks anymore when writting tags. >>> >>> libavformat/matroskaenc.c | 15 +++++++++------ >>> 1 file changed, 9 insertions(+), 6 deletions(-) >>> >>> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c >>> index 0878cb5..286b8b4 100644 >>> --- a/libavformat/matroskaenc.c >>> +++ b/libavformat/matroskaenc.c >>> @@ -2264,17 +2264,19 @@ static int mkv_write_trailer(AVFormatContext *s) >>> end_ebml_master_crc32(pb, &mkv->info_bc, mkv, mkv->info); >>> >>> // update stream durations >>> - if (mkv->stream_durations) { >>> + if (!mkv->is_live && mkv->stream_durations) { >>> int i; >>> + int64_t curr = avio_tell(mkv->tags_bc); >>> for (i = 0; i < s->nb_streams; ++i) { >>> AVStream *st = s->streams[i]; >>> - double duration_sec = mkv->stream_durations[i] * av_q2d(st->time_base); >>> - char duration_string[20] = ""; >>> >>> - av_log(s, AV_LOG_DEBUG, "stream %d end duration = %" PRIu64 "\n", i, >>> - mkv->stream_durations[i]); >>> + if (mkv->stream_duration_offsets[i] > 0) { >>> + double duration_sec = mkv->stream_durations[i] * av_q2d(st->time_base); >>> + char duration_string[20] = ""; >>> + >>> + av_log(s, AV_LOG_DEBUG, "stream %d end duration = %" PRIu64 "\n", i, >>> + mkv->stream_durations[i]); >>> >>> - if (!mkv->is_live && mkv->stream_duration_offsets[i] > 0) { >>> avio_seek(mkv->tags_bc, mkv->stream_duration_offsets[i], SEEK_SET); >>> >>> snprintf(duration_string, 20, "%02d:%02d:%012.9f", >>> @@ -2284,6 +2286,7 @@ static int mkv_write_trailer(AVFormatContext *s) >>> put_ebml_binary(mkv->tags_bc, MATROSKA_ID_TAGSTRING, duration_string, 20); >>> } >>> } >>> + avio_seek(mkv->tags_bc, curr, SEEK_SET); >>> } >>> if (mkv->tags.pos && !mkv->is_live) { >>> avio_seek(pb, mkv->tags.pos, SEEK_SET); >> >> >> Ping. It's a recent regression on a muxer, so I'd like to solve it asap. >> I'll push it later today if nobody comments. > > I tested this and it does fix the issue so that the added "key" tag is properly formed, whereas the tag is only null bytes without the patch. Thanks! > Dave Rice Pushed, thanks.
diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c index 0878cb5..286b8b4 100644 --- a/libavformat/matroskaenc.c +++ b/libavformat/matroskaenc.c @@ -2264,17 +2264,19 @@ static int mkv_write_trailer(AVFormatContext *s) end_ebml_master_crc32(pb, &mkv->info_bc, mkv, mkv->info); // update stream durations - if (mkv->stream_durations) { + if (!mkv->is_live && mkv->stream_durations) { int i; + int64_t curr = avio_tell(mkv->tags_bc); for (i = 0; i < s->nb_streams; ++i) { AVStream *st = s->streams[i]; - double duration_sec = mkv->stream_durations[i] * av_q2d(st->time_base); - char duration_string[20] = ""; - av_log(s, AV_LOG_DEBUG, "stream %d end duration = %" PRIu64 "\n", i, - mkv->stream_durations[i]); + if (mkv->stream_duration_offsets[i] > 0) { + double duration_sec = mkv->stream_durations[i] * av_q2d(st->time_base); + char duration_string[20] = ""; + + av_log(s, AV_LOG_DEBUG, "stream %d end duration = %" PRIu64 "\n", i, + mkv->stream_durations[i]); - if (!mkv->is_live && mkv->stream_duration_offsets[i] > 0) { avio_seek(mkv->tags_bc, mkv->stream_duration_offsets[i], SEEK_SET); snprintf(duration_string, 20, "%02d:%02d:%012.9f", @@ -2284,6 +2286,7 @@ static int mkv_write_trailer(AVFormatContext *s) put_ebml_binary(mkv->tags_bc, MATROSKA_ID_TAGSTRING, duration_string, 20); } } + avio_seek(mkv->tags_bc, curr, SEEK_SET); } if (mkv->tags.pos && !mkv->is_live) { avio_seek(pb, mkv->tags.pos, SEEK_SET);
The dynamic AVIOContext would get closed pointing to the wrong position in the buffer. This is a regression since 650e17d88b63b5aca6e0a43483e89e64b0f7d2dd. Signed-off-by: James Almer <jamrial@gmail.com> --- Example: ./ffmpeg -i fate-samples/vorbis/vorbis_chapter_extension_demo.ogg -c:a copy -metadata:c key=value out.mka The output is corrupt before this patch. The refactoring isn't necessary per se, but it comes in handy for the next patch, where attachments will not be treated as tracks anymore when writting tags. libavformat/matroskaenc.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)