Re: [FFmpeg-devel] [PATCH 23/24] ffmpeg: simplify the use of OutputStream.frame_number

Re: [FFmpeg-devel] [PATCH 23/24] ffmpeg: simplify the use of OutputStream.frame_number

[Anton Khirnov]

Quoting Michael Niedermayer (2021-12-14 22:13:43)
> On Mon, Dec 13, 2021 at 04:20:41PM +0100, Anton Khirnov wrote:
> > It features in limiting the number of output frames (-frames option) and
> > currently can be incremented from two places:
> > - for video encoding (not streamcopy), in do_video_out() after each
> >   successful avcodec_send_frame() call
> > - for all other cases, in of_submit_packet()
> > 
> > Not only is this inconsistent and confusing, but also the special
> > treatment for video encoding is redundant, since
> > AVCodecContext.frame_count stores the exact same value (number of
> > successful avcodec_send_frame()) calls.
> > 
> > Replace the use of OutputStream.frame_count in do_video_out() with
> > AVCodecContext.frame_count. Modify OutputStream.frame_count in the same
> > way for all streams.
> > ---
> >  fftools/ffmpeg.c     |  9 ++++-----
> >  fftools/ffmpeg_mux.c | 19 +++++++------------
> >  2 files changed, 11 insertions(+), 17 deletions(-)
> 
> This results in differences
> one is:
> ./ffmpeg -i ~/tickets/4072/CoP\ 1\ 1\ 3\ -M.avi -vframes 2 -bitexact -vcodec huffyuv /tmp/4072-fic-old.avi
> 
> -rw-r----- 1 michael michael 3572792 Dez 14 22:09 /tmp/4072-fic-new.avi
> -rw-r----- 1 michael michael 1908172 Dez 14 22:10 /tmp/4072-fic-old.avi
> 
> I see other differences caused by patches today i still need to investigate
> what causes what

Fixing this will require larger restructuring first, so I am dropping
this patch for now.

Please let me know if the rest of the set breaks anything else.

-- 
Anton Khirnov
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 23/24] ffmpeg: simplify the use of OutputStream.frame_number

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 1990 bytes --]

On Wed, Dec 15, 2021 at 08:36:25PM +0100, Anton Khirnov wrote:
> Quoting Michael Niedermayer (2021-12-14 22:13:43)
> > On Mon, Dec 13, 2021 at 04:20:41PM +0100, Anton Khirnov wrote:
> > > It features in limiting the number of output frames (-frames option) and
> > > currently can be incremented from two places:
> > > - for video encoding (not streamcopy), in do_video_out() after each
> > >   successful avcodec_send_frame() call
> > > - for all other cases, in of_submit_packet()
> > > 
> > > Not only is this inconsistent and confusing, but also the special
> > > treatment for video encoding is redundant, since
> > > AVCodecContext.frame_count stores the exact same value (number of
> > > successful avcodec_send_frame()) calls.
> > > 
> > > Replace the use of OutputStream.frame_count in do_video_out() with
> > > AVCodecContext.frame_count. Modify OutputStream.frame_count in the same
> > > way for all streams.
> > > ---
> > >  fftools/ffmpeg.c     |  9 ++++-----
> > >  fftools/ffmpeg_mux.c | 19 +++++++------------
> > >  2 files changed, 11 insertions(+), 17 deletions(-)
> > 
> > This results in differences
> > one is:
> > ./ffmpeg -i ~/tickets/4072/CoP\ 1\ 1\ 3\ -M.avi -vframes 2 -bitexact -vcodec huffyuv /tmp/4072-fic-old.avi
> > 
> > -rw-r----- 1 michael michael 3572792 Dez 14 22:09 /tmp/4072-fic-new.avi
> > -rw-r----- 1 michael michael 1908172 Dez 14 22:10 /tmp/4072-fic-old.avi
> > 
> > I see other differences caused by patches today i still need to investigate
> > what causes what
> 
> Fixing this will require larger restructuring first, so I am dropping
> this patch for now.
> 
> Please let me know if the rest of the set breaks anything else.

all failures from this patch set where from this patch i think

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I know you won't believe me, but the highest form of Human Excellence is
to question oneself and others. -- Socrates

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 02/24] ffmpeg: simplify getting output file size

[Andreas Rheinhardt]

