| Message ID | 20180727145749.9436-6-jamrial@gmail.com |
|---|---|
| State | Accepted |
| Commit | 0e27e2767001d22fd25164b87b42793a9e9bcfc3 |
| Headers | show |
On Fri, Jul 27, 2018 at 11:57:47AM -0300, James Almer wrote: > Signed-off-by: James Almer <jamrial@gmail.com> > --- > I'm not 100% sure this is correct. I also don't know if the CBS contexts need > to be fully reinitialized or not in this scenario. Because if so, then every > bsf using cbs will require a flush() callback as well. > > libavcodec/h264_redundant_pps_bsf.c | 9 +++++++++ > 1 file changed, 9 insertions(+) Do we have a fate test for this ? Also from a quick look, isnt this filter lacking checks ? it removes PPS but i see no check that these PPS differ only in the qp related parameters. Also doesnt this filter miss that there can be multiple PPS with different id# Maybe iam missing something and this is of course not about the patch at hand but it looks like this is a quite specific filter that cannot saftely be applied to h264 in general and if so this is not obvious from the documentation. thx [...]
On 7/28/2018 6:59 PM, Michael Niedermayer wrote: > On Fri, Jul 27, 2018 at 11:57:47AM -0300, James Almer wrote: >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> I'm not 100% sure this is correct. I also don't know if the CBS contexts need >> to be fully reinitialized or not in this scenario. Because if so, then every >> bsf using cbs will require a flush() callback as well. >> >> libavcodec/h264_redundant_pps_bsf.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) > > Do we have a fate test for this ? It doesn't look like this filter is tested. but even if it were, no decoder autoinserts it, so this flush() function would not be covered by any test either way. > Also from a quick look, isnt this filter lacking checks ? > it removes PPS but i see no check that these PPS differ only in the qp > related parameters. Also doesnt this filter miss that there can be multiple > PPS with different id# > > Maybe iam missing something and this is of course not about the > patch at hand but it looks like this is a quite specific filter that > cannot saftely be applied to h264 in general and if so this is not > obvious from the documentation. Mark is probably the best person to answer this. > > thx > > [...] > > > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
On 28/07/18 22:59, Michael Niedermayer wrote: > On Fri, Jul 27, 2018 at 11:57:47AM -0300, James Almer wrote: >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> I'm not 100% sure this is correct. I also don't know if the CBS contexts need >> to be fully reinitialized or not in this scenario. Because if so, then every >> bsf using cbs will require a flush() callback as well. >> >> libavcodec/h264_redundant_pps_bsf.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) > > Do we have a fate test for this ? No - I don't have a public file it applies to. (I can share an example privately.) > Also from a quick look, isnt this filter lacking checks ? > it removes PPS but i see no check that these PPS differ only in the qp > related parameters. Also doesnt this filter miss that there can be multiple > PPS with different id# The problematic streams repeatedly overwrite PPS id 0 with different parameters, and that's what we're fixing up here - using a stream like that in any case with global extradata fails on seeking or fragmenting. > Maybe iam missing something and this is of course not about the > patch at hand but it looks like this is a quite specific filter that > cannot saftely be applied to h264 in general and if so this is not > obvious from the documentation. The documentation says the filter "applies a specific fixup to some Blu-ray streams", which implies that it doesn't apply to anything else? Feel free to add some additional text which clarifies it to your reading if you don't agree. Thanks, - Mark
On 8/1/2018 7:20 PM, Mark Thompson wrote:
> The problematic streams repeatedly overwrite PPS id 0 with different parameters, and that's what we're fixing up here - using a stream like that in any case with global extradata fails on seeking or fragmenting.
So is this patch correct? Should i even add a flush() function, for that
matter? It's obvious this filter will never be autoinserted by any of
our decoders, but we can't say what API users may want to do.
On Wed, Aug 01, 2018 at 11:20:46PM +0100, Mark Thompson wrote: > On 28/07/18 22:59, Michael Niedermayer wrote: > > On Fri, Jul 27, 2018 at 11:57:47AM -0300, James Almer wrote: > >> Signed-off-by: James Almer <jamrial@gmail.com> > >> --- > >> I'm not 100% sure this is correct. I also don't know if the CBS contexts need > >> to be fully reinitialized or not in this scenario. Because if so, then every > >> bsf using cbs will require a flush() callback as well. > >> > >> libavcodec/h264_redundant_pps_bsf.c | 9 +++++++++ > >> 1 file changed, 9 insertions(+) > > > > Do we have a fate test for this ? > > No - I don't have a public file it applies to. (I can share an example privately.) > > > Also from a quick look, isnt this filter lacking checks ? > > it removes PPS but i see no check that these PPS differ only in the qp > > related parameters. Also doesnt this filter miss that there can be multiple > > PPS with different id# > > The problematic streams repeatedly overwrite PPS id 0 with different parameters, and that's what we're fixing up here - using a stream like that in any case with global extradata fails on seeking or fragmenting. > > > Maybe iam missing something and this is of course not about the > > patch at hand but it looks like this is a quite specific filter that > > cannot saftely be applied to h264 in general and if so this is not > > obvious from the documentation. > > The documentation says the filter "applies a specific fixup to some Blu-ray streams", which implies that it doesn't apply to anything else? Feel free to add some additional text which clarifies it to your reading if you don't agree. I think we interpret this differently. "applies F to B" to me does not imply anything about "Not B". And naivly i would have expected that it does not break "Not B". Being able to "just" apply this filter to all h264 without the need to manually first check each file if it is originating from blue-ray seems a realistic use case. I mean it could be a bit incovenient if one had a large amount of diverse input material. More so if not all is h264 ... Obviously this was not what others had in mind I can suggest a patch to clarify the docs if there is no interrest in making tha filter safe to "just" add thx [...]
diff --git a/libavcodec/h264_redundant_pps_bsf.c b/libavcodec/h264_redundant_pps_bsf.c index 26baca84e3..cc5a3060f5 100644 --- a/libavcodec/h264_redundant_pps_bsf.c +++ b/libavcodec/h264_redundant_pps_bsf.c @@ -35,6 +35,7 @@ typedef struct H264RedundantPPSContext { int global_pic_init_qp; int current_pic_init_qp; + int extradata_pic_init_qp; } H264RedundantPPSContext; @@ -145,6 +146,7 @@ static int h264_redundant_pps_init(AVBSFContext *bsf) h264_redundant_pps_fixup_pps(ctx, au->units[i].content); } + ctx->extradata_pic_init_qp = ctx->current_pic_init_qp; err = ff_cbs_write_extradata(ctx->output, bsf->par_out, au); if (err < 0) { av_log(bsf, AV_LOG_ERROR, "Failed to write extradata.\n"); @@ -157,6 +159,12 @@ static int h264_redundant_pps_init(AVBSFContext *bsf) return 0; } +static void h264_redundant_pps_flush(AVBSFContext *bsf) +{ + H264RedundantPPSContext *ctx = bsf->priv_data; + ctx->current_pic_init_qp = ctx->extradata_pic_init_qp; +} + static void h264_redundant_pps_close(AVBSFContext *bsf) { H264RedundantPPSContext *ctx = bsf->priv_data; @@ -172,6 +180,7 @@ const AVBitStreamFilter ff_h264_redundant_pps_bsf = { .name = "h264_redundant_pps", .priv_data_size = sizeof(H264RedundantPPSContext), .init = &h264_redundant_pps_init, + .flush = &h264_redundant_pps_flush, .close = &h264_redundant_pps_close, .filter = &h264_redundant_pps_filter, .codec_ids = h264_redundant_pps_codec_ids,
Signed-off-by: James Almer <jamrial@gmail.com> --- I'm not 100% sure this is correct. I also don't know if the CBS contexts need to be fully reinitialized or not in this scenario. Because if so, then every bsf using cbs will require a flush() callback as well. libavcodec/h264_redundant_pps_bsf.c | 9 +++++++++ 1 file changed, 9 insertions(+)