[FFmpeg-devel] [PATCH] avcodec/nvenc: Fix segfault with intra-only
In intra-only mode, frameIntervalP is 0, which means the frame data array is smaller than the number of surfaces. This causes a crash when closing the encoder. Fix this by making sure the frame data array is at least as big as the number of surfaces. --- libavcodec/nvenc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libavcodec/nvenc.c b/libavcodec/nvenc.c index a9945355ba..93e87b21db 100644 --- a/libavcodec/nvenc.c +++ b/libavcodec/nvenc.c @@ -1021,7 +1021,7 @@ static av_cold int nvenc_recalc_surfaces(AVCodecContext *avctx) // Output in the worst case will only start when the surface buffer is completely full. // Hence we need to keep at least the max amount of surfaces plus the max reorder delay around. -ctx->frame_data_array_nb = ctx->nb_surfaces + ctx->encode_config.frameIntervalP - 1; +ctx->frame_data_array_nb = FFMAX(ctx->nb_surfaces, ctx->nb_surfaces + ctx->encode_config.frameIntervalP - 1); return 0; } -- 2.39.2 ___ 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] avcodec/nvenc: Fix segfault with intra-only
On Thu, 20 Jun 2024 at 17:39, Josh Allmann wrote: > > In intra-only mode, frameIntervalP is 0, which means the frame > data array is smaller than the number of surfaces. This causes a > crash when closing the encoder. > > Fix this by making sure the frame data array is at least as big as > the number of surfaces. > --- > libavcodec/nvenc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavcodec/nvenc.c b/libavcodec/nvenc.c > index a9945355ba..93e87b21db 100644 > --- a/libavcodec/nvenc.c > +++ b/libavcodec/nvenc.c > @@ -1021,7 +1021,7 @@ static av_cold int nvenc_recalc_surfaces(AVCodecContext > *avctx) > > // Output in the worst case will only start when the surface buffer is > completely full. > // Hence we need to keep at least the max amount of surfaces plus the > max reorder delay around. > -ctx->frame_data_array_nb = ctx->nb_surfaces + > ctx->encode_config.frameIntervalP - 1; > +ctx->frame_data_array_nb = FFMAX(ctx->nb_surfaces, ctx->nb_surfaces + > ctx->encode_config.frameIntervalP - 1); > > return 0; > } > -- > 2.39.2 > Hello, Ping for review. This patch fixes an easily triggered crash with nvenc in intra-only mode, eg ffmpeg -i -c:v h264_nvenc -g 0 Josh ___ 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] lavfi/vf_scale_cuda: Add format option
Makes certain usages of the lavfi API easier.
---
libavfilter/vf_scale_cuda.c | 12 +++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/libavfilter/vf_scale_cuda.c b/libavfilter/vf_scale_cuda.c
index b7cdb81081..6b1ef2bb6f 100644
--- a/libavfilter/vf_scale_cuda.c
+++ b/libavfilter/vf_scale_cuda.c
@@ -81,6 +81,7 @@ typedef struct CUDAScaleContext {
char *w_expr; ///< width expression string
char *h_expr; ///< height expression string
+char *format_str;
CUcontext cu_ctx;
CUmodulecu_module;
@@ -101,7 +102,15 @@ static av_cold int cudascale_init(AVFilterContext *ctx)
{
CUDAScaleContext *s = ctx->priv;
-s->format = AV_PIX_FMT_NONE;
+if (!strcmp(s->format_str, "same")) {
+s->format = AV_PIX_FMT_NONE;
+} else {
+s->format = av_get_pix_fmt(s->format_str);
+if (s->format == AV_PIX_FMT_NONE) {
+av_log(ctx, AV_LOG_ERROR, "Unrecognized pixel format: %s\n",
s->format_str);
+return AVERROR(EINVAL);
+}
+}
s->frame = av_frame_alloc();
if (!s->frame)
return AVERROR(ENOMEM);
@@ -533,6 +542,7 @@ fail:
static const AVOption options[] = {
{ "w", "Output video width", OFFSET(w_expr), AV_OPT_TYPE_STRING,
{ .str = "iw" }, .flags = FLAGS },
{ "h", "Output video height", OFFSET(h_expr), AV_OPT_TYPE_STRING,
{ .str = "ih" }, .flags = FLAGS },
+{ "format", "Output pixel format", OFFSET(format_str), AV_OPT_TYPE_STRING,
{ .str = "same" }, .flags = FLAGS },
{ NULL },
};
--
2.17.1
___
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] lavfi/vf_scale_cuda: Add format option
On Fri, 24 May 2019 at 06:00, Timo Rothenpieler wrote:
>
> On 24/05/2019 01:49, Josh Allmann wrote:
> > Makes certain usages of the lavfi API easier.
> > ---
> > libavfilter/vf_scale_cuda.c | 12 +++-
> > 1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/libavfilter/vf_scale_cuda.c b/libavfilter/vf_scale_cuda.c
> > index b7cdb81081..6b1ef2bb6f 100644
> > --- a/libavfilter/vf_scale_cuda.c
> > +++ b/libavfilter/vf_scale_cuda.c
> > @@ -81,6 +81,7 @@ typedef struct CUDAScaleContext {
> >
> > char *w_expr; ///< width expression string
> > char *h_expr; ///< height expression string
> > +char *format_str;
> >
> > CUcontext cu_ctx;
> > CUmodulecu_module;
> > @@ -101,7 +102,15 @@ static av_cold int cudascale_init(AVFilterContext *ctx)
> > {
> > CUDAScaleContext *s = ctx->priv;
> >
> > -s->format = AV_PIX_FMT_NONE;
> > +if (!strcmp(s->format_str, "same")) {
> > +s->format = AV_PIX_FMT_NONE;
> > +} else {
> > +s->format = av_get_pix_fmt(s->format_str);
> > +if (s->format == AV_PIX_FMT_NONE) {
> > +av_log(ctx, AV_LOG_ERROR, "Unrecognized pixel format: %s\n",
> > s->format_str);
> > +return AVERROR(EINVAL);
> > +}
> > +}
> > s->frame = av_frame_alloc();
> > if (!s->frame)
> > return AVERROR(ENOMEM);
> > @@ -533,6 +542,7 @@ fail:
> > static const AVOption options[] = {
> > { "w", "Output video width", OFFSET(w_expr),
> > AV_OPT_TYPE_STRING, { .str = "iw" }, .flags = FLAGS },
> > { "h", "Output video height", OFFSET(h_expr),
> > AV_OPT_TYPE_STRING, { .str = "ih" }, .flags = FLAGS },
> > +{ "format", "Output pixel format", OFFSET(format_str),
> > AV_OPT_TYPE_STRING, { .str = "same" }, .flags = FLAGS },
> > { NULL },
> > };
>
> I'm not sure what to think about a dummy option like this. It might be
> very confusing for users to see a format option, which only accepts a
> single value, "same", and effectively does nothing.
>
Not sure I understand the issue. "same" is the default (terminology
borrowed from the scale_npp filter), and it'll assign the format to
whatever is passed in (eg, format=yuv420p assigns that).
>
> Not strictly against it, since I can see the convenience it adds when
> building command lines, but I'd like some second opinions on this.
>
Actually I'm using the API, albeit with some of lavfi conveniences to
parse filter strings. This avoids "wiring in" the output format
manually when crossing the lavfi boundary.
Here's a example that demonstrates the issue via CLI (this may
actually be a bug elsewhere?):
Broken:
ffmpeg -loglevel verbose -hwaccel cuvid -c:v h264_cuvid -i input.ts
-an -lavfi scale_cuda=w=426:h=240,hwdownload,format=yuv420p -c:v
libx264 out.ts
Working:
ffmpeg -loglevel verbose -hwaccel cuvid -c:v h264_cuvid -i input.ts
-an -lavfi scale_cuda=w=426:h=240:format=yuv420p,hwdownload,format=yuv420p
-c:v libx264 out.ts
___
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] lavfi/vf_scale_cuda: Reset frame size after acquiring from hwframe.
The first frame is scaled correctly, and subsequent frames are over-scaled / cropped since the frame data is reset with the hwframe after each invocation of the scaler. The hwframe-allocated frame has a width/height that is 32-bit aligned. The scaler uses this aligned width / height as its target, leading to "over-scaling" and then cropping of the result. To generate a broken test sample: ffmpeg -hwaccel cuvid -c:v h264_cuvid -i -an \ -lavfi scale_cuda=w=426:h=240 -c:v h264_nvenc --- Tested with NV12 and 420P inputs. Noting that YUV444P seems generally broken - both before/after this patch. libavfilter/vf_scale_cuda.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/libavfilter/vf_scale_cuda.c b/libavfilter/vf_scale_cuda.c index 6b1ef2bb6f..13eb3ad24c 100644 --- a/libavfilter/vf_scale_cuda.c +++ b/libavfilter/vf_scale_cuda.c @@ -489,6 +489,8 @@ static int cudascale_scale(AVFilterContext *ctx, AVFrame *out, AVFrame *in) av_frame_move_ref(out, s->frame); av_frame_move_ref(s->frame, s->tmp_frame); +s->frame->width = s->planes_out[0].width; +s->frame->height= s->planes_out[0].height; ret = av_frame_copy_props(out, in); if (ret < 0) -- 2.17.1 ___ 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] lavfi/vf_scale_cuda: Add format option
On Fri, 24 May 2019 at 09:55, Timo Rothenpieler wrote:
>
> On 24.05.2019 18:27, Josh Allmann wrote:
> > On Fri, 24 May 2019 at 06:00, Timo Rothenpieler
> > wrote:
> >>
> >> On 24/05/2019 01:49, Josh Allmann wrote:
> >>> Makes certain usages of the lavfi API easier.
> >>> ---
> >>>libavfilter/vf_scale_cuda.c | 12 +++-
> >>>1 file changed, 11 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/libavfilter/vf_scale_cuda.c b/libavfilter/vf_scale_cuda.c
> >>> index b7cdb81081..6b1ef2bb6f 100644
> >>> --- a/libavfilter/vf_scale_cuda.c
> >>> +++ b/libavfilter/vf_scale_cuda.c
> >>> @@ -81,6 +81,7 @@ typedef struct CUDAScaleContext {
> >>>
> >>>char *w_expr; ///< width expression string
> >>>char *h_expr; ///< height expression string
> >>> +char *format_str;
> >>>
> >>>CUcontext cu_ctx;
> >>>CUmodulecu_module;
> >>> @@ -101,7 +102,15 @@ static av_cold int cudascale_init(AVFilterContext
> >>> *ctx)
> >>>{
> >>>CUDAScaleContext *s = ctx->priv;
> >>>
> >>> -s->format = AV_PIX_FMT_NONE;
> >>> +if (!strcmp(s->format_str, "same")) {
> >>> +s->format = AV_PIX_FMT_NONE;
> >>> +} else {
> >>> +s->format = av_get_pix_fmt(s->format_str);
> >>> +if (s->format == AV_PIX_FMT_NONE) {
> >>> +av_log(ctx, AV_LOG_ERROR, "Unrecognized pixel format: %s\n",
> >>> s->format_str);
> >>> +return AVERROR(EINVAL);
> >>> +}
> >>> +}
> >>>s->frame = av_frame_alloc();
> >>>if (!s->frame)
> >>>return AVERROR(ENOMEM);
> >>> @@ -533,6 +542,7 @@ fail:
> >>>static const AVOption options[] = {
> >>>{ "w", "Output video width", OFFSET(w_expr),
> >>> AV_OPT_TYPE_STRING, { .str = "iw" }, .flags = FLAGS },
> >>>{ "h", "Output video height", OFFSET(h_expr),
> >>> AV_OPT_TYPE_STRING, { .str = "ih" }, .flags = FLAGS },
> >>> +{ "format", "Output pixel format", OFFSET(format_str),
> >>> AV_OPT_TYPE_STRING, { .str = "same" }, .flags = FLAGS },
> >>>{ NULL },
> >>>};
> >>
> >> I'm not sure what to think about a dummy option like this. It might be
> >> very confusing for users to see a format option, which only accepts a
> >> single value, "same", and effectively does nothing.
> >>
> >
> > Not sure I understand the issue. "same" is the default (terminology
> > borrowed from the scale_npp filter), and it'll assign the format to
> > whatever is passed in (eg, format=yuv420p assigns that).
>
> Oh, I misread that code as just always throwing an error if it's != "same".
>
> Unfortunately, that option is omitted for a reason.
> If you look at scalecuda_resize:
> https://git.videolan.org/?p=ffmpeg.git;a=blob;f=libavfilter/vf_scale_cuda.c;h=b7cdb81081ff4a34e7b641c533fc23a5714fed61;hb=HEAD#l380
>
> It has the assumption built into it that the output frame has the same
> format as the input frame. So if you were to set format=nv12 and then
> input a yuv420p frame, this will most likely crash or at least severely
> misbehave.
>
> I would not be opposed to scale_cuda gaining the ability to also change
> frame pix_fmts, we are lacking such a filter at the moment if one
> ignores scale_npp.
> But in its current state, it can't do that.
>
Ah! Makes sense now - thanks for the explanation.
> >>
> >> Not strictly against it, since I can see the convenience it adds when
> >> building command lines, but I'd like some second opinions on this.
> >>
> >
> > Actually I'm using the API, albeit with some of lavfi conveniences to
> > parse filter strings. This avoids "wiring in" the output format
> > manually when crossing the lavfi boundary.
> >
> > Here's a example that demonstrates the issue via CLI (this may
> > actually be a bug elsewhere?):
> >
> > Broken:
> > ffmpeg -loglevel verbose -hwaccel cuvid -c:v h264_cuvid -i input.ts
> > -an -lavfi scale_cuda=w=426:h=240,hwdownload,format=yuv420p -c:v
> > libx264 out.ts
> >
> > Working:
> > ffmpeg -loglevel verbose -hwaccel cuvid -c:v h264_cuvid -i input.ts
> > -an -lavfi scale_cuda=w=426:h=240:format=yuv420p,hwdownload,format=yuv420p
> > -c:v libx264 out.ts
>
> Is this actually working in a sense where the result looks sensible?
> Cause with how the code currently is, scale_cuda with format set to
> yuv420p and getting nv12 as input from h264_cuvid will produce a frame
> labeled as yuv420p but containing nv12 data.
>
You are correct - I didn't look at the output here.
___
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 v3] lavfi: add new iteration API
Hi, On Sat, 24 Mar 2018 at 14:40, Josh de Kock wrote: > > Signed-off-by: Josh de Kock > --- > configure| 29 +- > doc/APIchanges | 4 + > doc/writing_filters.txt | 6 +- > libavfilter/allfilters.c | 823 > +-- > libavfilter/avfilter.c | 50 +-- > libavfilter/avfilter.h | 29 +- > libavfilter/version.h| 3 + > 7 files changed, 489 insertions(+), 455 deletions(-) > This is a couple years too late, but wanted to drop a note that this particular API change was breaking : it made avfilter_register a no-op. The consequence is that it's much more difficult to maintain filters out-of-tree; preserving the old behavior without changes to user code requires a special build of ffmpeg that has the filter configured/compiled in. The recommended workaround of using avfilter_graph_alloc_filter requires more effort to wire the filter in explicitly. It also doesn't allow for conveniences such as using avfilter_graph_parse, since there doesn't seem to be a way to make filters accessible via avfilter_get_by_name outside of ffmpeg compile time. If there is another workaround that I'm missing, please let me know, or if there's some deeper rationale for the decision to disable this feature. Josh ___ 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 v3] avcodec: Add explicit capability flag for encoder flushing
On Mon, 13 Apr 2020 at 04:39, Anton Khirnov wrote: > > Quoting Philip Langdale (2020-04-11 01:47:43) > > We've been in this fuzzy situation where maybe you could call > > avcodec_flush_buffers() on an encoder but there weren't any encoders > > that supported it except maybe audiotoolboxenc. Then we added flush > > support to nvenc very intentionally, and it worked, but that was more a > > coincidence than anything else. And if you call avcodec_flush_buffers() > > on an encoder that doesn't support it, it'll leave the encoder in an > > undefined state, so that's not great. > > > > As part of cleaning this up, this change introduces a formal capability > > flag for encoders that support flushing and ensures a flush call is a > > no-op for any other encoder. This allows client code to check if it is > > meaningful to call flush on an encoder before actually doing it. > > > > I have not attempted to separate the steps taken inside > > avcodec_flush_buffers() because it's not doing anything that's wrong > > for an encoder. But I did add a sanity check to reject attempts to > > flush a frame threaded encoder because I couldn't wrap my head around > > whether that code path was actually safe or not. As this combination > > doesn't exist today, we'll deal with it if it ever comes up. > > > > Signed-off-by: Philip Langdale > > --- > > I'm missing two things here: > - motivation in the commit message (and possibly in the doxy too) - why > is this needed and how is it better than just closing the encoder and > creating a new one One use case is segmented encoding: processing one segment at a time, where closing and re-opening a new encoder is expensive (as is the case with HW / nvenc). The original writeup for the motivation leading up to "encoder flush" can be found here: https://patchwork.ffmpeg.org/project/ffmpeg/patch/1574125994-7782-1-git-send-email-joshua.allm...@gmail.com/ Josh ___ 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 v3] lavfi: add new iteration API
On Tue, 14 Apr 2020 at 01:53, Josh de Kock wrote: > > Hi, > > On Mon, Apr 13, 2020, at 10:32 PM, Josh Allmann wrote: > > Hi, > > > > On Sat, 24 Mar 2018 at 14:40, Josh de Kock wrote: > > > > > > Signed-off-by: Josh de Kock > > > --- > > > configure| 29 +- > > > doc/APIchanges | 4 + > > > doc/writing_filters.txt | 6 +- > > > libavfilter/allfilters.c | 823 > > > +-- > > > libavfilter/avfilter.c | 50 +-- > > > libavfilter/avfilter.h | 29 +- > > > libavfilter/version.h| 3 + > > > 7 files changed, 489 insertions(+), 455 deletions(-) > > > > > > > This is a couple years too late, but wanted to drop a note that this > > particular API change was breaking : it made avfilter_register a > > no-op. The consequence is that it's much more difficult to maintain > > filters out-of-tree; preserving the old behavior without changes to > > user code requires a special build of ffmpeg that has the filter > > configured/compiled in. The recommended workaround of using > > avfilter_graph_alloc_filter requires more effort to wire the filter in > > explicitly. It also doesn't allow for conveniences such as using > > avfilter_graph_parse, since there doesn't seem to be a way to make > > filters accessible via avfilter_get_by_name outside of ffmpeg compile > > time. > > > > If there is another workaround that I'm missing, please let me know, > > or if there's some deeper rationale for the decision to disable this > > feature. > > This was actually an intentional change, there was some trouble with how > external codecs/filters/etc should be handled. > > Since this was an unsupported use-case which was quite sensitive to ABI the > change was there to explicitly prevent people (ab)using the API like this. It > was also to prepare for potentially a new API to implement this in a proper > fashion (but as you can see this was never completed). > > I did begin working on this again a little while back but due to an > unfortunate > data-loss I will have to start from scratch to continue working on it (yes, > yes > 'where's your backup?' I know). It's likely to be something I will be working > on in the future since I will be doing FFmpeg stuff again soon. > > There is also the discussion of 'do we want this?' from a more ideological > perspective which we will have to re-discuss, maybe something like gstreamer > is > more suited for the majority of the use-cases (?). > Thanks for the explanation Josh. For what it's worth, count me as being at least one API user for which out-of-tree filter capability would be very helpful. (And to preempt some other reactions, "use gstreamer" is not really helpful for us either...) Josh ___ 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][DISCUSS] nvenc: Add encoder flush API.
This patch is meant to be an entry point for discussion around an
issue we are having with flushing the nvenc encoder while doing
segmented transcoding. Hopefully there will be a less kludgey
workaround than this.
First, some background some info on where this is coming from. We do
segmented transcoding on Nvidia GPUs using the libav* libraries [0].
The flow is roughly this:
1. Segment incoming stream
2. Send each segment to a transcoder
We've noticed a significant overhead around setting up new transcode
sessions / contexts for each segment, and this overhead is magnified
the more streams a given machine is processing, regardless of the
number of attached GPUs [1].
Now, the logical solution here would be to reuse the GPU sessions
for segments during a given stream. However, there is a problem
around flushing internal decode / encode buffers. Because we do
segmented transcoding [2], we need to ensure that all stages in the
transcode pipeline are completely flushed in between each segment.
Here is what we do for each stage of decode, filter and encode:
* Decoding : Cache the first packet of each segment. When the
IO layer EOFs, feed the cached packet with a sentinel pts of -1.
(This doesn't seem to cause issues with h264_cuvid.) Once a frame
is returned from the decoder with the sentinel pts set, we know
the decoder is flushed of legitimate input. For a typical 2-second
segment, this has typically added about 6 frames (~10%) of overhead
which is tolerable because decoding is typically less expensive than
encoding, No changes are required to FFmpeg itself, which is nice.
* Filtering : Close the filtergraph (via av_buffersrc_close) and re-
initialize the filter with each segment. Again, the overhead here
seems tolerable. Have not seen a straightforward way to drain the
filtergraph without also closing or re-opening it.
* Encoding : This patch.
We add a very special "av_nvenc_flush" API to signal end-of-stream
in the same way as `avcodec_send_packet(ctx, NULL)` but bypassing
all the higher-level libavcodec machinery before hitting nvenc.
This seems to successfully drain pending frames. Afterwards,
we can continue to send packets for the next segments via
`avcodec_send_packet` and the internal state will more-or-less
reinitialize as if nothing had happened.
Now, it is quite likely that this behavior is entirely accidental,
and should not be expected to be stable in the future.
While the nvenc encoder itself does seem to be "resumable" according
to the documentation around the `NV_ENC_FLAGS_EOS` flag (cf.
NVIDIA Video Encoder API Programming Guide), FFmpeg has no such
mode. So we've had to sort of inject one in here.
The questions here are:
* Are these workarounds reasonable for the problem of Nvidia GPU
sessions taking a long time to initialize when transcoding under
load?
* Is there an alternative to carrying around this patch to flush
the encoder in between segments?
* If there is no alternative, would you be open to a more formalized
addition to the avcodec API around "flushable" or "resumable"
encoders?
Thanks for your thoughts!
Josh
[0] https://github.com/livepeer/lpms
[1] https://gist.github.com/j0sh/ae9e5a97e794e364a6dfe513fa2591c2
[2] For historical reasons we cannot easily change right now
---
libavcodec/avcodec.h | 2 ++
libavcodec/nvenc.c | 5 +
2 files changed, 7 insertions(+)
diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
index bcb931f0dd..763a557d82 100644
--- a/libavcodec/avcodec.h
+++ b/libavcodec/avcodec.h
@@ -6232,6 +6232,8 @@ const AVCodecDescriptor
*avcodec_descriptor_get_by_name(const char *name);
*/
AVCPBProperties *av_cpb_properties_alloc(size_t *size);
+int av_nvenc_flush(AVCodecContext *avctx);
+
/**
* @}
*/
diff --git a/libavcodec/nvenc.c b/libavcodec/nvenc.c
index 111048d043..36134fa6a9 100644
--- a/libavcodec/nvenc.c
+++ b/libavcodec/nvenc.c
@@ -2071,6 +2071,11 @@ static void reconfig_encoder(AVCodecContext *avctx,
const AVFrame *frame)
}
}
+int attribute_align_arg av_nvenc_flush(AVCodecContext *avctx)
+{
+ return ff_nvenc_send_frame(avctx, NULL);
+}
+
int ff_nvenc_send_frame(AVCodecContext *avctx, const AVFrame *frame)
{
NVENCSTATUS nv_status;
--
2.17.1
___
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][DISCUSS] nvenc: Add encoder flush API.
On Fri, 20 Dec 2019 at 15:36, Philip Langdale wrote: > > On 2019-11-18 17:13, Josh Allmann wrote: > > This patch is meant to be an entry point for discussion around an > > issue we are having with flushing the nvenc encoder while doing > > segmented transcoding. Hopefully there will be a less kludgey > > workaround than this. > > Hi Josh, > > I happened to see your email recently, and took a quick look into > this. It seems that encoders are allowed to implement .flush() and > then avcodec_flush_buffers() can be called on them like on a > decoder. So I've posted a patch that does this (with the same impl > that you had). If that works for you, then that's all it takes - > no need for a new API call because there's already one you can use. That would be perfect - thought .flush() was decode-only for some reason. Thank you! Josh ___ 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] nvenc: implement flush to help allow an encoder to be re-used
On Fri, 20 Dec 2019 at 15:34, Philip Langdale wrote:
>
> It can be useful to re-use an encoder instance when doing segmented
> encodings, and this requires flushing the encoder at the start of
> each segment.
> ---
> libavcodec/nvenc.c | 5 +
> libavcodec/nvenc.h | 2 ++
> libavcodec/nvenc_h264.c | 1 +
> libavcodec/nvenc_hevc.c | 1 +
> 4 files changed, 9 insertions(+)
>
> diff --git a/libavcodec/nvenc.c b/libavcodec/nvenc.c
> index 310e30805d..9a96bf2bba 100644
> --- a/libavcodec/nvenc.c
> +++ b/libavcodec/nvenc.c
> @@ -2262,3 +2262,8 @@ int ff_nvenc_encode_frame(AVCodecContext *avctx,
> AVPacket *pkt,
>
> return 0;
> }
> +
> +av_cold void ff_nvenc_encode_flush(AVCodecContext *avctx)
> +{
> +ff_nvenc_send_frame(avctx, NULL);
One concern I had was about the long-term stability of this behavior.
Right now, it works, but perhaps only coincidentally? Being flushable
and resumable like this isn't explicitly part of the "contract" for
nvenc, as far as I can see. Could future changes inadvertently
introduce state that isn't reset in between flushes, breaking the
resumable behavior? If so, is there a way to safeguard against that?
Josh
___
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] nvenc: implement flush to help allow an encoder to be re-used
On Mon, 30 Dec 2019 at 16:40, Philip Langdale wrote: > > On Sat, 21 Dec 2019 14:54:38 -0800 > Philip Langdale wrote: > > > On Fri, 20 Dec 2019 16:07:18 -0800 > > Josh Allmann wrote: > > > > > One concern I had was about the long-term stability of this > > > behavior. Right now, it works, but perhaps only coincidentally? > > > Being flushable and resumable like this isn't explicitly part of > > > the "contract" for nvenc, as far as I can see. Could future changes > > > inadvertently introduce state that isn't reset in between flushes, > > > breaking the resumable behavior? If so, is there a way to safeguard > > > against that? > > > > > > Josh > > > > So, the behaviour at the ffmpeg level is something you can view as > > stable. If it was to break, I'd expect us to fix it. For nvenc itself, > > that's harder to make any statements about. I wouldn't expect the > > nvidia folks to change thing casually, but until they document a > > specific flush behaviour, there's always going to be a risk - > > ultimately we just have to react if they change something. > > Hi Phil, Flushing and resumption is documented/supported in nvenc via NV_ENC_FLAGS_EOS, but I wasn't sure if this was a feature that ffmpeg's integration was intentionally designed for. But if you confirm we can expect this behavior to be supported going forward, then that's great news. > > In an ideal world, you'd have a test running for this, but we're not > > set up to exercise any hwaccels in our automated fate executions. > > We do have internal tests [1] that should catch the issue if anything changes, so that might be of some help as well, although we currently only update ffmpeg on an as-needed basis. [1] https://github.com/livepeer/lpms/blob/master/ffmpeg/nvidia_test.go > > Did this form of the patch work for you? > > > > Hi Josh, > > Did you get a chance to try it? > > --phil Was delayed on testing this due to the holidays, my apologies. Can confirm that this patch works very nicely in conjunction with avcodec_flush_buffers . Thanks so much! Josh ___ 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] avformat/flvdec: Ignore the first two data/subtitle streams.
Previously, one or the other would have been ignored, but not both.
Since the probe terminates at three streams, it could exit
prematurely if both data and subtitles are present along with
slightly trailing media, usually video trailing audio.
Trailing media is common in RTMP, and encoders write strange metadata.
---
libavformat/flvdec.c | 24 +++-
1 file changed, 19 insertions(+), 5 deletions(-)
diff --git a/libavformat/flvdec.c b/libavformat/flvdec.c
index 4b9f46902b..1be8d98618 100644
--- a/libavformat/flvdec.c
+++ b/libavformat/flvdec.c
@@ -134,18 +134,32 @@ static void add_keyframes_index(AVFormatContext *s)
}
}
+static int is_ignorable(enum AVMediaType codec_type)
+{
+switch(codec_type) {
+case AVMEDIA_TYPE_SUBTITLE:
+case AVMEDIA_TYPE_DATA:
+return 1;
+}
+return 0;
+}
+
static AVStream *create_stream(AVFormatContext *s, int codec_type)
{
+int streams_to_ignore = 0, nb_streams = 0;
FLVContext *flv = s->priv_data;
AVStream *st = avformat_new_stream(s, NULL);
if (!st)
return NULL;
st->codecpar->codec_type = codec_type;
-if (s->nb_streams>=3 ||( s->nb_streams==2
- && s->streams[0]->codecpar->codec_type !=
AVMEDIA_TYPE_SUBTITLE
- && s->streams[1]->codecpar->codec_type !=
AVMEDIA_TYPE_SUBTITLE
- && s->streams[0]->codecpar->codec_type !=
AVMEDIA_TYPE_DATA
- && s->streams[1]->codecpar->codec_type !=
AVMEDIA_TYPE_DATA))
+
+if (s->nb_streams >= 1)
+streams_to_ignore += is_ignorable(s->streams[0]->codecpar->codec_type);
+if (s->nb_streams >= 2)
+streams_to_ignore += is_ignorable(s->streams[1]->codecpar->codec_type);
+
+nb_streams = s->nb_streams - streams_to_ignore;
+if (nb_streams >= 2)
s->ctx_flags &= ~AVFMTCTX_NOHEADER;
if (codec_type == AVMEDIA_TYPE_AUDIO) {
st->codecpar->bit_rate = flv->audio_bit_rate;
--
2.17.1
___
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] avformat/flvdec: Ignore the first two data/subtitle streams.
On Thu, 13 May 2021 at 16:38, Josh Allmann wrote: > > Previously, one or the other would have been ignored, but not both. > Since the probe terminates at three streams, it could exit > prematurely if both data and subtitles are present along with > slightly trailing media, usually video trailing audio. > > Trailing media is common in RTMP, and encoders write strange metadata. > --- Here's a trac ticket with some more context: https://trac.ffmpeg.org/ticket/9241 And a sample that exhibits the problem: https://trac.ffmpeg.org/attachment/ticket/9241/flv-probe-missing-streams.flv Josh ___ 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] rtmp: Plug leak if sending bytes read report fails.
---
libavformat/rtmpproto.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/libavformat/rtmpproto.c b/libavformat/rtmpproto.c
index faf2a6f244..b741e421af 100644
--- a/libavformat/rtmpproto.c
+++ b/libavformat/rtmpproto.c
@@ -2431,8 +2431,10 @@ static int get_packet(URLContext *s, int for_header)
rt->bytes_read += ret;
if (rt->bytes_read - rt->last_bytes_read > rt->receive_report_size) {
av_log(s, AV_LOG_DEBUG, "Sending bytes read report\n");
-if ((ret = gen_bytes_read(s, rt, rpkt.timestamp + 1)) < 0)
+if ((ret = gen_bytes_read(s, rt, rpkt.timestamp + 1)) < 0) {
+ff_rtmp_packet_destroy(&rpkt);
return ret;
+}
rt->last_bytes_read = rt->bytes_read;
}
--
2.14.2
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] aacenc: Free any extradata before re-allocating.
Fixes a leak that occurs if avctx->extradata contains any data
prior to opening the codec, eg left over from an initialization
call to avcodec_parameters_from_context.
---
libavcodec/aacenc.c | 4
1 file changed, 4 insertions(+)
diff --git a/libavcodec/aacenc.c b/libavcodec/aacenc.c
index 6d94c76905..f8fbe69d87 100644
--- a/libavcodec/aacenc.c
+++ b/libavcodec/aacenc.c
@@ -98,6 +98,10 @@ static int put_audio_specific_config(AVCodecContext *avctx)
int channels = (!s->needs_pce)*(s->channels - (s->channels == 8 ? 1 : 0));
const int max_size = 32;
+if (avctx->extradata) {
+av_freep(&avctx->extradata);
+avctx->extradata_size = 0;
+}
avctx->extradata = av_mallocz(max_size);
if (!avctx->extradata)
return AVERROR(ENOMEM);
--
2.14.2
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] aacenc: Free any extradata before re-allocating.
On 6 February 2018 at 03:16, Rostislav Pehlivanov
wrote:
> On 6 February 2018 at 06:56, Josh Allmann
> wrote:
>
> > Fixes a leak that occurs if avctx->extradata contains any data
> > prior to opening the codec, eg left over from an initialization
> > call to avcodec_parameters_from_context.
> > ---
> > libavcodec/aacenc.c | 4
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/libavcodec/aacenc.c b/libavcodec/aacenc.c
> > index 6d94c76905..f8fbe69d87 100644
> > --- a/libavcodec/aacenc.c
> > +++ b/libavcodec/aacenc.c
> > @@ -98,6 +98,10 @@ static int put_audio_specific_config(AVCodecContext
> > *avctx)
> > int channels = (!s->needs_pce)*(s->channels - (s->channels == 8 ? 1
> :
> > 0));
> > const int max_size = 32;
> >
> > +if (avctx->extradata) {
> > +av_freep(&avctx->extradata);
> > +avctx->extradata_size = 0;
> > +}
> > avctx->extradata = av_mallocz(max_size);
> > if (!avctx->extradata)
> > return AVERROR(ENOMEM);
> > --
> > 2.14.2
> >
> > ___
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
>
> No, its not up to the encoder to free up the extradata. Its up to the API
> user to close the avctx for the encoder which will free the extradata, even
> if encoder init fails. Besides, if you don't, you'll have a dirty context
> from the previous encoder since they don't have to set the same avctx
> fields.
>
While many (all?) other encoders share the pattern of allocating extradata
without checking for previous allocations, the abstraction seems... leaky?
(Pun fully intended.) If the encoder has its avctx fields set by
avcodec_parameters_to_context, then the extradata is deep-copied. Even when
deep copying, we do take care to deallocate any existing avctx extradata,
to avoid introducing the same type of leak. Without this patch, the API
user would have to explicitly undo the work that
avcodec_parameters_to_context is trying to help with. Hence, the 'right'
workflow when using avcodec_parameters_to_context would be:
0. Allocate codec context
1. Copy codec params to context with avcodec_parameters_to_context
2. Check if extradata exists in context and deallocate from context if so.
3. Open codec.
...
4. Close codec.
These semantics don't seem clean to me, and I think we should strive to
avoid making the user deal with corner cases like these explicitly. If not,
we should at least document it.
Josh
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] avcodec/noise_bsf: Add keyframes option.
---
doc/bitstream_filters.texi | 5 +
libavcodec/noise_bsf.c | 12
libavcodec/version.h | 2 +-
3 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/doc/bitstream_filters.texi b/doc/bitstream_filters.texi
index cfd81fa12d..a9f17f32ec 100644
--- a/doc/bitstream_filters.texi
+++ b/doc/bitstream_filters.texi
@@ -399,6 +399,11 @@ every byte is modified.
A numeral string, whose value is related to how often packets will be dropped.
Therefore, values below or equal to 0 are forbidden, and the lower the more
frequent packets will be dropped, with 1 meaning every packet is dropped.
+@item keyframes
+A numeral string, whose value indicates the number of keyframe packets that
+will be dropped from the beginning of the stream. This option will not add
+noise to the source data. If used with stream copy, then -copyinkf should
+be added to the output options as well.
@end table
The following example applies the modification to every byte but does not drop
diff --git a/libavcodec/noise_bsf.c b/libavcodec/noise_bsf.c
index 84b94032ad..363ea4fc71 100644
--- a/libavcodec/noise_bsf.c
+++ b/libavcodec/noise_bsf.c
@@ -32,6 +32,7 @@ typedef struct NoiseContext {
const AVClass *class;
int amount;
int dropamount;
+int keyframes;
unsigned int state;
} NoiseContext;
@@ -49,6 +50,13 @@ static int noise(AVBSFContext *ctx, AVPacket *out)
if (ret < 0)
return ret;
+if (s->keyframes > 0 && in->flags & AV_PKT_FLAG_KEY) {
+ s->keyframes--;
+ if (!s->keyframes) s->keyframes = -1;
+ av_packet_free(&in);
+ return AVERROR(EAGAIN);
+}
+
if (s->dropamount > 0 && s->state % s->dropamount == 0) {
s->state++;
av_packet_free(&in);
@@ -65,6 +73,9 @@ static int noise(AVBSFContext *ctx, AVPacket *out)
memcpy(out->data, in->data, in->size);
+if (s->keyframes)
+ return ret;
+
for (i = 0; i < out->size; i++) {
s->state += out->data[i] + 1;
if (s->state % amount == 0)
@@ -81,6 +92,7 @@ fail:
static const AVOption options[] = {
{ "amount", NULL, OFFSET(amount), AV_OPT_TYPE_INT, { .i64 = 0 }, 0,
INT_MAX },
{ "dropamount", NULL, OFFSET(dropamount), AV_OPT_TYPE_INT, { .i64 = 0 },
0, INT_MAX },
+{ "keyframes", NULL, OFFSET(keyframes), AV_OPT_TYPE_INT, { .i64 = 0 }, 0,
INT_MAX },
{ NULL },
};
diff --git a/libavcodec/version.h b/libavcodec/version.h
index ca18ce6e8b..1e84410d68 100644
--- a/libavcodec/version.h
+++ b/libavcodec/version.h
@@ -29,7 +29,7 @@
#define LIBAVCODEC_VERSION_MAJOR 58
#define LIBAVCODEC_VERSION_MINOR 13
-#define LIBAVCODEC_VERSION_MICRO 100
+#define LIBAVCODEC_VERSION_MICRO 101
#define LIBAVCODEC_VERSION_INT AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
LIBAVCODEC_VERSION_MINOR, \
--
2.14.2
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/noise_bsf: Add keyframes option.
On 7 March 2018 at 18:03, Michael Niedermayer wrote:
> On Tue, Mar 06, 2018 at 12:47:15PM -0800, Josh Allmann wrote:
>> ---
>> doc/bitstream_filters.texi | 5 +
>> libavcodec/noise_bsf.c | 12
>> libavcodec/version.h | 2 +-
>> 3 files changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/doc/bitstream_filters.texi b/doc/bitstream_filters.texi
>> index cfd81fa12d..a9f17f32ec 100644
>> --- a/doc/bitstream_filters.texi
>> +++ b/doc/bitstream_filters.texi
>> @@ -399,6 +399,11 @@ every byte is modified.
>> A numeral string, whose value is related to how often packets will be
>> dropped.
>> Therefore, values below or equal to 0 are forbidden, and the lower the more
>> frequent packets will be dropped, with 1 meaning every packet is dropped.
>> +@item keyframes
>> +A numeral string, whose value indicates the number of keyframe packets that
>> +will be dropped from the beginning of the stream. This option will not add
>> +noise to the source data. If used with stream copy, then -copyinkf should
>> +be added to the output options as well.
>> @end table
>>
>> The following example applies the modification to every byte but does not
>> drop
>> diff --git a/libavcodec/noise_bsf.c b/libavcodec/noise_bsf.c
>> index 84b94032ad..363ea4fc71 100644
>> --- a/libavcodec/noise_bsf.c
>> +++ b/libavcodec/noise_bsf.c
>> @@ -32,6 +32,7 @@ typedef struct NoiseContext {
>> const AVClass *class;
>> int amount;
>> int dropamount;
>> +int keyframes;
>> unsigned int state;
>> } NoiseContext;
>>
>> @@ -49,6 +50,13 @@ static int noise(AVBSFContext *ctx, AVPacket *out)
>> if (ret < 0)
>> return ret;
>>
>> +if (s->keyframes > 0 && in->flags & AV_PKT_FLAG_KEY) {
>> + s->keyframes--;
>> + if (!s->keyframes) s->keyframes = -1;
>> + av_packet_free(&in);
>> + return AVERROR(EAGAIN);
>> +}
>
Thanks for the feedback.
> I think keyframe should work like dropamount, that is randomly.
>
> a non random droping could be added, maybe by the user specifying in
> a list what to drop.
> That would be more powerfull and flexible but probably not much harder
Something like this?
noise=keyframes=1,3,5,7
in order to drop the 1st, 3rd, 5th and 7th keyframes?
> also keeping keyframes and dropamount behave the same
> is more consistent
>
Do you mean more consistent with respect to the 'amount' param of added noise,
as opposed to the patch's behavior of skipping noise if the 'keyframes' param is
present?
> also the indention depth mismatches the surrounding code
>
>
> [...]
>
> --
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Let us carefully observe those good qualities wherein our enemies excel us
> and endeavor to excel them, by avoiding what is faulty, and imitating what
> is excellent in them. -- Plutarch
>
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] avcodec/h264_mp4toannexb: Prepend SPS/PPS to buffering period SEI
Encoders may emit a buffering period SEI without a corresponding
SPS/PPS if the SPS/PPS is carried out-of-band, eg with avcc.
During Annex B conversion, this may result in the SPS/PPS being
inserted *after* the buffering period SEI but before the IDR NAL.
Since the buffering period SEI references the SPS, the SPS/PPS
needs to come first.
---
libavcodec/bsf/h264_mp4toannexb.c | 13 +
1 file changed, 13 insertions(+)
diff --git a/libavcodec/bsf/h264_mp4toannexb.c
b/libavcodec/bsf/h264_mp4toannexb.c
index 92af6a6881..6607f1e91a 100644
--- a/libavcodec/bsf/h264_mp4toannexb.c
+++ b/libavcodec/bsf/h264_mp4toannexb.c
@@ -363,6 +363,19 @@ static int h264_mp4toannexb_filter(AVBSFContext *ctx,
AVPacket *opkt)
if (!new_idr && unit_type == H264_NAL_IDR_SLICE && (buf[1] & 0x80))
new_idr = 1;
+/* If this is a buffering period SEI without a corresponding
sps/pps
+ * then prepend any existing sps/pps before the SEI */
+if (unit_type == H264_NAL_SEI && buf[1] == 0 && !sps_seen &&
!pps_seen) {
+if (s->sps_size) {
+count_or_copy(&out, &out_size, s->sps, s->sps_size,
PS_OUT_OF_BAND, j);
+sps_seen = 1;
+}
+if (s->pps_size) {
+count_or_copy(&out, &out_size, s->pps, s->pps_size,
PS_OUT_OF_BAND, j);
+pps_seen = 1;
+}
+}
+
/* prepend only to the first type 5 NAL unit of an IDR picture, if
no sps/pps are already present */
if (new_idr && unit_type == H264_NAL_IDR_SLICE && !sps_seen &&
!pps_seen) {
if (s->sps_size)
--
2.39.2
___
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] avcodec/h264_mp4toannexb: Prepend SPS/PPS to buffering period SEI
On Sat, 6 Jul 2024 at 09:37, Michael Niedermayer wrote: > > On Wed, Jul 03, 2024 at 02:05:06PM -0700, Josh Allmann wrote: > > Encoders may emit a buffering period SEI without a corresponding > > SPS/PPS if the SPS/PPS is carried out-of-band, eg with avcc. > > > > During Annex B conversion, this may result in the SPS/PPS being > > inserted *after* the buffering period SEI but before the IDR NAL. > > > > Since the buffering period SEI references the SPS, the SPS/PPS > > needs to come first. > > --- > > libavcodec/bsf/h264_mp4toannexb.c | 13 + > > 1 file changed, 13 insertions(+) > > breaks fate > > TESTh264-bsf-mp4toannexb > --- ./tests/ref/fate/h264-bsf-mp4toannexb 2024-07-01 23:30:40.656213791 > +0200 > +++ tests/data/fate/h264-bsf-mp4toannexb2024-07-06 12:13:56.491072296 > +0200 > @@ -1 +1 @@ > -5f04c27cc6ee8625fe2405fb0f7da9a3 > +ff2551123909f54c382294baa1bb4364 > Test h264-bsf-mp4toannexb failed. Look at > tests/data/fate/h264-bsf-mp4toannexb.err for details. > make: *** [tests/Makefile:311: fate-h264-bsf-mp4toannexb] Error 1 > Thanks for the heads up. Took a look into each of the failing fate tests from [0] and I think these changes are expected and OK. Each of the failing tests shows the problem that this patch fixes, which is that the SPS/PPS is written after the buffering period SEI. An easy way to eyeball the issue is that probing the Annex B output logs an error which says "non-existing SPS 0 referenced in buffering period" - in fact this is why I wrote this patch, to get to the bottom of that message. The NAL ordering can also be inspected with `bsf:v trace_headers`. There also seems to be a side benefit that makes segmenting more robust - details below. Some notes on each failing test: fate-segment-mp4-to-ts : Before this patch, the segments produced after 000.ts are not independently decodable, because only the first segment has any extradata [1]. After the patch, the segments can be decoded independently. Unless the intent of the test is to actually produce broken segments, this patch is probably correct in fixing that. Also see the `fate-h264-bsf-mp4toannexb` testcase. fate-h264-bsf-mp4toannexb : In the original version, the SPS/PPS is only written once - maybe because there are no true IDRs after the first frame, only recovery point SEIs. In the patched version, the SPS/PPS is written before each buffering period SEI, 6 times in total. I can see how this might be strictly unnecessary, but probably harmless from a spec standpoint. Also it seems to make segmented muxing more robust, since this testcase shares the same input as `fate-segment-mp4-to-ts` which produces broken segments without the patch. fate-h264_mp4toannexb_ticket2991 : This is a clean example of the problem that this patch solves: without it, a buffering period SEI comes before the SPS/PPS. Inspecting a diff of the NAL units [2], the only change is in the reordering of the NALs such that the SPS/PPS comes before the buffering period SEI, rather than after. If all that seems OK, then I can send another patch to update the fate references to match the new values. [0] https://patchwork.ffmpeg.org/check/104951/ [1] The first segment has extradata, but it is still in the wrong order without the patch. [2] https://gist.github.com/j0sh/c912056138822c4d8c9564f4062e1e7b Josh ___ 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] avcodec/h264_mp4toannexb: Prepend SPS/PPS to buffering period SEI
Encoders may emit a buffering period SEI without a corresponding
SPS/PPS if the SPS/PPS is carried out-of-band, eg with avcc.
During Annex B conversion, this may result in the SPS/PPS being
inserted *after* the buffering period SEI but before the IDR NAL.
Since the buffering period SEI references the SPS, the SPS/PPS
needs to come first.
---
Notes:
v2: Updated FATE test refs
libavcodec/bsf/h264_mp4toannexb.c | 13 +
tests/ref/fate/h264-bsf-mp4toannexb| 2 +-
tests/ref/fate/h264_mp4toannexb_ticket2991 | 18 +-
tests/ref/fate/segment-mp4-to-ts | 12 ++--
4 files changed, 29 insertions(+), 16 deletions(-)
diff --git a/libavcodec/bsf/h264_mp4toannexb.c
b/libavcodec/bsf/h264_mp4toannexb.c
index 92af6a6881..6607f1e91a 100644
--- a/libavcodec/bsf/h264_mp4toannexb.c
+++ b/libavcodec/bsf/h264_mp4toannexb.c
@@ -363,6 +363,19 @@ static int h264_mp4toannexb_filter(AVBSFContext *ctx,
AVPacket *opkt)
if (!new_idr && unit_type == H264_NAL_IDR_SLICE && (buf[1] & 0x80))
new_idr = 1;
+/* If this is a buffering period SEI without a corresponding
sps/pps
+ * then prepend any existing sps/pps before the SEI */
+if (unit_type == H264_NAL_SEI && buf[1] == 0 && !sps_seen &&
!pps_seen) {
+if (s->sps_size) {
+count_or_copy(&out, &out_size, s->sps, s->sps_size,
PS_OUT_OF_BAND, j);
+sps_seen = 1;
+}
+if (s->pps_size) {
+count_or_copy(&out, &out_size, s->pps, s->pps_size,
PS_OUT_OF_BAND, j);
+pps_seen = 1;
+}
+}
+
/* prepend only to the first type 5 NAL unit of an IDR picture, if
no sps/pps are already present */
if (new_idr && unit_type == H264_NAL_IDR_SLICE && !sps_seen &&
!pps_seen) {
if (s->sps_size)
diff --git a/tests/ref/fate/h264-bsf-mp4toannexb
b/tests/ref/fate/h264-bsf-mp4toannexb
index 2049f39701..81ff568f3d 100644
--- a/tests/ref/fate/h264-bsf-mp4toannexb
+++ b/tests/ref/fate/h264-bsf-mp4toannexb
@@ -1 +1 @@
-5f04c27cc6ee8625fe2405fb0f7da9a3
+ff2551123909f54c382294baa1bb4364
diff --git a/tests/ref/fate/h264_mp4toannexb_ticket2991
b/tests/ref/fate/h264_mp4toannexb_ticket2991
index f8e3e920d4..9a1fbf2f8c 100644
--- a/tests/ref/fate/h264_mp4toannexb_ticket2991
+++ b/tests/ref/fate/h264_mp4toannexb_ticket2991
@@ -1,4 +1,4 @@
-05d66e60ab22ee004720e0051af0fe74
*tests/data/fate/h264_mp4toannexb_ticket2991.h264
+b6ff5910928ad0b2a7eec481dcc41594
*tests/data/fate/h264_mp4toannexb_ticket2991.h264
1985815 tests/data/fate/h264_mp4toannexb_ticket2991.h264
#extradata 0: 47, 0x3a590d55
#tb 0: 1/120
@@ -6,7 +6,7 @@
#codec_id 0: h264
#dimensions 0: 1280x720
#sar 0: 3/4
-0, 0, 0,40040,37126, 0xb020184c
+0, 0, 0,40040,37126, 0x515c184c
0, 40040, 40040,40040, 6920, 0x8512361a, F=0x0
0, 80081, 80081,40040, 7550, 0x1bc56ed4, F=0x0
0, 120121, 120121,40040, 8752, 0xb8c6f0a1, F=0x0
@@ -21,7 +21,7 @@
0, 480485, 480485,40040,11234, 0x83cbd9fd, F=0x0
0, 520525, 520525,40040,17616, 0xfdf95104, F=0x0
0, 560566, 560566,40040,10689, 0x9633d32b, F=0x0
-0, 600606, 600606,40040,45291, 0x543c2cf6
+0, 600606, 600606,40040,45291, 0xa8292cf6
0, 640646, 640646,40040,20837, 0x051abfab, F=0x0
0, 680687, 680687,40040,21418, 0xe2a59d70, F=0x0
0, 720727, 720727,40040,15643, 0x15cf2cec, F=0x0
@@ -36,7 +36,7 @@
0,1081091,1081091,40040,13130, 0xcbb6bb8e, F=0x0
0,1121131,1121131,40040,16180, 0x5d188a7a, F=0x0
0,1161172,1161172,40040,14961, 0x9ff2f463, F=0x0
-0,1201212,1201212,40040,54296, 0xe6ec30ed
+0,1201212,1201212,40040,54296, 0x3ae830ed
0,1241252,1241252,40040,11500, 0x8c4852c9, F=0x0
0,1281293,1281293,40040,12065, 0xfb7954c3, F=0x0
0,1321333,1321333,40040,12532, 0xf0a935d3, F=0x0
@@ -51,7 +51,7 @@
0,1681697,1681697,40040,13250, 0xfed0deb8, F=0x0
0,1721737,1721737,40040,13360, 0xbf92d476, F=0x0
0,1761778,1761778,40040,11749, 0x3041eaf1, F=0x0
-0,1801818,1801818,40040,23997, 0xdbe6d5c4
+0,1801818,1801818,40040,23997, 0x2fe2d5c4
0,1841858,1841858,40040,16065, 0xe8f715b7, F=0x0
0,1881899,1881899,40040,16441, 0x0a4e060f, F=0x0
0,1921939,1921939,40040,17395, 0xa8edecc2, F=0x0
@@ -66,7 +66,7 @@
0,2282303,2282303,40040,13748, 0xed26aeb4, F=0x0
0,2322343,2322343,40040,15092, 0x3c983538, F=0x0
0,2362384,2362384,40040,14636, 0x9b278a6c, F=0x0
-0,2402424,24024
Re: [FFmpeg-devel] [PATCH] avcodec/h264_mp4toannexb: Prepend SPS/PPS to buffering period SEI
On Tue, 9 Jul 2024 at 12:06, Josh Allmann wrote:
>
> Encoders may emit a buffering period SEI without a corresponding
> SPS/PPS if the SPS/PPS is carried out-of-band, eg with avcc.
>
> During Annex B conversion, this may result in the SPS/PPS being
> inserted *after* the buffering period SEI but before the IDR NAL.
>
> Since the buffering period SEI references the SPS, the SPS/PPS
> needs to come first.
> ---
>
> Notes:
> v2: Updated FATE test refs
>
> libavcodec/bsf/h264_mp4toannexb.c | 13 +
> tests/ref/fate/h264-bsf-mp4toannexb| 2 +-
> tests/ref/fate/h264_mp4toannexb_ticket2991 | 18 +-
> tests/ref/fate/segment-mp4-to-ts | 12 ++--
> 4 files changed, 29 insertions(+), 16 deletions(-)
>
Ping for review. Looking at the FATE output, this patch fixes a number
of things - see [1] for details
[1] https://ffmpeg.org//pipermail/ffmpeg-devel/2024-July/330964.html
> diff --git a/libavcodec/bsf/h264_mp4toannexb.c
> b/libavcodec/bsf/h264_mp4toannexb.c
> index 92af6a6881..6607f1e91a 100644
> --- a/libavcodec/bsf/h264_mp4toannexb.c
> +++ b/libavcodec/bsf/h264_mp4toannexb.c
> @@ -363,6 +363,19 @@ static int h264_mp4toannexb_filter(AVBSFContext *ctx,
> AVPacket *opkt)
> if (!new_idr && unit_type == H264_NAL_IDR_SLICE && (buf[1] &
> 0x80))
> new_idr = 1;
>
> +/* If this is a buffering period SEI without a corresponding
> sps/pps
> + * then prepend any existing sps/pps before the SEI */
> +if (unit_type == H264_NAL_SEI && buf[1] == 0 && !sps_seen &&
> !pps_seen) {
> +if (s->sps_size) {
> +count_or_copy(&out, &out_size, s->sps, s->sps_size,
> PS_OUT_OF_BAND, j);
> +sps_seen = 1;
> +}
> +if (s->pps_size) {
> +count_or_copy(&out, &out_size, s->pps, s->pps_size,
> PS_OUT_OF_BAND, j);
> +pps_seen = 1;
> +}
> +}
> +
> /* prepend only to the first type 5 NAL unit of an IDR picture,
> if no sps/pps are already present */
> if (new_idr && unit_type == H264_NAL_IDR_SLICE && !sps_seen &&
> !pps_seen) {
> if (s->sps_size)
> diff --git a/tests/ref/fate/h264-bsf-mp4toannexb
> b/tests/ref/fate/h264-bsf-mp4toannexb
> index 2049f39701..81ff568f3d 100644
> --- a/tests/ref/fate/h264-bsf-mp4toannexb
> +++ b/tests/ref/fate/h264-bsf-mp4toannexb
> @@ -1 +1 @@
> -5f04c27cc6ee8625fe2405fb0f7da9a3
> +ff2551123909f54c382294baa1bb4364
> diff --git a/tests/ref/fate/h264_mp4toannexb_ticket2991
> b/tests/ref/fate/h264_mp4toannexb_ticket2991
> index f8e3e920d4..9a1fbf2f8c 100644
> --- a/tests/ref/fate/h264_mp4toannexb_ticket2991
> +++ b/tests/ref/fate/h264_mp4toannexb_ticket2991
> @@ -1,4 +1,4 @@
> -05d66e60ab22ee004720e0051af0fe74
> *tests/data/fate/h264_mp4toannexb_ticket2991.h264
> +b6ff5910928ad0b2a7eec481dcc41594
> *tests/data/fate/h264_mp4toannexb_ticket2991.h264
> 1985815 tests/data/fate/h264_mp4toannexb_ticket2991.h264
> #extradata 0: 47, 0x3a590d55
> #tb 0: 1/120
> @@ -6,7 +6,7 @@
> #codec_id 0: h264
> #dimensions 0: 1280x720
> #sar 0: 3/4
> -0, 0, 0,40040,37126, 0xb020184c
> +0, 0, 0,40040,37126, 0x515c184c
> 0, 40040, 40040,40040, 6920, 0x8512361a, F=0x0
> 0, 80081, 80081,40040, 7550, 0x1bc56ed4, F=0x0
> 0, 120121, 120121,40040, 8752, 0xb8c6f0a1, F=0x0
> @@ -21,7 +21,7 @@
> 0, 480485, 480485,40040,11234, 0x83cbd9fd, F=0x0
> 0, 520525, 520525,40040,17616, 0xfdf95104, F=0x0
> 0, 560566, 560566,40040,10689, 0x9633d32b, F=0x0
> -0, 600606, 600606,40040,45291, 0x543c2cf6
> +0, 600606, 600606,40040,45291, 0xa8292cf6
> 0, 640646, 640646,40040,20837, 0x051abfab, F=0x0
> 0, 680687, 680687,40040,21418, 0xe2a59d70, F=0x0
> 0, 720727, 720727,40040,15643, 0x15cf2cec, F=0x0
> @@ -36,7 +36,7 @@
> 0,1081091,1081091,40040,13130, 0xcbb6bb8e, F=0x0
> 0,1121131,1121131,40040,16180, 0x5d188a7a, F=0x0
> 0,1161172,1161172,40040,14961, 0x9ff2f463, F=0x0
> -0,1201212,1201212,40040,54296, 0xe6ec30ed
> +0,1201212,1201212,40040,54296, 0x3ae830ed
> 0,1241252,1241252,40040,11500, 0x8c4852c9, F=0x0
> 0,1281293,1281293,40040,12065, 0xfb7954c3, F=0x0
> 0,1
Re: [FFmpeg-devel] [PATCH] avcodec/h264_mp4toannexb: Prepend SPS/PPS to buffering period SEI
On Mon, 15 Jul 2024 at 10:48, Josh Allmann wrote: > > On Tue, 9 Jul 2024 at 12:06, Josh Allmann wrote: > > > > Encoders may emit a buffering period SEI without a corresponding > > SPS/PPS if the SPS/PPS is carried out-of-band, eg with avcc. > > > > During Annex B conversion, this may result in the SPS/PPS being > > inserted *after* the buffering period SEI but before the IDR NAL. > > > > Since the buffering period SEI references the SPS, the SPS/PPS > > needs to come first. > > --- > > > > Notes: > > v2: Updated FATE test refs > > > > libavcodec/bsf/h264_mp4toannexb.c | 13 + > > tests/ref/fate/h264-bsf-mp4toannexb| 2 +- > > tests/ref/fate/h264_mp4toannexb_ticket2991 | 18 +- > > tests/ref/fate/segment-mp4-to-ts | 12 ++-- > > 4 files changed, 29 insertions(+), 16 deletions(-) > > > > Ping for review. Looking at the FATE output, this patch fixes a number > of things - see [1] for details > > [1] https://ffmpeg.org//pipermail/ffmpeg-devel/2024-July/330964.html Pasted the wrong link - please see this for a review of the FATE test changes https://ffmpeg.org//pipermail/ffmpeg-devel/2024-July/330912.html ___ 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] avcodec/h264_mp4toannexb: Prepend SPS/PPS to buffering period SEI
On Tue, 9 Jul 2024 at 12:06, Josh Allmann wrote: > > Encoders may emit a buffering period SEI without a corresponding > SPS/PPS if the SPS/PPS is carried out-of-band, eg with avcc. > > During Annex B conversion, this may result in the SPS/PPS being > inserted *after* the buffering period SEI but before the IDR NAL. > > Since the buffering period SEI references the SPS, the SPS/PPS > needs to come first. > --- > > Notes: > v2: Updated FATE test refs > > libavcodec/bsf/h264_mp4toannexb.c | 13 + > tests/ref/fate/h264-bsf-mp4toannexb| 2 +- > tests/ref/fate/h264_mp4toannexb_ticket2991 | 18 +- > tests/ref/fate/segment-mp4-to-ts | 12 ++-- > 4 files changed, 29 insertions(+), 16 deletions(-) > Ping again for review. Looking at the FATE output, this patch fixes a number of things - see [1] for details [1] https://ffmpeg.org//pipermail/ffmpeg-devel/2024-July/330912.html ___ 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] avcodec/h264_mp4toannexb: Prepend SPS/PPS to buffering period SEI
On Mon, 22 Jul 2024 at 17:17, Timo Rothenpieler wrote: > > On 23/07/2024 01:01, Josh Allmann wrote: > > On Tue, 9 Jul 2024 at 12:06, Josh Allmann wrote: > >> > >> Encoders may emit a buffering period SEI without a corresponding > >> SPS/PPS if the SPS/PPS is carried out-of-band, eg with avcc. > >> > >> During Annex B conversion, this may result in the SPS/PPS being > >> inserted *after* the buffering period SEI but before the IDR NAL. > >> > >> Since the buffering period SEI references the SPS, the SPS/PPS > >> needs to come first. > >> --- > >> > >> Notes: > >> v2: Updated FATE test refs > >> > >> libavcodec/bsf/h264_mp4toannexb.c | 13 + > >> tests/ref/fate/h264-bsf-mp4toannexb| 2 +- > >> tests/ref/fate/h264_mp4toannexb_ticket2991 | 18 +- > >> tests/ref/fate/segment-mp4-to-ts | 12 ++-- > >> 4 files changed, 29 insertions(+), 16 deletions(-) > >> > > > > Ping again for review. Looking at the FATE output, this patch fixes a number > > of things - see [1] for details > > patch generally looks good to me, but I'm not closely familiar with the > code there. > Thanks, is there anyone else more familiar with the code who can also sign off on this patch? Josh ___ 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] avcodec/h264_mp4toannexb: Prepend SPS/PPS to buffering period SEI
Encoders may emit a buffering period SEI without a corresponding
SPS/PPS if the SPS/PPS is carried out-of-band, eg with avcc.
During Annex B conversion, this may result in the SPS/PPS being
inserted *after* the buffering period SEI but before the IDR NAL.
Since the buffering period SEI references the SPS, the SPS/PPS
needs to come first.
---
libavcodec/bsf/h264_mp4toannexb.c | 15 +++
tests/ref/fate/h264-bsf-mp4toannexb| 2 +-
tests/ref/fate/h264_mp4toannexb_ticket2991 | 18 +-
tests/ref/fate/segment-mp4-to-ts | 12 ++--
4 files changed, 31 insertions(+), 16 deletions(-)
diff --git a/libavcodec/bsf/h264_mp4toannexb.c
b/libavcodec/bsf/h264_mp4toannexb.c
index 92af6a6881..dda064287e 100644
--- a/libavcodec/bsf/h264_mp4toannexb.c
+++ b/libavcodec/bsf/h264_mp4toannexb.c
@@ -30,6 +30,7 @@
#include "bytestream.h"
#include "defs.h"
#include "h264.h"
+#include "sei.h"
typedef struct H264BSFContext {
uint8_t *sps;
@@ -363,6 +364,20 @@ static int h264_mp4toannexb_filter(AVBSFContext *ctx,
AVPacket *opkt)
if (!new_idr && unit_type == H264_NAL_IDR_SLICE && (buf[1] & 0x80))
new_idr = 1;
+/* If this is a buffering period SEI without a corresponding
sps/pps
+ * then prepend any existing sps/pps before the SEI */
+if (unit_type == H264_NAL_SEI && buf[1] ==
SEI_TYPE_BUFFERING_PERIOD &&
+!sps_seen && !pps_seen) {
+if (s->sps_size) {
+count_or_copy(&out, &out_size, s->sps, s->sps_size,
PS_OUT_OF_BAND, j);
+sps_seen = 1;
+}
+if (s->pps_size) {
+count_or_copy(&out, &out_size, s->pps, s->pps_size,
PS_OUT_OF_BAND, j);
+pps_seen = 1;
+}
+}
+
/* prepend only to the first type 5 NAL unit of an IDR picture, if
no sps/pps are already present */
if (new_idr && unit_type == H264_NAL_IDR_SLICE && !sps_seen &&
!pps_seen) {
if (s->sps_size)
diff --git a/tests/ref/fate/h264-bsf-mp4toannexb
b/tests/ref/fate/h264-bsf-mp4toannexb
index 2049f39701..81ff568f3d 100644
--- a/tests/ref/fate/h264-bsf-mp4toannexb
+++ b/tests/ref/fate/h264-bsf-mp4toannexb
@@ -1 +1 @@
-5f04c27cc6ee8625fe2405fb0f7da9a3
+ff2551123909f54c382294baa1bb4364
diff --git a/tests/ref/fate/h264_mp4toannexb_ticket2991
b/tests/ref/fate/h264_mp4toannexb_ticket2991
index f8e3e920d4..9a1fbf2f8c 100644
--- a/tests/ref/fate/h264_mp4toannexb_ticket2991
+++ b/tests/ref/fate/h264_mp4toannexb_ticket2991
@@ -1,4 +1,4 @@
-05d66e60ab22ee004720e0051af0fe74
*tests/data/fate/h264_mp4toannexb_ticket2991.h264
+b6ff5910928ad0b2a7eec481dcc41594
*tests/data/fate/h264_mp4toannexb_ticket2991.h264
1985815 tests/data/fate/h264_mp4toannexb_ticket2991.h264
#extradata 0: 47, 0x3a590d55
#tb 0: 1/120
@@ -6,7 +6,7 @@
#codec_id 0: h264
#dimensions 0: 1280x720
#sar 0: 3/4
-0, 0, 0,40040,37126, 0xb020184c
+0, 0, 0,40040,37126, 0x515c184c
0, 40040, 40040,40040, 6920, 0x8512361a, F=0x0
0, 80081, 80081,40040, 7550, 0x1bc56ed4, F=0x0
0, 120121, 120121,40040, 8752, 0xb8c6f0a1, F=0x0
@@ -21,7 +21,7 @@
0, 480485, 480485,40040,11234, 0x83cbd9fd, F=0x0
0, 520525, 520525,40040,17616, 0xfdf95104, F=0x0
0, 560566, 560566,40040,10689, 0x9633d32b, F=0x0
-0, 600606, 600606,40040,45291, 0x543c2cf6
+0, 600606, 600606,40040,45291, 0xa8292cf6
0, 640646, 640646,40040,20837, 0x051abfab, F=0x0
0, 680687, 680687,40040,21418, 0xe2a59d70, F=0x0
0, 720727, 720727,40040,15643, 0x15cf2cec, F=0x0
@@ -36,7 +36,7 @@
0,1081091,1081091,40040,13130, 0xcbb6bb8e, F=0x0
0,1121131,1121131,40040,16180, 0x5d188a7a, F=0x0
0,1161172,1161172,40040,14961, 0x9ff2f463, F=0x0
-0,1201212,1201212,40040,54296, 0xe6ec30ed
+0,1201212,1201212,40040,54296, 0x3ae830ed
0,1241252,1241252,40040,11500, 0x8c4852c9, F=0x0
0,1281293,1281293,40040,12065, 0xfb7954c3, F=0x0
0,1321333,1321333,40040,12532, 0xf0a935d3, F=0x0
@@ -51,7 +51,7 @@
0,1681697,1681697,40040,13250, 0xfed0deb8, F=0x0
0,1721737,1721737,40040,13360, 0xbf92d476, F=0x0
0,1761778,1761778,40040,11749, 0x3041eaf1, F=0x0
-0,1801818,1801818,40040,23997, 0xdbe6d5c4
+0,1801818,1801818,40040,23997, 0x2fe2d5c4
0,1841858,1841858,40040,16065, 0xe8f715b7, F=0x0
0,1881899,1881899,40040,16441, 0x0a4e060f, F=0x0
0,1921939,1921939,40040,17395, 0xa8edecc2, F=0x0
@@ -66,7 +66,7 @@
0,2282303,2282303,40040,13748, 0xed26aeb4, F=0x0
Re: [FFmpeg-devel] [PATCH] avcodec/h264_mp4toannexb: Prepend SPS/PPS to buffering period SEI
On Thu, 1 Aug 2024 at 00:58, Anton Khirnov wrote:
>
Thanks for the review.
> Quoting Josh Allmann (2024-07-09 21:05:47)
> > Encoders may emit a buffering period SEI without a corresponding
> > SPS/PPS if the SPS/PPS is carried out-of-band, eg with avcc.
> >
> > During Annex B conversion, this may result in the SPS/PPS being
> > inserted *after* the buffering period SEI but before the IDR NAL.
> >
> > Since the buffering period SEI references the SPS, the SPS/PPS
> > needs to come first.
> > ---
> >
> > Notes:
> > v2: Updated FATE test refs
> >
> > libavcodec/bsf/h264_mp4toannexb.c | 13 +
> > tests/ref/fate/h264-bsf-mp4toannexb| 2 +-
> > tests/ref/fate/h264_mp4toannexb_ticket2991 | 18 +-
> > tests/ref/fate/segment-mp4-to-ts | 12 ++--
> > 4 files changed, 29 insertions(+), 16 deletions(-)
> >
> > diff --git a/libavcodec/bsf/h264_mp4toannexb.c
> > b/libavcodec/bsf/h264_mp4toannexb.c
> > index 92af6a6881..6607f1e91a 100644
> > --- a/libavcodec/bsf/h264_mp4toannexb.c
> > +++ b/libavcodec/bsf/h264_mp4toannexb.c
> > @@ -363,6 +363,19 @@ static int h264_mp4toannexb_filter(AVBSFContext *ctx,
> > AVPacket *opkt)
> > if (!new_idr && unit_type == H264_NAL_IDR_SLICE && (buf[1] &
> > 0x80))
> > new_idr = 1;
> >
> > +/* If this is a buffering period SEI without a corresponding
> > sps/pps
> > + * then prepend any existing sps/pps before the SEI */
> > +if (unit_type == H264_NAL_SEI && buf[1] == 0 && !sps_seen &&
> > !pps_seen) {
>
> That 0 should be SEI_TYPE_BUFFERING_PERIOD, right?
>
Yes - fixed
> > +if (s->sps_size) {
> > +count_or_copy(&out, &out_size, s->sps, s->sps_size,
> > PS_OUT_OF_BAND, j);
> > +sps_seen = 1;
> > +}
> > +if (s->pps_size) {
> > +count_or_copy(&out, &out_size, s->pps, s->pps_size,
> > PS_OUT_OF_BAND, j);
> > +pps_seen = 1;
> > +}
>
> Is there a reason to insert the PPS? IIUC only the SPS is needed.
>
I believe it would be needed if using this bsf with the segment muxer,
and segmentation happens on a recovery point (with a buffering
period), eg in the test fate-segment-mp4-to-ts . Granted it is kind of
incidental that this patch actually fixes that specific case. I have
never seen a SPS without a PPS though.
Josh
___
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] avcodec/h264_mp4toannexb: Prepend SPS/PPS to buffering period SEI
On Thu, 1 Aug 2024 at 14:37, Josh Allmann wrote:
>
> Encoders may emit a buffering period SEI without a corresponding
> SPS/PPS if the SPS/PPS is carried out-of-band, eg with avcc.
>
> During Annex B conversion, this may result in the SPS/PPS being
> inserted *after* the buffering period SEI but before the IDR NAL.
>
> Since the buffering period SEI references the SPS, the SPS/PPS
> needs to come first.
> ---
> libavcodec/bsf/h264_mp4toannexb.c | 15 +++
> tests/ref/fate/h264-bsf-mp4toannexb| 2 +-
> tests/ref/fate/h264_mp4toannexb_ticket2991 | 18 +-
> tests/ref/fate/segment-mp4-to-ts | 12 ++--
> 4 files changed, 31 insertions(+), 16 deletions(-)
>
Ping for (re-)review on this patch which addresses comments from [1]
Explanation for the FATE changes here [2] - it turns out that several
of the FATE samples exhibit the same behavior that this patch fixes,
so it is a net improvement
[1] https://ffmpeg.org//pipermail/ffmpeg-devel/2024-August/331958.html
[2] https://ffmpeg.org//pipermail/ffmpeg-devel/2024-July/330912.html
Josh
> diff --git a/libavcodec/bsf/h264_mp4toannexb.c
> b/libavcodec/bsf/h264_mp4toannexb.c
> index 92af6a6881..dda064287e 100644
> --- a/libavcodec/bsf/h264_mp4toannexb.c
> +++ b/libavcodec/bsf/h264_mp4toannexb.c
> @@ -30,6 +30,7 @@
> #include "bytestream.h"
> #include "defs.h"
> #include "h264.h"
> +#include "sei.h"
>
> typedef struct H264BSFContext {
> uint8_t *sps;
> @@ -363,6 +364,20 @@ static int h264_mp4toannexb_filter(AVBSFContext *ctx,
> AVPacket *opkt)
> if (!new_idr && unit_type == H264_NAL_IDR_SLICE && (buf[1] &
> 0x80))
> new_idr = 1;
>
> +/* If this is a buffering period SEI without a corresponding
> sps/pps
> + * then prepend any existing sps/pps before the SEI */
> +if (unit_type == H264_NAL_SEI && buf[1] ==
> SEI_TYPE_BUFFERING_PERIOD &&
> +!sps_seen && !pps_seen) {
> +if (s->sps_size) {
> +count_or_copy(&out, &out_size, s->sps, s->sps_size,
> PS_OUT_OF_BAND, j);
> +sps_seen = 1;
> +}
> +if (s->pps_size) {
> +count_or_copy(&out, &out_size, s->pps, s->pps_size,
> PS_OUT_OF_BAND, j);
> +pps_seen = 1;
> +}
> +}
> +
> /* prepend only to the first type 5 NAL unit of an IDR picture,
> if no sps/pps are already present */
> if (new_idr && unit_type == H264_NAL_IDR_SLICE && !sps_seen &&
> !pps_seen) {
> if (s->sps_size)
> diff --git a/tests/ref/fate/h264-bsf-mp4toannexb
> b/tests/ref/fate/h264-bsf-mp4toannexb
> index 2049f39701..81ff568f3d 100644
> --- a/tests/ref/fate/h264-bsf-mp4toannexb
> +++ b/tests/ref/fate/h264-bsf-mp4toannexb
> @@ -1 +1 @@
> -5f04c27cc6ee8625fe2405fb0f7da9a3
> +ff2551123909f54c382294baa1bb4364
> diff --git a/tests/ref/fate/h264_mp4toannexb_ticket2991
> b/tests/ref/fate/h264_mp4toannexb_ticket2991
> index f8e3e920d4..9a1fbf2f8c 100644
> --- a/tests/ref/fate/h264_mp4toannexb_ticket2991
> +++ b/tests/ref/fate/h264_mp4toannexb_ticket2991
> @@ -1,4 +1,4 @@
> -05d66e60ab22ee004720e0051af0fe74
> *tests/data/fate/h264_mp4toannexb_ticket2991.h264
> +b6ff5910928ad0b2a7eec481dcc41594
> *tests/data/fate/h264_mp4toannexb_ticket2991.h264
> 1985815 tests/data/fate/h264_mp4toannexb_ticket2991.h264
> #extradata 0: 47, 0x3a590d55
> #tb 0: 1/120
> @@ -6,7 +6,7 @@
> #codec_id 0: h264
> #dimensions 0: 1280x720
> #sar 0: 3/4
> -0, 0, 0,40040,37126, 0xb020184c
> +0, 0, 0,40040,37126, 0x515c184c
> 0, 40040, 40040,40040, 6920, 0x8512361a, F=0x0
> 0, 80081, 80081,40040, 7550, 0x1bc56ed4, F=0x0
> 0, 120121, 120121,40040, 8752, 0xb8c6f0a1, F=0x0
> @@ -21,7 +21,7 @@
> 0, 480485, 480485,40040,11234, 0x83cbd9fd, F=0x0
> 0, 520525, 520525,40040,17616, 0xfdf95104, F=0x0
> 0, 560566, 560566,40040,10689, 0x9633d32b, F=0x0
> -0, 600606, 600606,40040,45291, 0x543c2cf6
> +0, 600606, 600606,40040,45291, 0xa8292cf6
> 0, 640646, 640646,40040,20837, 0x051abfab, F=0x0
> 0, 680687, 680687,40040,21418, 0xe2a59d70, F=0x0
> 0, 720727, 720727,40040,15643, 0x15cf2cec, F=0x0
> @@ -36,7 +36,7 @@
> 0,1081091,1081091,40040,13130, 0xcbb6bb
Re: [FFmpeg-devel] [PATCH] avcodec/h264_mp4toannexb: Prepend SPS/PPS to buffering period SEI
On Wed, 7 Aug 2024 at 09:13, Josh Allmann wrote: > > On Thu, 1 Aug 2024 at 14:37, Josh Allmann wrote: > > > > Encoders may emit a buffering period SEI without a corresponding > > SPS/PPS if the SPS/PPS is carried out-of-band, eg with avcc. > > > > During Annex B conversion, this may result in the SPS/PPS being > > inserted *after* the buffering period SEI but before the IDR NAL. > > > > Since the buffering period SEI references the SPS, the SPS/PPS > > needs to come first. > > --- > > libavcodec/bsf/h264_mp4toannexb.c | 15 +++ > > tests/ref/fate/h264-bsf-mp4toannexb| 2 +- > > tests/ref/fate/h264_mp4toannexb_ticket2991 | 18 +- > > tests/ref/fate/segment-mp4-to-ts | 12 ++-- > > 4 files changed, 31 insertions(+), 16 deletions(-) > > > > Ping for (re-)review on this patch which addresses comments from [1] > > Explanation for the FATE changes here [2] - it turns out that several > of the FATE samples exhibit the same behavior that this patch fixes, > so it is a net improvement > > [1] https://ffmpeg.org//pipermail/ffmpeg-devel/2024-August/331958.html > [2] https://ffmpeg.org//pipermail/ffmpeg-devel/2024-July/330912.html > Gentle ping for re-review. Josh ___ 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] avformat/movenc: Remove dts delay from duration.
A negative start_dts value (eg, delay from edit lists) typically yields
a duration larger than end_pts. During edit list processing, the
delay is removed again, yielding the correct duration within the elst.
However, other duration-carrying atoms (tkhd, mvhd, mdhd) still have
the delay incorporated into their durations. This is incorrect.
Fix this by withholding delay from the duration if edit lists are used.
This also simplifies edit-list processing a bit, since the delay
does not need to be removed from the calculated duration again.
---
The mov spec says that the tkhd duration is "derived from the track's
edits" [1] and the duratons of the other atoms (mvhd, mdhd) are in turn
taken from the longest track. So it seems that incorporating the delay
into the track duration is a bug in itself when the edit list has the
correct duration, and this propagates out tothe other top-level durations.
Unsure of how this change interacts with other modes that may expect
negative timestamps such as CMAF, so the patch errs on the side of
caution and only takes effect if edit lists are used. Can loosen that
up if necessary.
[1]
https://developer.apple.com/library/archive/documentation/QuickTime/QTFF/QTFFChap2/qtff2.html#//apple_ref/doc/uid/TP4939-CH204-BBCEIDFA
libavformat/movenc.c | 13 -
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/libavformat/movenc.c b/libavformat/movenc.c
index 7db2e28840..31441a9f6c 100644
--- a/libavformat/movenc.c
+++ b/libavformat/movenc.c
@@ -2831,7 +2831,14 @@ static int64_t calc_pts_duration(MOVMuxContext *mov,
MOVTrack *track)
if (track->end_pts != AV_NOPTS_VALUE &&
track->start_dts != AV_NOPTS_VALUE &&
track->start_cts != AV_NOPTS_VALUE) {
-return track->end_pts - (track->start_dts + track->start_cts);
+int64_t dur = track->end_pts, delay = track->start_dts +
track->start_cts;
+/* Note, this delay is calculated from the pts of the first sample,
+ * ensuring that we don't reduce the duration for cases with
+ * dts<0 pts=0. */
+if (delay > 0 || !mov->use_editlist) {
+ dur -= delay;
+}
+return dur;
}
return track->track_duration;
}
@@ -3118,10 +3125,6 @@ static int mov_write_edts_tag(AVIOContext *pb,
MOVMuxContext *mov,
* rounded to 0 when represented in MOV_TIMESCALE units. */
av_assert0(av_rescale_rnd(start_dts, MOV_TIMESCALE, track->timescale,
AV_ROUND_DOWN) <= 0);
start_ct = -FFMIN(start_dts, 0);
-/* Note, this delay is calculated from the pts of the first sample,
- * ensuring that we don't reduce the duration for cases with
- * dts<0 pts=0. */
-duration += delay;
}
/* For fragmented files, we don't know the full length yet. Setting
--
2.24.3 (Apple Git-128)
___
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] avformat/movenc: Remove dts delay from duration.
On Fri, 11 Dec 2020 at 14:07, Martin Storsjö wrote:
>
> On Fri, 11 Dec 2020, Josh Allmann wrote:
>
> > A negative start_dts value (eg, delay from edit lists) typically yields
> > a duration larger than end_pts. During edit list processing, the
> > delay is removed again, yielding the correct duration within the elst.
> >
> > However, other duration-carrying atoms (tkhd, mvhd, mdhd) still have
> > the delay incorporated into their durations. This is incorrect.
> >
> > Fix this by withholding delay from the duration if edit lists are used.
> > This also simplifies edit-list processing a bit, since the delay
> > does not need to be removed from the calculated duration again.
> > ---
> >
> > The mov spec says that the tkhd duration is "derived from the track's
> > edits" [1] and the duratons of the other atoms (mvhd, mdhd) are in turn
> > taken from the longest track. So it seems that incorporating the delay
> > into the track duration is a bug in itself when the edit list has the
> > correct duration, and this propagates out tothe other top-level durations.
> >
> > Unsure of how this change interacts with other modes that may expect
> > negative timestamps such as CMAF, so the patch errs on the side of
> > caution and only takes effect if edit lists are used. Can loosen that
> > up if necessary.
> >
> > [1]
> > https://developer.apple.com/library/archive/documentation/QuickTime/QTFF/QTFFChap2/qtff2.html#//apple_ref/doc/uid/TP4939-CH204-BBCEIDFA
> >
> > libavformat/movenc.c | 13 -
> > 1 file changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/libavformat/movenc.c b/libavformat/movenc.c
> > index 7db2e28840..31441a9f6c 100644
> > --- a/libavformat/movenc.c
> > +++ b/libavformat/movenc.c
> > @@ -2831,7 +2831,14 @@ static int64_t calc_pts_duration(MOVMuxContext *mov,
> > MOVTrack *track)
> > if (track->end_pts != AV_NOPTS_VALUE &&
> > track->start_dts != AV_NOPTS_VALUE &&
> > track->start_cts != AV_NOPTS_VALUE) {
> > -return track->end_pts - (track->start_dts + track->start_cts);
> > +int64_t dur = track->end_pts, delay = track->start_dts +
> > track->start_cts;
> > +/* Note, this delay is calculated from the pts of the first sample,
> > + * ensuring that we don't reduce the duration for cases with
> > + * dts<0 pts=0. */
>
> If you have a stream starting with dts<0 pts=0, you'll have start_pts =
> start_dts + start_cts = 0. That gives delay=0 after your modification. But
> the comment says "don't reduce the duration for cases with pts=0" - where
> the delay variable would be zero anyway?
>
I'm not quite sure what you mean - that the comment is outdated?
Or that this modification would perhaps not behave as expected?
For what it's worth, the cases I'm concerned with have start_pts < 0.
>
>
> I don't manage to follow the reasoning and explanation in the commit
> message. To be able to concretely reason about this issue at all, we need
> to look at a concrete example. Can you provide a sample input file and a
> reproducible command, and point out which exact field in the muxer output
> of that case that you consider wrong?
>
Had to create a trac to find somewhere to host the sample. Tried to put
some details there but the formatting seems messed up and I can't figure
out how to edit, apologies. So here is some more info -
Input sample:
https://trac.ffmpeg.org/raw-attachment/ticket/9028/test-timecode.mp4
Run the following for a transmuxed clip from 3s for a 5s duration:
ffmpeg -ss 3 -i test-timecode.mp4 -t 5 -c copy out.mp4
Note that the actual cut location is mid-GOP, so there's a 1s pts delay
at the beginning of the output file with negative pts.
ffprobe shows:
ffprobe -show_streams -show_format out.mp4 2>&1 | grep duration=
duration=5.166992 # stream duration - correct
duration=6.167000 # format duration - incorrect
mp4dump'ing out.mp4 gives this:
# incorrect: duration should be sum of elst durations
[tkhd] size=12+80, flags=3
duration = 6167
# correct
[elst] size=12+16
entry_count = 1
entry/segment duration = 5167
# incorrect; derived from track duration (tkhd)
[mvhd] size=12+96
timescale = 1000
duration = 6167
> // Martin
>
> ___
> 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 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] avformat/movenc: Remove dts delay from duration.
Hi Martin,
On Sat, 19 Dec 2020 at 15:10, Martin Storsjö wrote:
>
> On Fri, 11 Dec 2020, Josh Allmann wrote:
>
> > On Fri, 11 Dec 2020 at 14:07, Martin Storsjö wrote:
> >>
> >> On Fri, 11 Dec 2020, Josh Allmann wrote:
> >>
> >> > A negative start_dts value (eg, delay from edit lists) typically yields
> >> > a duration larger than end_pts. During edit list processing, the
> >> > delay is removed again, yielding the correct duration within the elst.
> >> >
> >> > However, other duration-carrying atoms (tkhd, mvhd, mdhd) still have
> >> > the delay incorporated into their durations. This is incorrect.
> >> >
> >> > Fix this by withholding delay from the duration if edit lists are used.
> >> > This also simplifies edit-list processing a bit, since the delay
> >> > does not need to be removed from the calculated duration again.
> >> > ---
> >> >
> >> > The mov spec says that the tkhd duration is "derived from the track's
> >> > edits" [1] and the duratons of the other atoms (mvhd, mdhd) are in turn
> >> > taken from the longest track. So it seems that incorporating the delay
> >> > into the track duration is a bug in itself when the edit list has the
> >> > correct duration, and this propagates out tothe other top-level
> >> > durations.
> >> >
> >> > Unsure of how this change interacts with other modes that may expect
> >> > negative timestamps such as CMAF, so the patch errs on the side of
> >> > caution and only takes effect if edit lists are used. Can loosen that
> >> > up if necessary.
> >> >
> >> > [1]
> >> > https://developer.apple.com/library/archive/documentation/QuickTime/QTFF/QTFFChap2/qtff2.html#//apple_ref/doc/uid/TP4939-CH204-BBCEIDFA
> >> >
> >> > libavformat/movenc.c | 13 -
> >> > 1 file changed, 8 insertions(+), 5 deletions(-)
> >> >
> >> > diff --git a/libavformat/movenc.c b/libavformat/movenc.c
> >> > index 7db2e28840..31441a9f6c 100644
> >> > --- a/libavformat/movenc.c
> >> > +++ b/libavformat/movenc.c
> >> > @@ -2831,7 +2831,14 @@ static int64_t calc_pts_duration(MOVMuxContext
> >> > *mov, MOVTrack *track)
> >> > if (track->end_pts != AV_NOPTS_VALUE &&
> >> > track->start_dts != AV_NOPTS_VALUE &&
> >> > track->start_cts != AV_NOPTS_VALUE) {
> >> > -return track->end_pts - (track->start_dts + track->start_cts);
> >> > +int64_t dur = track->end_pts, delay = track->start_dts +
> >> > track->start_cts;
> >> > +/* Note, this delay is calculated from the pts of the first
> >> > sample,
> >> > + * ensuring that we don't reduce the duration for cases with
> >> > + * dts<0 pts=0. */
> >>
> >> If you have a stream starting with dts<0 pts=0, you'll have start_pts =
> >> start_dts + start_cts = 0. That gives delay=0 after your modification. But
> >> the comment says "don't reduce the duration for cases with pts=0" - where
> >> the delay variable would be zero anyway?
> >>
> >
> > I'm not quite sure what you mean - that the comment is outdated?
> > Or that this modification would perhaps not behave as expected?
> >
> > For what it's worth, the cases I'm concerned with have start_pts < 0.
> >
> >>
> >>
> >> I don't manage to follow the reasoning and explanation in the commit
> >> message. To be able to concretely reason about this issue at all, we need
> >> to look at a concrete example. Can you provide a sample input file and a
> >> reproducible command, and point out which exact field in the muxer output
> >> of that case that you consider wrong?
> >>
> >
> > Had to create a trac to find somewhere to host the sample. Tried to put
> > some details there but the formatting seems messed up and I can't figure
> > out how to edit, apologies. So here is some more info -
> >
> > Input sample:
> >
> > https://trac.ffmpeg.org/raw-attachment/ticket/9028/test-timecode.mp4
> >
> > Run the following for a transmuxed clip from 3s for a 5s duration:
> >
> > ffmpeg -ss 3 -i test-timecode.mp4 -t 5 -c copy out.mp4
> >
> > Note that the actual cut location is mid-GOP, so there's a 1s
Re: [FFmpeg-devel] [PATCH] avformat/movenc: Remove dts delay from duration.
Hi Martin, On Fri, 15 Jan 2021 at 04:59, Martin Storsjö wrote: > > Hi Josh, > > On Tue, 22 Dec 2020, Josh Allmann wrote: > > > Thank you for taking the time to look into this! Tested with your > > alternative patch and it does seem to be an improvement. I was able to > > find a case where it didn't seem to do the correct thing (described > > below), but this is not a regression; master doesn't do the correct > > thing, and neither does my patch. I haven't looked much at what the > > code is doing beyond running these tests, but could find some time to > > dig in early 2021 if it's not fixed by then. > > > > I think this issue isn't related to the mov muxer at least, but is more > related to how stream copy works at the ffmpeg.c level, and/or how the > seeking is done. > Thanks for the tip - still haven't had a chance to investigate the behavior. > As that's unrelated, and I haven't heard any objections to my version of > the patch, I think I'll go ahead and land it soon then. > The patch is certainly an improvement over the current behavior, no objections from me. Josh ___ 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".