Anton Khirnov:
> Stop calling avio_size()/avio_tell(), which are potentially heavy
> operations and may interfere with muxer operation. Use the recently
> introduced bytes_written field instead.
> ---
>  fftools/ffmpeg.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> index bdeff9a12e..b77531cb7f 100644
> --- a/fftools/ffmpeg.c
> +++ b/fftools/ffmpeg.c
> @@ -1677,8 +1677,7 @@ static void print_report(int is_last_report, int64_t timer_start, int64_t cur_ti
>  {
>      AVBPrint buf, buf_script;
>      OutputStream *ost;
> -    AVFormatContext *oc;
> -    int64_t total_size;
> +    int64_t total_size = -1;
>      AVCodecContext *enc;
>      int frame_number, vid, i;
>      double bitrate;
> @@ -1708,11 +1707,8 @@ static void print_report(int is_last_report, int64_t timer_start, int64_t cur_ti
>      t = (cur_time-timer_start) / 1000000.0;
>  
>  
> -    oc = output_files[0]->ctx;
> -
> -    total_size = avio_size(oc->pb);
> -    if (total_size <= 0) // FIXME improve avio_size() so it works with non seekable output too
> -        total_size = avio_tell(oc->pb);
> +    if (output_files[0]->ctx->pb)
> +        total_size = output_files[0]->ctx->pb->bytes_written;
>  
>      vid = 0;
>      av_bprint_init(&buf, 0, AV_BPRINT_SIZE_AUTOMATIC);
> 

1. bytes_written is only updated when the data is actually written, i.e.
the stuff still residing in the AVIOContext's internal buffer is not
accounted yet. This might lead to the bitrate being significantly
underestimated at the beginning of output.
2. In case of seekable output when data already written is overwritten
the new stats will be totally off. With -movflags +faststart the final
size is off by a factor of two.

- 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".

Re: [FFmpeg-devel] [PATCH 03/24] ffmpeg: remove a redundant assignment of interrupt_callback

[Andreas Rheinhardt]

Anton Khirnov:
> It is already set in open_output_file().
> ---
>  fftools/ffmpeg.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> index b77531cb7f..fb017a1e37 100644
> --- a/fftools/ffmpeg.c
> +++ b/fftools/ffmpeg.c
> @@ -2947,8 +2947,6 @@ static int check_init_output_file(OutputFile *of, int file_index)
>              return 0;
>      }
>  
> -    of->ctx->interrupt_callback = int_cb;
> -
>      ret = avformat_write_header(of->ctx, &of->opts);
>      if (ret < 0) {
>          av_log(NULL, AV_LOG_ERROR,
> 

Seems fine.

- 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".

Re: [FFmpeg-devel] [PATCH 17/24] ffmpeg: do not log to the muxer context

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 1416 bytes --]

On Mon, Dec 13, 2021 at 04:20:35PM +0100, Anton Khirnov wrote:
> All other logging goes to NULL context.
> ---
>  fftools/ffmpeg.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> index d69e4119ef..afd442ff4e 100644
> --- a/fftools/ffmpeg.c
> +++ b/fftools/ffmpeg.c
> @@ -2954,7 +2954,6 @@ static void init_encoder_time_base(OutputStream *ost, AVRational default_time_ba
>  {
>      InputStream *ist = get_input_stream(ost);
>      AVCodecContext *enc_ctx = ost->enc_ctx;
> -    AVFormatContext *oc;
>  
>      if (ost->enc_timebase.num > 0) {
>          enc_ctx->time_base = ost->enc_timebase;
> @@ -2967,8 +2966,7 @@ static void init_encoder_time_base(OutputStream *ost, AVRational default_time_ba
>              return;
>          }
>  
> -        oc = output_files[ost->file_index]->ctx;
> -        av_log(oc, AV_LOG_WARNING, "Input stream data not available, using default time base\n");
> +        av_log(NULL, AV_LOG_WARNING, "Input stream data not available, using default time base\n");
>      }

If this no longer shows the muxer then some information should be added so
the user knows to which part the warning belongs on a complex transcode

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

He who knows, does not speak. He who speaks, does not know. -- Lao Tsu

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 06/24] ffmpeg: move writing the trailer to ffmpeg_mux.c

[Andreas Rheinhardt]

Anton Khirnov:
> ---
>  fftools/ffmpeg.c     | 16 +++-------------
>  fftools/ffmpeg.h     |  1 +
>  fftools/ffmpeg_mux.c | 21 +++++++++++++++++++++
>  3 files changed, 25 insertions(+), 13 deletions(-)
> 
> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> index a00fe58063..1fb10869b4 100644
> --- a/fftools/ffmpeg.c
> +++ b/fftools/ffmpeg.c
> @@ -4436,19 +4436,9 @@ static int transcode(void)
>  
>      /* write the trailer if needed */
>      for (i = 0; i < nb_output_files; i++) {
> -        os = output_files[i]->ctx;
> -        if (!output_files[i]->header_written) {
> -            av_log(NULL, AV_LOG_ERROR,
> -                   "Nothing was written into output file %d (%s), because "
> -                   "at least one of its streams received no packets.\n",
> -                   i, os->url);
> -            continue;
> -        }
> -        if ((ret = av_write_trailer(os)) < 0) {
> -            av_log(NULL, AV_LOG_ERROR, "Error writing trailer of %s: %s\n", os->url, av_err2str(ret));
> -            if (exit_on_error)
> -                exit_program(1);
> -        }
> +        ret = of_write_trailer(output_files[i]);
> +        if (exit_on_error)

Shouldn't you check for ret and exit_on_error here?

> +            exit_program(1);
>      }
>  
>      /* dump report by using the first video and audio streams */
> diff --git a/fftools/ffmpeg.h b/fftools/ffmpeg.h
> index fed34b06f8..91c313d8ef 100644
> --- a/fftools/ffmpeg.h
> +++ b/fftools/ffmpeg.h
> @@ -686,6 +686,7 @@ int hwaccel_decode_init(AVCodecContext *avctx);
>  
>  /* open the muxer when all the streams are initialized */
>  int of_check_init(OutputFile *of);
> +int of_write_trailer(OutputFile *of);
>  
>  void of_write_packet(OutputFile *of, AVPacket *pkt, OutputStream *ost,
>                       int unqueue);
> diff --git a/fftools/ffmpeg_mux.c b/fftools/ffmpeg_mux.c
> index e7c6ddd8f8..fec8537506 100644
> --- a/fftools/ffmpeg_mux.c
> +++ b/fftools/ffmpeg_mux.c
> @@ -291,3 +291,24 @@ int of_check_init(OutputFile *of)
>  
>      return 0;
>  }
> +
> +int of_write_trailer(OutputFile *of)
> +{
> +    int ret;
> +
> +    if (!of->header_written) {
> +        av_log(NULL, AV_LOG_ERROR,
> +               "Nothing was written into output file %d (%s), because "
> +               "at least one of its streams received no packets.\n",
> +               of->index, of->ctx->url);
> +        return AVERROR(EINVAL);
> +    }
> +
> +    ret = av_write_trailer(of->ctx);
> +    if (ret < 0) {
> +        av_log(NULL, AV_LOG_ERROR, "Error writing trailer of %s: %s\n", of->ctx->url, av_err2str(ret));
> +        return ret;
> +    }
> +
> +    return 0;
> +}
> 

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 05/24] ffmpeg: move some muxing-related code into a separate file

[Andreas Rheinhardt]

Anton Khirnov:
> This is a first step towards making muxers more independent from the
> rest of the code.
> ---
>  fftools/Makefile     |  11 +-
>  fftools/ffmpeg.c     | 273 ++--------------------------------------
>  fftools/ffmpeg.h     |  10 ++
>  fftools/ffmpeg_mux.c | 293 +++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 320 insertions(+), 267 deletions(-)
>  create mode 100644 fftools/ffmpeg_mux.c
> 
> diff --git a/fftools/Makefile b/fftools/Makefile
> index da420786eb..bd90ef8f32 100644
> --- a/fftools/Makefile
> +++ b/fftools/Makefile
> @@ -9,7 +9,16 @@ AVBASENAMES  = ffmpeg ffplay ffprobe
>  ALLAVPROGS   = $(AVBASENAMES:%=%$(PROGSSUF)$(EXESUF))
>  ALLAVPROGS_G = $(AVBASENAMES:%=%$(PROGSSUF)_g$(EXESUF))
>  
> -OBJS-ffmpeg                        += fftools/ffmpeg_opt.o fftools/ffmpeg_filter.o fftools/ffmpeg_hw.o
> +OBJS-ffmpeg +=                  \
> +    fftools/ffmpeg_filter.o     \
> +    fftools/ffmpeg_hw.o         \
> +    fftools/ffmpeg_mux.o        \
> +    fftools/ffmpeg_opt.o        \
> +
> +ifndef CONFIG_VIDEOTOOLBOX
> +OBJS-ffmpeg-$(CONFIG_VDA)          += fftools/ffmpeg_videotoolbox.o
> +endif
> +OBJS-ffmpeg-$(CONFIG_VIDEOTOOLBOX) += fftools/ffmpeg_videotoolbox.o

Rebase error.

>  
>  define DOFFTOOL
>  OBJS-$(1) += fftools/cmdutils.o fftools/$(1).o $(OBJS-$(1)-yes)


_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 05/24] ffmpeg: move some muxing-related code into a separate file

[Andreas Rheinhardt]

Anton Khirnov:
> This is a first step towards making muxers more independent from the
> rest of the code.
> ---
>  fftools/Makefile     |  11 +-
>  fftools/ffmpeg.c     | 273 ++--------------------------------------
>  fftools/ffmpeg.h     |  10 ++
>  fftools/ffmpeg_mux.c | 293 +++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 320 insertions(+), 267 deletions(-)
>  create mode 100644 fftools/ffmpeg_mux.c
> 
> diff --git a/fftools/ffmpeg_mux.c b/fftools/ffmpeg_mux.c
> new file mode 100644
> index 0000000000..e7c6ddd8f8
> --- /dev/null
> +++ b/fftools/ffmpeg_mux.c
> @@ -0,0 +1,293 @@
> +/*
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +#include <stdio.h>
> +#include <string.h>
> +
> +#include "libavformat/avformat.h"
> +#include "libavformat/avio.h"
> +
> +#include "libavcodec/packet.h"
> +
> +#include "libavutil/fifo.h"
> +#include "libavutil/intreadwrite.h"
> +#include "libavutil/log.h"
> +#include "libavutil/mem.h"
> +#include "libavutil/timestamp.h"
> +
> +#include "ffmpeg.h"
> +

These library headers are ordered reversely to our usual order. Is this
intended? (It has the advantage that e.g. missing lavu headers in the
lavf headers could be uncovered.)

- 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".

Re: [FFmpeg-devel] [PATCH 05/24] ffmpeg: move some muxing-related code into a separate file

[Anton Khirnov]

Quoting Andreas Rheinhardt (2021-12-17 02:55:43)
> Anton Khirnov:
> > This is a first step towards making muxers more independent from the
> > rest of the code.
> > ---
> >  fftools/Makefile     |  11 +-
> >  fftools/ffmpeg.c     | 273 ++--------------------------------------
> >  fftools/ffmpeg.h     |  10 ++
> >  fftools/ffmpeg_mux.c | 293 +++++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 320 insertions(+), 267 deletions(-)
> >  create mode 100644 fftools/ffmpeg_mux.c
> > 
> > diff --git a/fftools/ffmpeg_mux.c b/fftools/ffmpeg_mux.c
> > new file mode 100644
> > index 0000000000..e7c6ddd8f8
> > --- /dev/null
> > +++ b/fftools/ffmpeg_mux.c
> > @@ -0,0 +1,293 @@
> > +/*
> > + * This file is part of FFmpeg.
> > + *
> > + * FFmpeg is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser General Public
> > + * License as published by the Free Software Foundation; either
> > + * version 2.1 of the License, or (at your option) any later version.
> > + *
> > + * FFmpeg is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > + * Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public
> > + * License along with FFmpeg; if not, write to the Free Software
> > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> > + */
> > +
> > +#include <stdio.h>
> > +#include <string.h>
> > +
> > +#include "libavformat/avformat.h"
> > +#include "libavformat/avio.h"
> > +
> > +#include "libavcodec/packet.h"
> > +
> > +#include "libavutil/fifo.h"
> > +#include "libavutil/intreadwrite.h"
> > +#include "libavutil/log.h"
> > +#include "libavutil/mem.h"
> > +#include "libavutil/timestamp.h"
> > +
> > +#include "ffmpeg.h"
> > +
> 
> These library headers are ordered reversely to our usual order. Is
> this intended?

I am not aware of there being a usual order. So no, it's not.

> (It has the advantage that e.g. missing lavu headers
> in the lavf headers could be uncovered.)

Won't make checkheaders also find that?

-- 
Anton Khirnov
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 05/24] ffmpeg: move some muxing-related code into a separate file

