Re: [FFmpeg-devel] [PATCH 4/5] fftools/ffmpeg: flush and recreate encoder instance if resolution changes
> From: ffmpeg-devel On Behalf Of > Nicolas George > Sent: Friday, June 12, 2020 01:52 > To: FFmpeg development discussions and patches de...@ffmpeg.org> > Subject: Re: [FFmpeg-devel] [PATCH 4/5] fftools/ffmpeg: flush and recreate > encoder instance if resolution changes > > Fu, Linjie (12020-06-11): > > If rawvideo here means .h264, attached the output file produced by libx264: > > Now, rawvideo means rawvideo. > For Raw YUV video, it is not able to notify player/user about the resolution changing since it only contains data information. The usage for raw video is discussed previously. For now we'd like to use rawvideo encoder To do some bit-exact compare to make sure the result of decoder is good without scaling. (matches the output of other media decoder component like gstreamer) - Linjie ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 4/5] fftools/ffmpeg: flush and recreate encoder instance if resolution changes
Fu, Linjie (12020-06-11): > If rawvideo here means .h264, attached the output file produced by libx264: Now, rawvideo means rawvideo. Regards, -- Nicolas George signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 4/5] fftools/ffmpeg: flush and recreate encoder instance if resolution changes
Fu, Linjie (12020-06-11): > Indeed, tested with .mp4 and .h264 for encoder libx264, the results are > playable. > (discarding global header if resolution changing is allowed, then we can keep > the > Sequence header in each Key frame and make the resolution changing noticeable) > > Please help to comment, thx. > https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=1470 Did it produce a rawvideo stream that can be decoded? I doubt it. Regards, -- Nicolas George signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 4/5] fftools/ffmpeg: flush and recreate encoder instance if resolution changes
> From: Nicolas George > Sent: Wednesday, June 10, 2020 19:54 > To: FFmpeg development discussions and patches de...@ffmpeg.org> > Cc: Fu, Linjie > Subject: Re: [FFmpeg-devel] [PATCH 4/5] fftools/ffmpeg: flush and recreate > encoder instance if resolution changes > > Linjie Fu (12020-06-09): > > Signed-off-by: Linjie Fu > > --- > > Should be squashed with: > > https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=1434 > > Apart from the issue of accessing non-public fields, this needs to be > intensively tested. If it allows to create unplayable files, then it is > not acceptable. It should be tested with codecs that use extradata (x264 > for example) and also with rawvideo. > Indeed, tested with .mp4 and .h264 for encoder libx264, the results are playable. (discarding global header if resolution changing is allowed, then we can keep the Sequence header in each Key frame and make the resolution changing noticeable) Please help to comment, thx. https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=1470 - Linjie ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 4/5] fftools/ffmpeg: flush and recreate encoder instance if resolution changes
> From: ffmpeg-devel On Behalf Of
> Anton Khirnov
> Sent: Wednesday, June 10, 2020 16:31
> To: FFmpeg development discussions and patches de...@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 4/5] fftools/ffmpeg: flush and recreate
> encoder instance if resolution changes
>
> Quoting Linjie Fu (2020-06-09 10:48:46)
> > Signed-off-by: Linjie Fu
> > ---
> > Should be squashed with:
> > https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=1434
> >
> > fftools/ffmpeg.c | 11 +++
> > 1 file changed, 11 insertions(+)
> >
> > diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> > index 5859781..8cdd532 100644
> > --- a/fftools/ffmpeg.c
> > +++ b/fftools/ffmpeg.c
> > @@ -130,6 +130,7 @@ static void do_video_stats(OutputStream *ost, int
> frame_size);
> > static BenchmarkTimeStamps get_benchmark_time_stamps(void);
> > static int64_t getmaxrss(void);
> > static int ifilter_has_all_input_formats(FilterGraph *fg);
> > +static void flush_encoders(void);
> >
> > static int run_as_daemon = 0;
> > static int nb_frames_dup = 0;
> > @@ -1058,11 +1059,21 @@ static void do_video_out(OutputFile *of,
> >
> > if (next_picture && (enc->width != next_picture->width ||
> > enc->height != next_picture->height)) {
> > +flush_encoders();
> > +avcodec_flush_buffers(enc);
> > if (!(enc->codec->capabilities &
> AV_CODEC_CAP_VARIABLE_DIMENSIONS)) {
> > av_log(NULL, AV_LOG_ERROR, "Variable dimension encoding "
> > "is not supported by %s.\n", enc->codec->name);
> > goto error;
> > }
> > +
> > +enc->width = next_picture->width;
> > +enc->height = next_picture->height;
> > +
> > +if (enc->codec->close(enc) < 0)
> > +goto error;
> > +if (enc->codec->init(enc) < 0)
> > +goto error;
>
> Absolutely not.
> Those are private fields, they must not be accessed by libavcodec
> callers.
>
> What I meant was freeing the encoder and creating a completely new
> encoder instance.
Got it and updated, please help to comment, thx.
https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=1470
- Linjie
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 4/5] fftools/ffmpeg: flush and recreate encoder instance if resolution changes
Linjie Fu (12020-06-09): > Signed-off-by: Linjie Fu > --- > Should be squashed with: > https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=1434 Apart from the issue of accessing non-public fields, this needs to be intensively tested. If it allows to create unplayable files, then it is not acceptable. It should be tested with codecs that use extradata (x264 for example) and also with rawvideo. Regards, -- Nicolas George signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 4/5] fftools/ffmpeg: flush and recreate encoder instance if resolution changes
Quoting Linjie Fu (2020-06-09 10:48:46)
> Signed-off-by: Linjie Fu
> ---
> Should be squashed with:
> https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=1434
>
> fftools/ffmpeg.c | 11 +++
> 1 file changed, 11 insertions(+)
>
> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> index 5859781..8cdd532 100644
> --- a/fftools/ffmpeg.c
> +++ b/fftools/ffmpeg.c
> @@ -130,6 +130,7 @@ static void do_video_stats(OutputStream *ost, int
> frame_size);
> static BenchmarkTimeStamps get_benchmark_time_stamps(void);
> static int64_t getmaxrss(void);
> static int ifilter_has_all_input_formats(FilterGraph *fg);
> +static void flush_encoders(void);
>
> static int run_as_daemon = 0;
> static int nb_frames_dup = 0;
> @@ -1058,11 +1059,21 @@ static void do_video_out(OutputFile *of,
>
> if (next_picture && (enc->width != next_picture->width ||
> enc->height != next_picture->height)) {
> +flush_encoders();
> +avcodec_flush_buffers(enc);
> if (!(enc->codec->capabilities & AV_CODEC_CAP_VARIABLE_DIMENSIONS)) {
> av_log(NULL, AV_LOG_ERROR, "Variable dimension encoding "
> "is not supported by %s.\n", enc->codec->name);
> goto error;
> }
> +
> +enc->width = next_picture->width;
> +enc->height = next_picture->height;
> +
> +if (enc->codec->close(enc) < 0)
> +goto error;
> +if (enc->codec->init(enc) < 0)
> +goto error;
Absolutely not.
Those are private fields, they must not be accessed by libavcodec
callers.
What I meant was freeing the encoder and creating a completely new
encoder instance.
--
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-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 4/5] fftools/ffmpeg: flush and recreate encoder instance if resolution changes
> From: ffmpeg-devel On Behalf Of
> James Almer
> Sent: Wednesday, June 10, 2020 12:22
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH 4/5] fftools/ffmpeg: flush and recreate
> encoder instance if resolution changes
>
> On 6/9/2020 5:48 AM, Linjie Fu wrote:
> > Signed-off-by: Linjie Fu
> > ---
> > Should be squashed with:
> > https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=1434
> >
> > fftools/ffmpeg.c | 11 +++
> > 1 file changed, 11 insertions(+)
> >
> > diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> > index 5859781..8cdd532 100644
> > --- a/fftools/ffmpeg.c
> > +++ b/fftools/ffmpeg.c
> > @@ -130,6 +130,7 @@ static void do_video_stats(OutputStream *ost, int
> frame_size);
> > static BenchmarkTimeStamps get_benchmark_time_stamps(void);
> > static int64_t getmaxrss(void);
> > static int ifilter_has_all_input_formats(FilterGraph *fg);
> > +static void flush_encoders(void);
> >
> > static int run_as_daemon = 0;
> > static int nb_frames_dup = 0;
> > @@ -1058,11 +1059,21 @@ static void do_video_out(OutputFile *of,
> >
> > if (next_picture && (enc->width != next_picture->width ||
> > enc->height != next_picture->height)) {
> > +flush_encoders();
> > +avcodec_flush_buffers(enc);
>
> This only works for a limited amount of encoders that set the
> AV_CODEC_CAP_ENCODER_FLUSH codec capability, and it's a no-op after
> emitting a warning otherwise.
Yes, hence would like to declare VARIABLE_DIMENSIONS and ENCODER_FLUSH
capability for encoders as well:
https://patchwork.ffmpeg.org/project/ffmpeg/patch/1591692568-19385-1-git-send-email-linjie...@intel.com/
- Linjie
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 4/5] fftools/ffmpeg: flush and recreate encoder instance if resolution changes
On 6/9/2020 5:48 AM, Linjie Fu wrote:
> Signed-off-by: Linjie Fu
> ---
> Should be squashed with:
> https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=1434
>
> fftools/ffmpeg.c | 11 +++
> 1 file changed, 11 insertions(+)
>
> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> index 5859781..8cdd532 100644
> --- a/fftools/ffmpeg.c
> +++ b/fftools/ffmpeg.c
> @@ -130,6 +130,7 @@ static void do_video_stats(OutputStream *ost, int
> frame_size);
> static BenchmarkTimeStamps get_benchmark_time_stamps(void);
> static int64_t getmaxrss(void);
> static int ifilter_has_all_input_formats(FilterGraph *fg);
> +static void flush_encoders(void);
>
> static int run_as_daemon = 0;
> static int nb_frames_dup = 0;
> @@ -1058,11 +1059,21 @@ static void do_video_out(OutputFile *of,
>
> if (next_picture && (enc->width != next_picture->width ||
> enc->height != next_picture->height)) {
> +flush_encoders();
> +avcodec_flush_buffers(enc);
This only works for a limited amount of encoders that set the
AV_CODEC_CAP_ENCODER_FLUSH codec capability, and it's a no-op after
emitting a warning otherwise.
> if (!(enc->codec->capabilities & AV_CODEC_CAP_VARIABLE_DIMENSIONS)) {
> av_log(NULL, AV_LOG_ERROR, "Variable dimension encoding "
> "is not supported by %s.\n", enc->codec->name);
> goto error;
> }
> +
> +enc->width = next_picture->width;
> +enc->height = next_picture->height;
> +
> +if (enc->codec->close(enc) < 0)
> +goto error;
> +if (enc->codec->init(enc) < 0)
> +goto error;
> }
>
> if (ost->source_index >= 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-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 4/5] fftools/ffmpeg: flush and recreate encoder instance if resolution changes
> From: Devin Heitmueller
> Sent: Tuesday, June 9, 2020 23:47
> To: FFmpeg development discussions and patches de...@ffmpeg.org>
> Cc: Fu, Linjie
> Subject: Re: [FFmpeg-devel] [PATCH 4/5] fftools/ffmpeg: flush and recreate
> encoder instance if resolution changes
>
> On Tue, Jun 9, 2020 at 4:53 AM Linjie Fu wrote:
> >
> > Signed-off-by: Linjie Fu
> > ---
> > Should be squashed with:
> > https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=1434
> >
> > fftools/ffmpeg.c | 11 +++
> > 1 file changed, 11 insertions(+)
> >
> > diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> > index 5859781..8cdd532 100644
> > --- a/fftools/ffmpeg.c
> > +++ b/fftools/ffmpeg.c
> > @@ -130,6 +130,7 @@ static void do_video_stats(OutputStream *ost, int
> frame_size);
> > static BenchmarkTimeStamps get_benchmark_time_stamps(void);
> > static int64_t getmaxrss(void);
> > static int ifilter_has_all_input_formats(FilterGraph *fg);
> > +static void flush_encoders(void);
> >
> > static int run_as_daemon = 0;
> > static int nb_frames_dup = 0;
> > @@ -1058,11 +1059,21 @@ static void do_video_out(OutputFile *of,
> >
> > if (next_picture && (enc->width != next_picture->width ||
> > enc->height != next_picture->height)) {
> > +flush_encoders();
> > +avcodec_flush_buffers(enc);
> > if (!(enc->codec->capabilities &
> AV_CODEC_CAP_VARIABLE_DIMENSIONS)) {
> > av_log(NULL, AV_LOG_ERROR, "Variable dimension encoding "
> > "is not supported by %s.\n", enc->codec->name);
> > goto error;
> > }
> > +
> > +enc->width = next_picture->width;
> > +enc->height = next_picture->height;
>
> Perhaps from a workflow standpoint it makes more sense to move the
> code which changes the encoder parameters to after where you close the
> existing encoder (i.e. between the close and init calls). I can't
> think of a specific case where this might break a downstream encoder,
That's the reason I simply set the width/height ahead.
> but it seems like a good practice to only have the parameters applied
> to the new encoder instance.
Indeed, fixed locally.
> > +
> > +if (enc->codec->close(enc) < 0)
> > +goto error;
> > +if (enc->codec->init(enc) < 0)
> > +goto error;
> > }
> >
> > if (ost->source_index >= 0)
>
> In general do we really think this is a safe thing to do? Does
> something also need to be propagated to the output as well? I know
> that this would break use cases like the decklink output where the
> frame resolution suddenly changes in the middle of the stream without
> calling the output's write_header() routine.
Yes, noticed the constraints in sequence header and container.
Since resolution changing is allowed in single bitstream, one global header is
not
enough anymore as Nicolas has pointed out in [1].
Hence as to the container level, for the formats with AVFMT_GLOBALHEADER flag,
we should place sps/pps information in key frame instead of in extradata.
(e.g. disable AV_CODEC_FLAG_GLOBAL_HEADER)
-if (oc->oformat->flags & AVFMT_GLOBALHEADER)
+if (oc->oformat->flags & AVFMT_GLOBALHEADER && ost->autoscale)
ost->enc_ctx->flags |= AV_CODEC_FLAG_GLOBAL_HEADER;
Verified this works, at least for containers like mp4.
- Linjie
[1]
https://patchwork.ffmpeg.org/project/ffmpeg/patch/1591606685-4450-1-git-send-email-linjie...@intel.com/#55293
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 4/5] fftools/ffmpeg: flush and recreate encoder instance if resolution changes
On Tue, Jun 9, 2020 at 4:53 AM Linjie Fu wrote:
>
> Signed-off-by: Linjie Fu
> ---
> Should be squashed with:
> https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=1434
>
> fftools/ffmpeg.c | 11 +++
> 1 file changed, 11 insertions(+)
>
> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> index 5859781..8cdd532 100644
> --- a/fftools/ffmpeg.c
> +++ b/fftools/ffmpeg.c
> @@ -130,6 +130,7 @@ static void do_video_stats(OutputStream *ost, int
> frame_size);
> static BenchmarkTimeStamps get_benchmark_time_stamps(void);
> static int64_t getmaxrss(void);
> static int ifilter_has_all_input_formats(FilterGraph *fg);
> +static void flush_encoders(void);
>
> static int run_as_daemon = 0;
> static int nb_frames_dup = 0;
> @@ -1058,11 +1059,21 @@ static void do_video_out(OutputFile *of,
>
> if (next_picture && (enc->width != next_picture->width ||
> enc->height != next_picture->height)) {
> +flush_encoders();
> +avcodec_flush_buffers(enc);
> if (!(enc->codec->capabilities & AV_CODEC_CAP_VARIABLE_DIMENSIONS)) {
> av_log(NULL, AV_LOG_ERROR, "Variable dimension encoding "
> "is not supported by %s.\n", enc->codec->name);
> goto error;
> }
> +
> +enc->width = next_picture->width;
> +enc->height = next_picture->height;
Perhaps from a workflow standpoint it makes more sense to move the
code which changes the encoder parameters to after where you close the
existing encoder (i.e. between the close and init calls). I can't
think of a specific case where this might break a downstream encoder,
but it seems like a good practice to only have the parameters applied
to the new encoder instance.
> +
> +if (enc->codec->close(enc) < 0)
> +goto error;
> +if (enc->codec->init(enc) < 0)
> +goto error;
> }
>
> if (ost->source_index >= 0)
In general do we really think this is a safe thing to do? Does
something also need to be propagated to the output as well? I know
that this would break use cases like the decklink output where the
frame resolution suddenly changes in the middle of the stream without
calling the output's write_header() routine.
Devin
--
Devin Heitmueller, Senior Software Engineer
LTN Global Communications
o: +1 (301) 363-1001
w: https://ltnglobal.com e: devin.heitmuel...@ltnglobal.com
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH 4/5] fftools/ffmpeg: flush and recreate encoder instance if resolution changes
Signed-off-by: Linjie Fu
---
Should be squashed with:
https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=1434
fftools/ffmpeg.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
index 5859781..8cdd532 100644
--- a/fftools/ffmpeg.c
+++ b/fftools/ffmpeg.c
@@ -130,6 +130,7 @@ static void do_video_stats(OutputStream *ost, int
frame_size);
static BenchmarkTimeStamps get_benchmark_time_stamps(void);
static int64_t getmaxrss(void);
static int ifilter_has_all_input_formats(FilterGraph *fg);
+static void flush_encoders(void);
static int run_as_daemon = 0;
static int nb_frames_dup = 0;
@@ -1058,11 +1059,21 @@ static void do_video_out(OutputFile *of,
if (next_picture && (enc->width != next_picture->width ||
enc->height != next_picture->height)) {
+flush_encoders();
+avcodec_flush_buffers(enc);
if (!(enc->codec->capabilities & AV_CODEC_CAP_VARIABLE_DIMENSIONS)) {
av_log(NULL, AV_LOG_ERROR, "Variable dimension encoding "
"is not supported by %s.\n", enc->codec->name);
goto error;
}
+
+enc->width = next_picture->width;
+enc->height = next_picture->height;
+
+if (enc->codec->close(enc) < 0)
+goto error;
+if (enc->codec->init(enc) < 0)
+goto error;
}
if (ost->source_index >= 0)
--
2.7.4
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
