diff mbox series

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

Message ID 20200310162616.39278-1-tfoucu@gmail.com
State New
Headers show
Series [FFmpeg-devel] libavformat/matroskaenc.c: Check if we can seek before writing the end of the ebml of the segment.
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Thierry Foucu March 10, 2020, 4:26 p.m. UTC
---
 libavformat/matroskaenc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Andreas Rheinhardt March 11, 2020, 4:44 p.m. UTC | #1
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
Thierry Foucu March 11, 2020, 7:15 p.m. UTC | #2
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".
Andreas Rheinhardt March 12, 2020, 12:35 a.m. UTC | #3
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
Thierry Foucu March 12, 2020, 5:12 p.m. UTC | #4
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 mbox series

Patch

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);
     }