[Andreas Rheinhardt]

Anton Khirnov:
> Quoting Andreas Rheinhardt (2021-12-17 02:55:43)
>> Anton Khirnov:
>>> This is a first step towards making muxers more independent from the
>>> rest of the code.
>>> ---
>>>  fftools/Makefile     |  11 +-
>>>  fftools/ffmpeg.c     | 273 ++--------------------------------------
>>>  fftools/ffmpeg.h     |  10 ++
>>>  fftools/ffmpeg_mux.c | 293 +++++++++++++++++++++++++++++++++++++++++++
>>>  4 files changed, 320 insertions(+), 267 deletions(-)
>>>  create mode 100644 fftools/ffmpeg_mux.c
>>>
>>> diff --git a/fftools/ffmpeg_mux.c b/fftools/ffmpeg_mux.c
>>> new file mode 100644
>>> index 0000000000..e7c6ddd8f8
>>> --- /dev/null
>>> +++ b/fftools/ffmpeg_mux.c
>>> @@ -0,0 +1,293 @@
>>> +/*
>>> + * This file is part of FFmpeg.
>>> + *
>>> + * FFmpeg is free software; you can redistribute it and/or
>>> + * modify it under the terms of the GNU Lesser General Public
>>> + * License as published by the Free Software Foundation; either
>>> + * version 2.1 of the License, or (at your option) any later version.
>>> + *
>>> + * FFmpeg is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>>> + * Lesser General Public License for more details.
>>> + *
>>> + * You should have received a copy of the GNU Lesser General Public
>>> + * License along with FFmpeg; if not, write to the Free Software
>>> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
>>> + */
>>> +
>>> +#include <stdio.h>
>>> +#include <string.h>
>>> +
>>> +#include "libavformat/avformat.h"
>>> +#include "libavformat/avio.h"
>>> +
>>> +#include "libavcodec/packet.h"
>>> +
>>> +#include "libavutil/fifo.h"
>>> +#include "libavutil/intreadwrite.h"
>>> +#include "libavutil/log.h"
>>> +#include "libavutil/mem.h"
>>> +#include "libavutil/timestamp.h"
>>> +
>>> +#include "ffmpeg.h"
>>> +
>>
>> These library headers are ordered reversely to our usual order. Is
>> this intended?
> 
> I am not aware of there being a usual order. So no, it's not.
> 

The usual order is the inverse of linking order (i.e. lavu-lavc-lavf).

>> (It has the advantage that e.g. missing lavu headers
>> in the lavf headers could be uncovered.)
> 
> Won't make checkheaders also find that?
> 

Some of our headers behave differently when included internally (when
DHAVE_AV_CONFIG_H is defined) than when included externally; the fftools
belong to the latter category, I believe checkheaders belongs to the former.

- 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".

Re: [FFmpeg-devel] [PATCH 09/24] ffmpeg_mux: add private muxer context

[Andreas Rheinhardt]

