| Message ID | 20200310162616.39278-1-tfoucu@gmail.com |
|---|---|
| State | Withdrawn |
| Headers | show |
| Series | [FFmpeg-devel] libavformat/matroskaenc.c: Check if we can seek before writing the end of the ebml of the segment. | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
Thierry Foucu: > --- > libavformat/matroskaenc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c > index 42f21eae8b..cf436f9e3e 100644 > --- a/libavformat/matroskaenc.c > +++ b/libavformat/matroskaenc.c > @@ -2630,7 +2630,7 @@ static int mkv_write_trailer(AVFormatContext *s) > avio_seek(pb, currentpos, SEEK_SET); > } > > - if (!mkv->is_live) { > + if ((pb->seekable & AVIO_SEEKABLE_NORMAL) && !mkv->is_live) { > end_ebml_master(pb, mkv->segment); > } > > Why? Even if the output is unseekable, the seek to the beginning might succeed if it happens in the output buffer (unlikely but possible). Or do you have a specific usecase in mind where the segment should be unknown-length? - Andreas
On Wed, Mar 11, 2020 at 10:13 AM Andreas Rheinhardt < andreas.rheinhardt@gmail.com> wrote: > Thierry Foucu: > > --- > > libavformat/matroskaenc.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c > > index 42f21eae8b..cf436f9e3e 100644 > > --- a/libavformat/matroskaenc.c > > +++ b/libavformat/matroskaenc.c > > @@ -2630,7 +2630,7 @@ static int mkv_write_trailer(AVFormatContext *s) > > avio_seek(pb, currentpos, SEEK_SET); > > } > > > > - if (!mkv->is_live) { > > + if ((pb->seekable & AVIO_SEEKABLE_NORMAL) && !mkv->is_live) { > > end_ebml_master(pb, mkv->segment); > > } > > > > > Why? Even if the output is unseekable, the seek to the beginning might > succeed if it happens in the output buffer (unlikely but possible). Or > do you have a specific usecase in mind where the segment should be > unknown-length? > > In most cases in the muxer, we are checking if the file support seeking before writing some ebml, like we do for the cues in the trailer function. It is true that in some cases, the offset will be in the output buffer, then why not as well check it for cues? but most of the time, this offset will not when we are in streaming more. Maybe the aviobuf layer should not then call io_seek if the under layer has is_streamed = true? > - 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".
Thierry Foucu: > On Wed, Mar 11, 2020 at 10:13 AM Andreas Rheinhardt < > andreas.rheinhardt@gmail.com> wrote: > >> Thierry Foucu: >>> --- >>> libavformat/matroskaenc.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c >>> index 42f21eae8b..cf436f9e3e 100644 >>> --- a/libavformat/matroskaenc.c >>> +++ b/libavformat/matroskaenc.c >>> @@ -2630,7 +2630,7 @@ static int mkv_write_trailer(AVFormatContext *s) >>> avio_seek(pb, currentpos, SEEK_SET); >>> } >>> >>> - if (!mkv->is_live) { >>> + if ((pb->seekable & AVIO_SEEKABLE_NORMAL) && !mkv->is_live) { >>> end_ebml_master(pb, mkv->segment); >>> } >>> >>> >> Why? Even if the output is unseekable, the seek to the beginning might >> succeed if it happens in the output buffer (unlikely but possible). Or >> do you have a specific usecase in mind where the segment should be >> unknown-length? >> >> > In most cases in the muxer, we are checking if the file support seeking > before writing some ebml, like we do for the cues in the trailer function. > It is true that in some cases, the offset will be in the output buffer, > then why not as well check it for cues? Because in order to write cues, one would have to store them first and we don't do that given that it is very unlikely for the cues to be used later. Furthermore, in the other cases (e.g. the Info element): These are already written and freed when writing the header if the output is unseekable (or actually: if the output was unseekable during writing the header; the logic is wrong if the output's seekability status changes and dangerous if it was unseekable when writing the header and changes to seekable later; I'll send a patch for this). > but most of the time, this offset will not when we are in streaming more. > > Maybe the aviobuf layer should not then call io_seek if the under > layer has is_streamed > = true? > First of all, the aviobuf layer doesn't call io_seek() any more because it has been removed in 6e8e8431. And does it matter from which layer the error for a failed seek comes if the seek was unsuccessfull? (Furthermore, there is (at least?) an instance where the is_streamed is wrong: Namely if you use the seekable option for file output. But that seems to be intentional.) - Andreas
Thanks Andreas On Wed, Mar 11, 2020 at 5:35 PM Andreas Rheinhardt < andreas.rheinhardt@gmail.com> wrote: > Thierry Foucu: > > On Wed, Mar 11, 2020 at 10:13 AM Andreas Rheinhardt < > > andreas.rheinhardt@gmail.com> wrote: > > > >> Thierry Foucu: > >>> --- > >>> libavformat/matroskaenc.c | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c > >>> index 42f21eae8b..cf436f9e3e 100644 > >>> --- a/libavformat/matroskaenc.c > >>> +++ b/libavformat/matroskaenc.c > >>> @@ -2630,7 +2630,7 @@ static int mkv_write_trailer(AVFormatContext *s) > >>> avio_seek(pb, currentpos, SEEK_SET); > >>> } > >>> > >>> - if (!mkv->is_live) { > >>> + if ((pb->seekable & AVIO_SEEKABLE_NORMAL) && !mkv->is_live) { > >>> end_ebml_master(pb, mkv->segment); > >>> } > >>> > >>> > >> Why? Even if the output is unseekable, the seek to the beginning might > >> succeed if it happens in the output buffer (unlikely but possible). Or > >> do you have a specific usecase in mind where the segment should be > >> unknown-length? > >> > >> > > In most cases in the muxer, we are checking if the file support seeking > > before writing some ebml, like we do for the cues in the trailer > function. > > It is true that in some cases, the offset will be in the output buffer, > > then why not as well check it for cues? > > Because in order to write cues, one would have to store them first and > we don't do that given that it is very unlikely for the cues to be > used later. > Furthermore, in the other cases (e.g. the Info element): These are > already written and freed when writing the header if the output is > unseekable (or actually: if the output was unseekable during writing > the header; the logic is wrong if the output's seekability status > changes and dangerous if it was unseekable when writing the header and > changes to seekable later; I'll send a patch for this). > > > but most of the time, this offset will not when we are in streaming > more. > > > > Maybe the aviobuf layer should not then call io_seek if the under > > layer has is_streamed > > = true? > > > First of all, the aviobuf layer doesn't call io_seek() any more > because it has been removed in 6e8e8431. And does it matter from which > layer the error for a failed seek comes if the seek was unsuccessfull? > (Furthermore, there is (at least?) an instance where the is_streamed > is wrong: Namely if you use the seekable option for file output. But > that seems to be intentional.) > > We have some code to check if ffmpeg was calling seek for some streamable url or not and found this one. what about this: not only check for the seek function to be present (which will true for file.c) but check if it is seekable? =================================== diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c index cc22beff28..bbf7545750 100644 --- a/libavformat/aviobuf.c +++ b/libavformat/aviobuf.c @@ -312,7 +312,7 @@ int64_t avio_seek(AVIOContext *s, int64_t offset, int whence) if (s->write_flag) { flush_buffer(s); } - if (!s->seek) + if (!s->seek || s->seekable == 0) return AVERROR(EPIPE); if ((res = s->seek(s->opaque, offset, SEEK_SET)) < 0) return res; > - 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".
diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c index 42f21eae8b..cf436f9e3e 100644 --- a/libavformat/matroskaenc.c +++ b/libavformat/matroskaenc.c @@ -2630,7 +2630,7 @@ static int mkv_write_trailer(AVFormatContext *s) avio_seek(pb, currentpos, SEEK_SET); } - if (!mkv->is_live) { + if ((pb->seekable & AVIO_SEEKABLE_NORMAL) && !mkv->is_live) { end_ebml_master(pb, mkv->segment); }