Re: [FFmpeg-devel] [PATCH 25/31] cbs: Add function to update video codec parameters
On 29/07/2019 02:33, Andreas Rheinhardt wrote:
> Mark Thompson:
>> On 20/06/2019 00:45, Andreas Rheinhardt wrote:
>>> If any of the *_metadata filter based upon cbs currently updates a video
>>> codec parameter like color information, the AVCodecParameters are not
>>> updated accordingly, so that e.g. muxers write header values based upon
>>> outdated information that may precede and thereby nullify the new values
>>> on the bitstream level.
>>> This commit adds a function to also update the video codec parameters
>>> so that the above situation can be fixed in a unified manner.
>>>
>>> Signed-off-by: Andreas Rheinhardt
>>> ---
>>> libavcodec/cbs.c | 35 +++
>>> libavcodec/cbs.h | 15 +++
>>> 2 files changed, 50 insertions(+)
>>>
>>> diff --git a/libavcodec/cbs.c b/libavcodec/cbs.c
>>> index 47679eca1b..37b080b5ba 100644
>>> --- a/libavcodec/cbs.c
>>> +++ b/libavcodec/cbs.c
>>> @@ -342,6 +342,41 @@ int ff_cbs_write_extradata(CodedBitstreamContext *ctx,
>>> return 0;
>>> }
>>>
>>> +void ff_cbs_update_video_parameters(CodedBitstreamContext *ctx,
>>
>> The context argument isn't used at all.
>>
> I know. I thought that's how you wanted the API to look like. After
> all, ff_cbs_fragment_uninit always used a context argument without
> needing it (cbs_unit_uninit doesn't use its context argument at all
> and it is the only place where ff_cbs_fragment_uninit used it; I of
> course kept this behaviour when replacing ff_cbs_fragment_uninit). Now
> that it seems that this was unintentional, I will send a patch to
> remove the argument.
Hmm. I think the real reason that seemed wrong to me was that it isn't really
a CBS function, rather a BSF function - it can be used by things with no other
CBS interaction, such as prores_metadata. So, maybe it should go in bsf.h
rather than cbs.h?
ff_cbs_fragment_uninit(), on the other hand, really is a CBS function, so even
though it doesn't currently use the context/logging argument it still feels
right to pass it.
>>> +AVCodecParameters *par, int profile,
>>> +int level, int width, int height,
>>> +int field_order, int color_range,
>>> +int color_primaries, int color_trc,
>>> +int color_space, int chroma_location,
>>> +int video_delay)
>>> +{
>>> +#define SET_IF_NONNEGATIVE(elem) \
>>> +if (elem >= 0) \
>>> +par->elem = elem;
>>> +SET_IF_NONNEGATIVE(profile)
>>> +SET_IF_NONNEGATIVE(level)
>>> +SET_IF_NONNEGATIVE(width)
>>> +SET_IF_NONNEGATIVE(height)
>>> +SET_IF_NONNEGATIVE(field_order)
>>> +SET_IF_NONNEGATIVE(video_delay)
>>> +#undef SET_IF_NONNEGATIVE
>>> +
>>> +#define SET_IF_VALID(elem, upper_bound) \
>>> +if (0 <= elem && elem < upper_bound) \
>>> +par->elem = elem;
>>> +SET_IF_VALID(color_range, AVCOL_RANGE_NB)
>>> +SET_IF_VALID(color_trc, AVCOL_TRC_NB)
>>> +SET_IF_VALID(color_space, AVCOL_SPC_NB)
>>
>> I think we generally want to admit future values for the 23001-8 / H.273
>> fields (see range ..255 on all the metadata filters).
>>
> Do you want to omit checking entirely? Or should I still check against
> the sentinel? I prefer the latter. I dislike setting an enum to
> something else than an enum constant and moreover, if a future
> standard would allow (say) color_range > 255 (or a bsf would simply
> forget to check the value), but libavcodec does not, then it might be
> that the compiler uses char as underlying type for the enum which of
> course could not hold such a value.
No future standard will backward-compatibly allow color_range > 255 because
it's an 8-bit field in all current use.
I'm purely considering the behaviour for a new N + 1 value, since other
components do allow that (for example, in
).
>>> +SET_IF_VALID(chroma_location, AVCHROMA_LOC_NB)
>>> +#undef SET_IF_VALID
>>> +
>>> +if (0 <= color_primaries && color_primaries <= AVCOL_PRI_SMPTE432
>>> + || color_primaries == AVCOL_PRI_JEDEC_P22)
>>> +par->color_primaries = color_primaries;
>>> +
> And how about the gap in the color_primaries values?
Same argument applies, I think?
>>> +return;
>>> +}
>>> +
>>> int ff_cbs_write_packet(CodedBitstreamContext *ctx,
>>> AVPacket *pkt,
>>> CodedBitstreamFragment *frag)
>>> diff --git a/libavcodec/cbs.h b/libavcodec/cbs.h
>>> index e1e6055ceb..1655f790cd 100644
>>> --- a/libavcodec/cbs.h
>>> +++ b/libavcodec/cbs.h
>>> @@ -304,6 +304,21 @@ int ff_cbs_write_extradata(CodedBitstreamContext *ctx,
>>> AVCodecParameters *par,
>>>
Re: [FFmpeg-devel] [PATCH 25/31] cbs: Add function to update video codec parameters
Mark Thompson:
> On 20/06/2019 00:45, Andreas Rheinhardt wrote:
>> If any of the *_metadata filter based upon cbs currently updates a video
>> codec parameter like color information, the AVCodecParameters are not
>> updated accordingly, so that e.g. muxers write header values based upon
>> outdated information that may precede and thereby nullify the new values
>> on the bitstream level.
>> This commit adds a function to also update the video codec parameters
>> so that the above situation can be fixed in a unified manner.
>>
>> Signed-off-by: Andreas Rheinhardt
>> ---
>> libavcodec/cbs.c | 35 +++
>> libavcodec/cbs.h | 15 +++
>> 2 files changed, 50 insertions(+)
>>
>> diff --git a/libavcodec/cbs.c b/libavcodec/cbs.c
>> index 47679eca1b..37b080b5ba 100644
>> --- a/libavcodec/cbs.c
>> +++ b/libavcodec/cbs.c
>> @@ -342,6 +342,41 @@ int ff_cbs_write_extradata(CodedBitstreamContext *ctx,
>> return 0;
>> }
>>
>> +void ff_cbs_update_video_parameters(CodedBitstreamContext *ctx,
>
> The context argument isn't used at all.
>
I know. I thought that's how you wanted the API to look like. After
all, ff_cbs_fragment_uninit always used a context argument without
needing it (cbs_unit_uninit doesn't use its context argument at all
and it is the only place where ff_cbs_fragment_uninit used it; I of
course kept this behaviour when replacing ff_cbs_fragment_uninit). Now
that it seems that this was unintentional, I will send a patch to
remove the argument.
>> +AVCodecParameters *par, int profile,
>> +int level, int width, int height,
>> +int field_order, int color_range,
>> +int color_primaries, int color_trc,
>> +int color_space, int chroma_location,
>> +int video_delay)
>> +{
>> +#define SET_IF_NONNEGATIVE(elem) \
>> +if (elem >= 0) \
>> +par->elem = elem;
>> +SET_IF_NONNEGATIVE(profile)
>> +SET_IF_NONNEGATIVE(level)
>> +SET_IF_NONNEGATIVE(width)
>> +SET_IF_NONNEGATIVE(height)
>> +SET_IF_NONNEGATIVE(field_order)
>> +SET_IF_NONNEGATIVE(video_delay)
>> +#undef SET_IF_NONNEGATIVE
>> +
>> +#define SET_IF_VALID(elem, upper_bound) \
>> +if (0 <= elem && elem < upper_bound) \
>> +par->elem = elem;
>> +SET_IF_VALID(color_range, AVCOL_RANGE_NB)
>> +SET_IF_VALID(color_trc, AVCOL_TRC_NB)
>> +SET_IF_VALID(color_space, AVCOL_SPC_NB)
>
> I think we generally want to admit future values for the 23001-8 / H.273
> fields (see range ..255 on all the metadata filters).
>
Do you want to omit checking entirely? Or should I still check against
the sentinel? I prefer the latter. I dislike setting an enum to
something else than an enum constant and moreover, if a future
standard would allow (say) color_range > 255 (or a bsf would simply
forget to check the value), but libavcodec does not, then it might be
that the compiler uses char as underlying type for the enum which of
course could not hold such a value.
>> +SET_IF_VALID(chroma_location, AVCHROMA_LOC_NB)
>> +#undef SET_IF_VALID
>> +
>> +if (0 <= color_primaries && color_primaries <= AVCOL_PRI_SMPTE432
>> + || color_primaries == AVCOL_PRI_JEDEC_P22)
>> +par->color_primaries = color_primaries;
>> +
And how about the gap in the color_primaries values?
>> +return;
>> +}
>> +
>> int ff_cbs_write_packet(CodedBitstreamContext *ctx,
>> AVPacket *pkt,
>> CodedBitstreamFragment *frag)
>> diff --git a/libavcodec/cbs.h b/libavcodec/cbs.h
>> index e1e6055ceb..1655f790cd 100644
>> --- a/libavcodec/cbs.h
>> +++ b/libavcodec/cbs.h
>> @@ -304,6 +304,21 @@ int ff_cbs_write_extradata(CodedBitstreamContext *ctx,
>> AVCodecParameters *par,
>> CodedBitstreamFragment *frag);
>>
>> +/**
>> + * Update the parameters of an AVCodecParameters structure
>> + *
>> + * If a parameter is negative, the corresponding member is not
>> + * modified. For the color/chroma parameters, only values that
>> + * are part of the relevant enumeration are written.
>> + */
>> +void ff_cbs_update_video_parameters(CodedBitstreamContext *ctx,
>> +AVCodecParameters *par, int profile,
>> +int level, int width, int height,
>> +int field_order, int color_range,
>> +int color_primaries, int color_trc,
>> +int color_space, int chroma_location,
>> +int video_delay);
>
> I find the calls with -1, -1, -1 pretty horrible, and all the extra pointer
> arguments being passed around in the metadata filters are not very nice
> either.
>
>
Re: [FFmpeg-devel] [PATCH 25/31] cbs: Add function to update video codec parameters
On 20/06/2019 00:45, Andreas Rheinhardt wrote:
> If any of the *_metadata filter based upon cbs currently updates a video
> codec parameter like color information, the AVCodecParameters are not
> updated accordingly, so that e.g. muxers write header values based upon
> outdated information that may precede and thereby nullify the new values
> on the bitstream level.
> This commit adds a function to also update the video codec parameters
> so that the above situation can be fixed in a unified manner.
>
> Signed-off-by: Andreas Rheinhardt
> ---
> libavcodec/cbs.c | 35 +++
> libavcodec/cbs.h | 15 +++
> 2 files changed, 50 insertions(+)
>
> diff --git a/libavcodec/cbs.c b/libavcodec/cbs.c
> index 47679eca1b..37b080b5ba 100644
> --- a/libavcodec/cbs.c
> +++ b/libavcodec/cbs.c
> @@ -342,6 +342,41 @@ int ff_cbs_write_extradata(CodedBitstreamContext *ctx,
> return 0;
> }
>
> +void ff_cbs_update_video_parameters(CodedBitstreamContext *ctx,
The context argument isn't used at all.
> +AVCodecParameters *par, int profile,
> +int level, int width, int height,
> +int field_order, int color_range,
> +int color_primaries, int color_trc,
> +int color_space, int chroma_location,
> +int video_delay)
> +{
> +#define SET_IF_NONNEGATIVE(elem) \
> +if (elem >= 0) \
> +par->elem = elem;
> +SET_IF_NONNEGATIVE(profile)
> +SET_IF_NONNEGATIVE(level)
> +SET_IF_NONNEGATIVE(width)
> +SET_IF_NONNEGATIVE(height)
> +SET_IF_NONNEGATIVE(field_order)
> +SET_IF_NONNEGATIVE(video_delay)
> +#undef SET_IF_NONNEGATIVE
> +
> +#define SET_IF_VALID(elem, upper_bound) \
> +if (0 <= elem && elem < upper_bound) \
> +par->elem = elem;
> +SET_IF_VALID(color_range, AVCOL_RANGE_NB)
> +SET_IF_VALID(color_trc, AVCOL_TRC_NB)
> +SET_IF_VALID(color_space, AVCOL_SPC_NB)
I think we generally want to admit future values for the 23001-8 / H.273 fields
(see range ..255 on all the metadata filters).
> +SET_IF_VALID(chroma_location, AVCHROMA_LOC_NB)
> +#undef SET_IF_VALID
> +
> +if (0 <= color_primaries && color_primaries <= AVCOL_PRI_SMPTE432
> + || color_primaries == AVCOL_PRI_JEDEC_P22)
> +par->color_primaries = color_primaries;
> +
> +return;
> +}
> +
> int ff_cbs_write_packet(CodedBitstreamContext *ctx,
> AVPacket *pkt,
> CodedBitstreamFragment *frag)
> diff --git a/libavcodec/cbs.h b/libavcodec/cbs.h
> index e1e6055ceb..1655f790cd 100644
> --- a/libavcodec/cbs.h
> +++ b/libavcodec/cbs.h
> @@ -304,6 +304,21 @@ int ff_cbs_write_extradata(CodedBitstreamContext *ctx,
> AVCodecParameters *par,
> CodedBitstreamFragment *frag);
>
> +/**
> + * Update the parameters of an AVCodecParameters structure
> + *
> + * If a parameter is negative, the corresponding member is not
> + * modified. For the color/chroma parameters, only values that
> + * are part of the relevant enumeration are written.
> + */
> +void ff_cbs_update_video_parameters(CodedBitstreamContext *ctx,
> +AVCodecParameters *par, int profile,
> +int level, int width, int height,
> +int field_order, int color_range,
> +int color_primaries, int color_trc,
> +int color_space, int chroma_location,
> +int video_delay);
I find the calls with -1, -1, -1 pretty horrible, and all the extra pointer
arguments being passed around in the metadata filters are not very nice either.
To avoid both of those, how about something like this:
typedef struct foo {
int profile;
int level;
int width;
...
} foo;
init_foo(foo *p)
{
set all fields to minus one (or some other placeholder value)
}
update_codecpar_with_foo(AVCodecPar *par, const foo *p)
{
as above
}
Then in each BSF:
struct BarMetadataContext {
...
foo parameters;
} BarMetadataContext;
update_stuff()
{
...
ctx->parameters.whatever = not-minus-one
}
init()
{
init_foo(>parameters);
update_stuff()
update_codecpar_with_foo(bsf->par_out, >foo);
}
What do you think?
Thanks,
- Mark
___
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 25/31] cbs: Add function to update video codec parameters
If any of the *_metadata filter based upon cbs currently updates a video
codec parameter like color information, the AVCodecParameters are not
updated accordingly, so that e.g. muxers write header values based upon
outdated information that may precede and thereby nullify the new values
on the bitstream level.
This commit adds a function to also update the video codec parameters
so that the above situation can be fixed in a unified manner.
Signed-off-by: Andreas Rheinhardt
---
libavcodec/cbs.c | 35 +++
libavcodec/cbs.h | 15 +++
2 files changed, 50 insertions(+)
diff --git a/libavcodec/cbs.c b/libavcodec/cbs.c
index 47679eca1b..37b080b5ba 100644
--- a/libavcodec/cbs.c
+++ b/libavcodec/cbs.c
@@ -342,6 +342,41 @@ int ff_cbs_write_extradata(CodedBitstreamContext *ctx,
return 0;
}
+void ff_cbs_update_video_parameters(CodedBitstreamContext *ctx,
+AVCodecParameters *par, int profile,
+int level, int width, int height,
+int field_order, int color_range,
+int color_primaries, int color_trc,
+int color_space, int chroma_location,
+int video_delay)
+{
+#define SET_IF_NONNEGATIVE(elem) \
+if (elem >= 0) \
+par->elem = elem;
+SET_IF_NONNEGATIVE(profile)
+SET_IF_NONNEGATIVE(level)
+SET_IF_NONNEGATIVE(width)
+SET_IF_NONNEGATIVE(height)
+SET_IF_NONNEGATIVE(field_order)
+SET_IF_NONNEGATIVE(video_delay)
+#undef SET_IF_NONNEGATIVE
+
+#define SET_IF_VALID(elem, upper_bound) \
+if (0 <= elem && elem < upper_bound) \
+par->elem = elem;
+SET_IF_VALID(color_range, AVCOL_RANGE_NB)
+SET_IF_VALID(color_trc, AVCOL_TRC_NB)
+SET_IF_VALID(color_space, AVCOL_SPC_NB)
+SET_IF_VALID(chroma_location, AVCHROMA_LOC_NB)
+#undef SET_IF_VALID
+
+if (0 <= color_primaries && color_primaries <= AVCOL_PRI_SMPTE432
+ || color_primaries == AVCOL_PRI_JEDEC_P22)
+par->color_primaries = color_primaries;
+
+return;
+}
+
int ff_cbs_write_packet(CodedBitstreamContext *ctx,
AVPacket *pkt,
CodedBitstreamFragment *frag)
diff --git a/libavcodec/cbs.h b/libavcodec/cbs.h
index e1e6055ceb..1655f790cd 100644
--- a/libavcodec/cbs.h
+++ b/libavcodec/cbs.h
@@ -304,6 +304,21 @@ int ff_cbs_write_extradata(CodedBitstreamContext *ctx,
AVCodecParameters *par,
CodedBitstreamFragment *frag);
+/**
+ * Update the parameters of an AVCodecParameters structure
+ *
+ * If a parameter is negative, the corresponding member is not
+ * modified. For the color/chroma parameters, only values that
+ * are part of the relevant enumeration are written.
+ */
+void ff_cbs_update_video_parameters(CodedBitstreamContext *ctx,
+AVCodecParameters *par, int profile,
+int level, int width, int height,
+int field_order, int color_range,
+int color_primaries, int color_trc,
+int color_space, int chroma_location,
+int video_delay);
+
/**
* Write the bitstream of a fragment to a packet.
*
--
2.21.0
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