Anton Khirnov:
> Move header_written into it, which is not (and should not be) used by
> any code outside of ffmpeg_mux.
> 
> In the future this context will contain more muxer-private state that
> should not be visible to other code.
> ---
>  fftools/ffmpeg.h     |  6 ++++--
>  fftools/ffmpeg_mux.c | 26 ++++++++++++++++++++++----
>  fftools/ffmpeg_opt.c |  6 ++++++
>  3 files changed, 32 insertions(+), 6 deletions(-)
> 
> diff --git a/fftools/ffmpeg.h b/fftools/ffmpeg.h
> index c28acad4f1..279a99cc48 100644
> --- a/fftools/ffmpeg.h
> +++ b/fftools/ffmpeg.h
> @@ -573,9 +573,12 @@ typedef struct OutputStream {
>      int64_t error[4];
>  } OutputStream;
>  
> +typedef struct Muxer Muxer;
> +
>  typedef struct OutputFile {
>      int index;
>  
> +    Muxer                *mux;
>      const AVOutputFormat *format;
>  
>      AVFormatContext *ctx;
> @@ -586,8 +589,6 @@ typedef struct OutputFile {
>      uint64_t limit_filesize; /* filesize limit expressed in bytes */
>  
>      int shortest;
> -
> -    int header_written;
>  } OutputFile;
>  
>  extern InputStream **input_streams;
> @@ -686,6 +687,7 @@ int hw_device_setup_for_filter(FilterGraph *fg);
>  
>  int hwaccel_decode_init(AVCodecContext *avctx);
>  
> +int of_muxer_init(OutputFile *of);
>  /* open the muxer when all the streams are initialized */
>  int of_check_init(OutputFile *of);
>  int of_write_trailer(OutputFile *of);
> diff --git a/fftools/ffmpeg_mux.c b/fftools/ffmpeg_mux.c
> index 9787fe7f78..cf85db674e 100644
> --- a/fftools/ffmpeg_mux.c
> +++ b/fftools/ffmpeg_mux.c
> @@ -32,6 +32,10 @@
>  
>  #include "ffmpeg.h"
>  
> +struct Muxer {
> +    int header_written;
> +};
> +
>  static void close_all_output_streams(OutputStream *ost, OSTFinished this_stream, OSTFinished others)
>  {
>      int i;
> @@ -64,7 +68,7 @@ void of_write_packet(OutputFile *of, AVPacket *pkt, OutputStream *ost,
>          ost->frame_number++;
>      }
>  
> -    if (!of->header_written) {
> +    if (!of->mux->header_written) {
>          AVPacket *tmp_pkt;
>          /* the muxer is not initialized yet, buffer the packet */
>          if (!av_fifo_space(ost->muxing_queue)) {
> @@ -195,7 +199,7 @@ static int print_sdp(void)
>      AVFormatContext **avc;
>  
>      for (i = 0; i < nb_output_files; i++) {
> -        if (!output_files[i]->header_written)
> +        if (!output_files[i]->mux->header_written)
>              return 0;
>      }
>  
> @@ -259,7 +263,7 @@ int of_check_init(OutputFile *of)
>          return ret;
>      }
>      //assert_avoptions(of->opts);
> -    of->header_written = 1;
> +    of->mux->header_written = 1;
>  
>      av_dump_format(of->ctx, of->index, of->ctx->url, 1);
>      nb_output_dumped++;
> @@ -296,7 +300,7 @@ int of_write_trailer(OutputFile *of)
>  {
>      int ret;
>  
> -    if (!of->header_written) {
> +    if (!of->mux->header_written) {
>          av_log(NULL, AV_LOG_ERROR,
>                 "Nothing was written into output file %d (%s), because "
>                 "at least one of its streams received no packets.\n",
> @@ -327,5 +331,19 @@ void of_close(OutputFile **pof)
>      avformat_free_context(s);
>      av_dict_free(&of->opts);
>  
> +    av_freep(&of->mux);
> +
>      av_freep(pof);
>  }
> +
> +int of_muxer_init(OutputFile *of)
> +{
> +    Muxer *mux = av_mallocz(sizeof(*mux));
> +
> +    if (!mux)
> +        return AVERROR(ENOMEM);
> +
> +    of->mux  = mux;
> +
> +    return 0;
> +}
> diff --git a/fftools/ffmpeg_opt.c b/fftools/ffmpeg_opt.c
> index 433102bb49..5b42b450f3 100644
> --- a/fftools/ffmpeg_opt.c
> +++ b/fftools/ffmpeg_opt.c
> @@ -2886,6 +2886,12 @@ loop_end:
>          exit_program(1);
>      }
>  
> +    err = of_muxer_init(of);
> +    if (err < 0) {
> +        av_log(NULL, AV_LOG_FATAL, "Error initializing the muxer\n");

Totally confusing error message: A user will think that it is the actual
muxer and not some struct Muxer that could not be initialized.

> +        exit_program(1);
> +    }
> +
>      return 0;
>  }
>  
> 

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 16/24] ffmpeg: access output file chapters through a wrapper

[Andreas Rheinhardt]

Anton Khirnov:
> Avoid accessing the muxer context directly, as this will become
> forbidden in future commits.
> ---
>  fftools/ffmpeg.c     | 15 +++++++++------
>  fftools/ffmpeg.h     |  2 ++
>  fftools/ffmpeg_mux.c |  7 +++++++
>  3 files changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> index 902f190191..d69e4119ef 100644
> --- a/fftools/ffmpeg.c
> +++ b/fftools/ffmpeg.c
> @@ -2909,12 +2909,15 @@ static void parse_forced_key_frames(char *kf, OutputStream *ost,
>              *next++ = 0;
>  
>          if (!memcmp(p, "chapters", 8)) {
> -
> -            AVFormatContext *avf = output_files[ost->file_index]->ctx;
> +            OutputFile *of = output_files[ost->file_index];
> +            AVChapter * const *ch;
> +            unsigned int    nb_ch;
>              int j;
>  
> -            if (avf->nb_chapters > INT_MAX - size ||
> -                !(pts = av_realloc_f(pts, size += avf->nb_chapters - 1,
> +            ch = of_get_chapters(of, &nb_ch);
> +
> +            if (nb_ch > INT_MAX - size ||
> +                !(pts = av_realloc_f(pts, size += nb_ch - 1,
>                                       sizeof(*pts)))) {
>                  av_log(NULL, AV_LOG_FATAL,
>                         "Could not allocate forced key frames array.\n");
> @@ -2923,8 +2926,8 @@ static void parse_forced_key_frames(char *kf, OutputStream *ost,
>              t = p[8] ? parse_time_or_die("force_key_frames", p + 8, 1) : 0;
>              t = av_rescale_q(t, AV_TIME_BASE_Q, avctx->time_base);
>  
> -            for (j = 0; j < avf->nb_chapters; j++) {
> -                AVChapter *c = avf->chapters[j];
> +            for (j = 0; j < nb_ch; j++) {
> +                const AVChapter *c = ch[j];
>                  av_assert1(index < size);
>                  pts[index++] = av_rescale_q(c->start, c->time_base,
>                                              avctx->time_base) + t;
> diff --git a/fftools/ffmpeg.h b/fftools/ffmpeg.h
> index 78c2295c8e..8119282aed 100644
> --- a/fftools/ffmpeg.h
> +++ b/fftools/ffmpeg.h
> @@ -697,5 +697,7 @@ void of_write_packet(OutputFile *of, AVPacket *pkt, OutputStream *ost,
>                       int unqueue);
>  int of_finished(OutputFile *of);
>  int64_t of_bytes_written(OutputFile *of);
> +AVChapter * const *
> +of_get_chapters(OutputFile *of, unsigned int *nb_chapters);
>  
>  #endif /* FFTOOLS_FFMPEG_H */
> diff --git a/fftools/ffmpeg_mux.c b/fftools/ffmpeg_mux.c
> index 3ee0fc0667..6c9f10db9c 100644
> --- a/fftools/ffmpeg_mux.c
> +++ b/fftools/ffmpeg_mux.c
> @@ -390,3 +390,10 @@ int64_t of_bytes_written(OutputFile *of)
>      return of->mux->final_filesize ? of->mux->final_filesize :
>              pb ? pb->bytes_written : -1;
>  }
> +
> +AVChapter * const *
> +of_get_chapters(OutputFile *of, unsigned int *nb_chapters)
> +{
> +    *nb_chapters = of->ctx->nb_chapters;
> +    return of->ctx->chapters;
> +}
> 

I don't see any benefit of factoring muxing out of OutputFile at all; to
the contrary, it adds another layer of indirection, of allocations for
your opaque structs, of prohibitions and of unnatural getters like this
one here. If your patchset were already applied and someone posted the
reverse of patch #15, I'd LGTM it, because it makes checking for the
bitexact flag local to the place where it is used and thereby avoids
storing these variables in a context for only one usage.
I also think it is more natural to store each OutputStream's queue in
the OutputStream instead of adding a new array to your struct Muxer.

- 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".

Re: [FFmpeg-devel] [PATCH 16/24] ffmpeg: access output file chapters through a wrapper

[Anton Khirnov]

Quoting Andreas Rheinhardt (2021-12-17 00:08:07)
> Anton Khirnov:
> > Avoid accessing the muxer context directly, as this will become
> > forbidden in future commits.
> > ---
> >  fftools/ffmpeg.c     | 15 +++++++++------
> >  fftools/ffmpeg.h     |  2 ++
> >  fftools/ffmpeg_mux.c |  7 +++++++
> >  3 files changed, 18 insertions(+), 6 deletions(-)
> > 
> > diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> > index 902f190191..d69e4119ef 100644
> > --- a/fftools/ffmpeg.c
> > +++ b/fftools/ffmpeg.c
> > @@ -2909,12 +2909,15 @@ static void parse_forced_key_frames(char *kf, OutputStream *ost,
> >              *next++ = 0;
> >  
> >          if (!memcmp(p, "chapters", 8)) {
> > -
> > -            AVFormatContext *avf = output_files[ost->file_index]->ctx;
> > +            OutputFile *of = output_files[ost->file_index];
> > +            AVChapter * const *ch;
> > +            unsigned int    nb_ch;
> >              int j;
> >  
> > -            if (avf->nb_chapters > INT_MAX - size ||
> > -                !(pts = av_realloc_f(pts, size += avf->nb_chapters - 1,
> > +            ch = of_get_chapters(of, &nb_ch);
> > +
> > +            if (nb_ch > INT_MAX - size ||
> > +                !(pts = av_realloc_f(pts, size += nb_ch - 1,
> >                                       sizeof(*pts)))) {
> >                  av_log(NULL, AV_LOG_FATAL,
> >                         "Could not allocate forced key frames array.\n");
> > @@ -2923,8 +2926,8 @@ static void parse_forced_key_frames(char *kf, OutputStream *ost,
> >              t = p[8] ? parse_time_or_die("force_key_frames", p + 8, 1) : 0;
> >              t = av_rescale_q(t, AV_TIME_BASE_Q, avctx->time_base);
> >  
> > -            for (j = 0; j < avf->nb_chapters; j++) {
> > -                AVChapter *c = avf->chapters[j];
> > +            for (j = 0; j < nb_ch; j++) {
> > +                const AVChapter *c = ch[j];
> >                  av_assert1(index < size);
> >                  pts[index++] = av_rescale_q(c->start, c->time_base,
> >                                              avctx->time_base) + t;
> > diff --git a/fftools/ffmpeg.h b/fftools/ffmpeg.h
> > index 78c2295c8e..8119282aed 100644
> > --- a/fftools/ffmpeg.h
> > +++ b/fftools/ffmpeg.h
> > @@ -697,5 +697,7 @@ void of_write_packet(OutputFile *of, AVPacket *pkt, OutputStream *ost,
> >                       int unqueue);
> >  int of_finished(OutputFile *of);
> >  int64_t of_bytes_written(OutputFile *of);
> > +AVChapter * const *
> > +of_get_chapters(OutputFile *of, unsigned int *nb_chapters);
> >  
> >  #endif /* FFTOOLS_FFMPEG_H */
> > diff --git a/fftools/ffmpeg_mux.c b/fftools/ffmpeg_mux.c
> > index 3ee0fc0667..6c9f10db9c 100644
> > --- a/fftools/ffmpeg_mux.c
> > +++ b/fftools/ffmpeg_mux.c
> > @@ -390,3 +390,10 @@ int64_t of_bytes_written(OutputFile *of)
> >      return of->mux->final_filesize ? of->mux->final_filesize :
> >              pb ? pb->bytes_written : -1;
> >  }
> > +
> > +AVChapter * const *
> > +of_get_chapters(OutputFile *of, unsigned int *nb_chapters)
> > +{
> > +    *nb_chapters = of->ctx->nb_chapters;
> > +    return of->ctx->chapters;
> > +}
> > 
> 
> I don't see any benefit of factoring muxing out of OutputFile at all; to
> the contrary, it adds another layer of indirection, of allocations for
> your opaque structs, of prohibitions and of unnatural getters like this
> one here.

The benefit is in making it clear which objects can be accessed by what
code. ffmpeg.c currently has a big problem with lack of structure -
random bits of code change random bits of state without much coherence
to it. As a result it is very hard to reason about the code or add new
features cleanly (or even at all, without breaking 10 random unrelated
use cases). In that light I see the new indirections and prohibitions as
an improvement.

The ultimate goal I'm aiming at here is splitting each component
(demuxer, decoder, filtergraph, encoder, muxer) into a separate object
with clearly defined interfaces. This will allow implementing certain
features people want, such as
- multithreading invidividual components
- sending a single encoder's output to multiple muxers
- looping an encoder back to a decoder

I agree that this specific getter is not very pretty (better suggestions
are welcome). But it's hard enough to move forward here at all, if I had
to worry about every commit smelling extra sweet I would never get
anywhere at all.

> If your patchset were already applied and someone posted the reverse
> of patch #15, I'd LGTM it, because it makes checking for the bitexact
> flag local to the place where it is used and thereby avoids storing
> these variables in a context for only one usage.

One reason why the bitexact refactor is an improvement on its own
(disregarding the above considerations), is that currently
set_encoder_id() needs to know how exactly the bitexact flags can be set
(either through -f[f]lags bitexact or -bitexact -- that's why it
initializes codec_flags to ost->enc_ctx->flags). With my patch, that
logic is concentrated into options parsing, which is where it belongs.

> I also think it is more natural to store each OutputStream's queue in
> the OutputStream instead of adding a new array to your struct Muxer.

The reasons for why it's done this way are in the commit message.

-- 
Anton Khirnov
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 19/24] ffmpeg: fix initial muxing queue size

[Andreas Rheinhardt]

Anton Khirnov:
> It stores pointers to AVPacket, not AVPackets themselves.
> ---
>  fftools/ffmpeg_mux.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fftools/ffmpeg_mux.c b/fftools/ffmpeg_mux.c
> index 5a12a1c899..9281e6c94c 100644
> --- a/fftools/ffmpeg_mux.c
> +++ b/fftools/ffmpeg_mux.c
> @@ -411,7 +411,7 @@ int of_muxer_init(OutputFile *of, uint64_t limit_filesize)
>  
>      for (int i = 0; i < of->ctx->nb_streams; i++) {
>          MuxStream *ms = &mux->streams[i];
> -        ms->muxing_queue = av_fifo_alloc(8 * sizeof(AVPacket));
> +        ms->muxing_queue = av_fifo_alloc(8 * sizeof(AVPacket*));
>          if (!ms->muxing_queue) {
>              ret = AVERROR(ENOMEM);
>              goto fail;
> 

LGTM.

- 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".

Re: [FFmpeg-devel] [PATCH 21/24] ffmpeg_mux: split of_write_packet()

[Andreas Rheinhardt]

Anton Khirnov:
> It is currently called from two places:
> - output_packet() in ffmpeg.c, which submits the newly available output
>   packet to the muxer
> - from of_check_init() in ffmpeg_mux.c after the header has been
>   written, to flush the muxing queue
> 
> Some packets will thus be processed by this function twice, so it
> requires an extra parameter to indicate the place it is called from and
> avoid modifying some state twice.
> 
> This is fragile and hard to follow, so split this function into two.
> Also rename of_write_packet() to of_submit_packet() to better reflect
> its new purpose.
> ---
>  fftools/ffmpeg.c     |  4 +--
>  fftools/ffmpeg.h     |  3 +--
>  fftools/ffmpeg_mux.c | 63 ++++++++++++++++++++++++--------------------
>  3 files changed, 37 insertions(+), 33 deletions(-)
> 
> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> index e9a5c0f523..bbedf867b4 100644
> --- a/fftools/ffmpeg.c
> +++ b/fftools/ffmpeg.c
> @@ -728,11 +728,11 @@ static void output_packet(OutputFile *of, AVPacket *pkt,
>          if (ret < 0)
>              goto finish;
>          while ((ret = av_bsf_receive_packet(ost->bsf_ctx, pkt)) >= 0)
> -            of_write_packet(of, pkt, ost, 0);
> +            of_submit_packet(of, pkt, ost);
>          if (ret == AVERROR(EAGAIN))
>              ret = 0;
>      } else if (!eof)
> -        of_write_packet(of, pkt, ost, 0);
> +        of_submit_packet(of, pkt, ost);
>  
>  finish:
>      if (ret < 0 && ret != AVERROR_EOF) {
> diff --git a/fftools/ffmpeg.h b/fftools/ffmpeg.h
> index e6e472f994..76c8dfa4c8 100644
> --- a/fftools/ffmpeg.h
> +++ b/fftools/ffmpeg.h
> @@ -684,8 +684,7 @@ int of_check_init(OutputFile *of);
>  int of_write_trailer(OutputFile *of);
>  void of_close(OutputFile **pof);
>  
> -void of_write_packet(OutputFile *of, AVPacket *pkt, OutputStream *ost,
> -                     int unqueue);
> +void of_submit_packet(OutputFile *of, AVPacket *pkt, OutputStream *ost);
>  int of_finished(OutputFile *of);
>  int64_t of_bytes_written(OutputFile *of);
>  AVChapter * const *
> diff --git a/fftools/ffmpeg_mux.c b/fftools/ffmpeg_mux.c
> index d4b674c9e2..e97ec8ab93 100644
> --- a/fftools/ffmpeg_mux.c
> +++ b/fftools/ffmpeg_mux.c
> @@ -102,39 +102,12 @@ static int queue_packet(OutputFile *of, OutputStream *ost, AVPacket *pkt)
>      return 0;
>  }
>  
> -void of_write_packet(OutputFile *of, AVPacket *pkt, OutputStream *ost,
> -                     int unqueue)
> +static void write_packet(OutputFile *of, OutputStream *ost, AVPacket *pkt)
>  {
>      AVFormatContext *s = of->ctx;
>      AVStream *st = ost->st;
>      int ret;
>  
> -    /*
> -     * Audio encoders may split the packets --  #frames in != #packets out.
> -     * But there is no reordering, so we can limit the number of output packets
> -     * by simply dropping them here.
> -     * Counting encoded video frames needs to be done separately because of
> -     * reordering, see do_video_out().
> -     * Do not count the packet when unqueued because it has been counted when queued.
> -     */
> -    if (!(st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO && ost->encoding_needed) && !unqueue) {
> -        if (ost->frame_number >= ost->max_frames) {
> -            av_packet_unref(pkt);
> -            return;
> -        }
> -        ost->frame_number++;
> -    }

Factoring this chunk out of write_packet() (effectively inlining
unqueue) looks good (and I actually pondered it myself),

> -
> -    /* the muxer is not initialized yet, buffer the packet */
> -    if (!of->mux->header_written) {
> -        ret = queue_packet(of, ost, pkt);
> -        if (ret < 0) {
> -            av_packet_unref(pkt);
> -            exit_program(1);
> -        }
> -        return;
> -    }
> -

but I could not prove that the header has already been written in case
unqueue == 0. Can you guarantee this to be so and explain it to me?

>      if ((st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO && video_sync_method == VSYNC_DROP) ||
>          (st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO && audio_sync_method < 0))
>          pkt->pts = pkt->dts = AV_NOPTS_VALUE;
> @@ -225,6 +198,38 @@ void of_write_packet(OutputFile *of, AVPacket *pkt, OutputStream *ost,
>      }
>  }
>  
> +void of_submit_packet(OutputFile *of, AVPacket *pkt, OutputStream *ost)
> +{
> +    AVStream *st = ost->st;
> +    int ret;
> +
> +    /*
> +     * Audio encoders may split the packets --  #frames in != #packets out.
> +     * But there is no reordering, so we can limit the number of output packets
> +     * by simply dropping them here.
> +     * Counting encoded video frames needs to be done separately because of
> +     * reordering, see do_video_out().
> +     */
> +    if (!(st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO && ost->encoding_needed)) {
> +        if (ost->frame_number >= ost->max_frames) {
> +            av_packet_unref(pkt);
> +            return;
> +        }
> +        ost->frame_number++;
> +    }
> +
> +    if (of->mux->header_written) {
> +        write_packet(of, ost, pkt);
> +    } else {
> +        /* the muxer is not initialized yet, buffer the packet */
> +        ret = queue_packet(of, ost, pkt);
> +        if (ret < 0) {
> +            av_packet_unref(pkt);
> +            exit_program(1);
> +        }
> +    }
> +}
> +
>  static int print_sdp(void)
>  {
>      char sdp[16384];
> @@ -324,7 +329,7 @@ int of_check_init(OutputFile *of)
>              AVPacket *pkt;
>              av_fifo_generic_read(ms->muxing_queue, &pkt, sizeof(pkt), NULL);
>              ms->muxing_queue_data_size -= pkt->size;
> -            of_write_packet(of, pkt, ost, 1);
> +            write_packet(of, ost, pkt);
>              av_packet_free(&pkt);
>          }
>      }
> 

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 21/24] ffmpeg_mux: split of_write_packet()

[Anton Khirnov]

Quoting Andreas Rheinhardt (2021-12-17 00:42:25)
> Anton Khirnov:
> > diff --git a/fftools/ffmpeg_mux.c b/fftools/ffmpeg_mux.c
> > index d4b674c9e2..e97ec8ab93 100644
> > --- a/fftools/ffmpeg_mux.c
> > +++ b/fftools/ffmpeg_mux.c
> > @@ -102,39 +102,12 @@ static int queue_packet(OutputFile *of, OutputStream *ost, AVPacket *pkt)
> >      return 0;
> >  }
> >  
> > -void of_write_packet(OutputFile *of, AVPacket *pkt, OutputStream *ost,
> > -                     int unqueue)
> > +static void write_packet(OutputFile *of, OutputStream *ost, AVPacket *pkt)
> >  {
> >      AVFormatContext *s = of->ctx;
> >      AVStream *st = ost->st;
> >      int ret;
> >  
> > -    /*
> > -     * Audio encoders may split the packets --  #frames in != #packets out.
> > -     * But there is no reordering, so we can limit the number of output packets
> > -     * by simply dropping them here.
> > -     * Counting encoded video frames needs to be done separately because of
> > -     * reordering, see do_video_out().
> > -     * Do not count the packet when unqueued because it has been counted when queued.
> > -     */
> > -    if (!(st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO && ost->encoding_needed) && !unqueue) {
> > -        if (ost->frame_number >= ost->max_frames) {
> > -            av_packet_unref(pkt);
> > -            return;
> > -        }
> > -        ost->frame_number++;
> > -    }
> 
> Factoring this chunk out of write_packet() (effectively inlining
> unqueue) looks good (and I actually pondered it myself),
> 
> > -
> > -    /* the muxer is not initialized yet, buffer the packet */
> > -    if (!of->mux->header_written) {
> > -        ret = queue_packet(of, ost, pkt);
> > -        if (ret < 0) {
> > -            av_packet_unref(pkt);
> > -            exit_program(1);
> > -        }
> > -        return;
> > -    }
> > -
> 
> but I could not prove that the header has already been written in case
> unqueue == 0. Can you guarantee this to be so and explain it to me?

I don't understand what you mean. unqueue == 0 in the current code tells
you nothing about whether the header has been written.
How does that relate to the patch?

-- 
Anton Khirnov
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 21/24] ffmpeg_mux: split of_write_packet()

[Andreas Rheinhardt]

Anton Khirnov:
> Quoting Andreas Rheinhardt (2021-12-17 00:42:25)
>> Anton Khirnov:
>>> diff --git a/fftools/ffmpeg_mux.c b/fftools/ffmpeg_mux.c
>>> index d4b674c9e2..e97ec8ab93 100644
>>> --- a/fftools/ffmpeg_mux.c
>>> +++ b/fftools/ffmpeg_mux.c
>>> @@ -102,39 +102,12 @@ static int queue_packet(OutputFile *of, OutputStream *ost, AVPacket *pkt)
>>>      return 0;
>>>  }
>>>  
>>> -void of_write_packet(OutputFile *of, AVPacket *pkt, OutputStream *ost,
>>> -                     int unqueue)
>>> +static void write_packet(OutputFile *of, OutputStream *ost, AVPacket *pkt)
>>>  {
>>>      AVFormatContext *s = of->ctx;
>>>      AVStream *st = ost->st;
>>>      int ret;
>>>  
>>> -    /*
>>> -     * Audio encoders may split the packets --  #frames in != #packets out.
>>> -     * But there is no reordering, so we can limit the number of output packets
>>> -     * by simply dropping them here.
>>> -     * Counting encoded video frames needs to be done separately because of
>>> -     * reordering, see do_video_out().
>>> -     * Do not count the packet when unqueued because it has been counted when queued.
>>> -     */
>>> -    if (!(st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO && ost->encoding_needed) && !unqueue) {
>>> -        if (ost->frame_number >= ost->max_frames) {
>>> -            av_packet_unref(pkt);
>>> -            return;
>>> -        }
>>> -        ost->frame_number++;
>>> -    }
>>
>> Factoring this chunk out of write_packet() (effectively inlining
>> unqueue) looks good (and I actually pondered it myself),
>>
>>> -
>>> -    /* the muxer is not initialized yet, buffer the packet */
>>> -    if (!of->mux->header_written) {
>>> -        ret = queue_packet(of, ost, pkt);
>>> -        if (ret < 0) {
>>> -            av_packet_unref(pkt);
>>> -            exit_program(1);
>>> -        }
>>> -        return;
>>> -    }
>>> -
>>
>> but I could not prove that the header has already been written in case
>> unqueue == 0. Can you guarantee this to be so and explain it to me?
> 
> I don't understand what you mean. unqueue == 0 in the current code tells
> you nothing about whether the header has been written.
> How does that relate to the patch?
> 

Wait, I misread this: You keep the header_written check for the two
callers in output_packet(), but remove it for the one caller for which
we know that initialization has already happened. This is good. Sorry
for the confusion.

- 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".

Re: [FFmpeg-devel] [PATCH 11/24] ffmpeg: set want_sdp when initializing the muxer

[Andreas Rheinhardt]

Anton Khirnov:
> Allows making the variable local to ffmpeg_mux.
> ---
>  fftools/ffmpeg.c     | 9 +--------
>  fftools/ffmpeg.h     | 1 -
>  fftools/ffmpeg_mux.c | 5 +++++
>  3 files changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> index 3ed1201fda..f76e5df8d2 100644
> --- a/fftools/ffmpeg.c
> +++ b/fftools/ffmpeg.c
> @@ -138,8 +138,6 @@ static int nb_frames_drop = 0;
>  static int64_t decode_error_stat[2];
>  unsigned nb_output_dumped = 0;
>  
> -int want_sdp = 1;
> -
>  static BenchmarkTimeStamps current_time;
>  AVIOContext *progress_avio = NULL;
>  
> @@ -4557,7 +4555,7 @@ static void log_callback_null(void *ptr, int level, const char *fmt, va_list vl)
>  
>  int main(int argc, char **argv)
>  {
> -    int i, ret;
> +    int ret;
>      BenchmarkTimeStamps ti;
>  
>      init_dynload();
> @@ -4600,11 +4598,6 @@ int main(int argc, char **argv)
>          exit_program(1);
>      }
>  
> -    for (i = 0; i < nb_output_files; i++) {
> -        if (strcmp(output_files[i]->format->name, "rtp"))
> -            want_sdp = 0;
> -    }
> -
>      current_time = ti = get_benchmark_time_stamps();
>      if (transcode() < 0)
>          exit_program(1);
> diff --git a/fftools/ffmpeg.h b/fftools/ffmpeg.h
> index 67ff391334..4aed24c2a7 100644
> --- a/fftools/ffmpeg.h
> +++ b/fftools/ffmpeg.h
> @@ -646,7 +646,6 @@ extern char *qsv_device;
>  #endif
>  extern HWDevice *filter_hw_device;
>  
> -extern int want_sdp;
>  extern unsigned nb_output_dumped;
>  extern int main_return_code;
>  
> diff --git a/fftools/ffmpeg_mux.c b/fftools/ffmpeg_mux.c
> index aba4b563a4..e6417cdb8c 100644
> --- a/fftools/ffmpeg_mux.c
> +++ b/fftools/ffmpeg_mux.c
> @@ -38,6 +38,8 @@ struct Muxer {
>      int header_written;
>  };
>  
> +static int want_sdp = 1;
> +
>  static void close_all_output_streams(OutputStream *ost, OSTFinished this_stream, OSTFinished others)
>  {
>      int i;
> @@ -349,6 +351,9 @@ int of_muxer_init(OutputFile *of, uint64_t limit_filesize)
>  
>      mux->limit_filesize = limit_filesize;
>  
> +    if (strcmp(of->format->name, "rtp"))
> +        want_sdp = 0;
> +
>      return 0;
>  }
>  
> 

This variable could actually be completely local to
check_init_output_file()/of_check_init().

- 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".

Re: [FFmpeg-devel] [PATCH 11/24] ffmpeg: set want_sdp when initializing the muxer

[Anton Khirnov]

Quoting Andreas Rheinhardt (2021-12-16 22:40:31)
> Anton Khirnov:
> > Allows making the variable local to ffmpeg_mux.
> > ---
> >  fftools/ffmpeg.c     | 9 +--------
> >  fftools/ffmpeg.h     | 1 -
> >  fftools/ffmpeg_mux.c | 5 +++++
> >  3 files changed, 6 insertions(+), 9 deletions(-)
> > 
> > diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> > index 3ed1201fda..f76e5df8d2 100644
> > --- a/fftools/ffmpeg.c
> > +++ b/fftools/ffmpeg.c
> > @@ -138,8 +138,6 @@ static int nb_frames_drop = 0;
> >  static int64_t decode_error_stat[2];
> >  unsigned nb_output_dumped = 0;
> >  
> > -int want_sdp = 1;
> > -
> >  static BenchmarkTimeStamps current_time;
> >  AVIOContext *progress_avio = NULL;
> >  
> > @@ -4557,7 +4555,7 @@ static void log_callback_null(void *ptr, int level, const char *fmt, va_list vl)
> >  
> >  int main(int argc, char **argv)
> >  {
> > -    int i, ret;
> > +    int ret;
> >      BenchmarkTimeStamps ti;
> >  
> >      init_dynload();
> > @@ -4600,11 +4598,6 @@ int main(int argc, char **argv)
> >          exit_program(1);
> >      }
> >  
> > -    for (i = 0; i < nb_output_files; i++) {
> > -        if (strcmp(output_files[i]->format->name, "rtp"))
> > -            want_sdp = 0;
> > -    }
> > -
> >      current_time = ti = get_benchmark_time_stamps();
> >      if (transcode() < 0)
> >          exit_program(1);
> > diff --git a/fftools/ffmpeg.h b/fftools/ffmpeg.h
> > index 67ff391334..4aed24c2a7 100644
> > --- a/fftools/ffmpeg.h
> > +++ b/fftools/ffmpeg.h
> > @@ -646,7 +646,6 @@ extern char *qsv_device;
> >  #endif
> >  extern HWDevice *filter_hw_device;
> >  
> > -extern int want_sdp;
> >  extern unsigned nb_output_dumped;
> >  extern int main_return_code;
> >  
> > diff --git a/fftools/ffmpeg_mux.c b/fftools/ffmpeg_mux.c
> > index aba4b563a4..e6417cdb8c 100644
> > --- a/fftools/ffmpeg_mux.c
> > +++ b/fftools/ffmpeg_mux.c
> > @@ -38,6 +38,8 @@ struct Muxer {
> >      int header_written;
> >  };
> >  
> > +static int want_sdp = 1;
> > +
> >  static void close_all_output_streams(OutputStream *ost, OSTFinished this_stream, OSTFinished others)
> >  {
> >      int i;
> > @@ -349,6 +351,9 @@ int of_muxer_init(OutputFile *of, uint64_t limit_filesize)
> >  
> >      mux->limit_filesize = limit_filesize;
> >  
> > +    if (strcmp(of->format->name, "rtp"))
> > +        want_sdp = 0;
> > +
> >      return 0;
> >  }
> >  
> > 
> 
> This variable could actually be completely local to
> check_init_output_file()/of_check_init().

If we are to have global variables at all (I'd prefer we didn't), I'd
rather have them visible, not hidden inside functions.

-- 
Anton Khirnov
_______________________________________________
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".