[FFmpeg-devel] [PATCH] libavformat/matroskaenc.c: Check if we can seek before writing the end of the ebml of the segment.

[Thierry Foucu]

Thanks Andreas

On Wed, Mar 11, 2020 at 5:35 PM Andreas Rheinhardt <
andreas.rheinhardt at gmail.com> wrote:

> Thierry Foucu:
> > On Wed, Mar 11, 2020 at 10:13 AM Andreas Rheinhardt <
> > andreas.rheinhardt at 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 at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".



-- 

Thierry Foucu