[FFmpeg-devel] [PATCH v3 1/2] avcodec/codec_par: Add codec properties field to AVCodecParameters

[Andreas Rheinhardt]

Soft Works:
> 
> 
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
>> Hendrik Leppkes
>> Sent: Wednesday, October 6, 2021 8:57 AM
>> To: FFmpeg development discussions and patches <ffmpeg-
>> devel at ffmpeg.org>
>> Subject: Re: [FFmpeg-devel] [PATCH v3 1/2] avcodec/codec_par: Add
>> codec properties field to AVCodecParameters
>>
>> On Wed, Oct 6, 2021 at 8:45 AM Soft Works <softworkz at hotmail.com>
>> wrote:
>>> diff --git a/libavcodec/codec_par.h b/libavcodec/codec_par.h
>>> index 10cf79dff1..42ed8deb13 100644
>>> --- a/libavcodec/codec_par.h
>>> +++ b/libavcodec/codec_par.h
>>> @@ -198,6 +198,10 @@ typedef struct AVCodecParameters {
>>>       * Audio only. Number of samples to skip after a
>> discontinuity.
>>>       */
>>>      int seek_preroll;
>>> +    /**
>>> +     * Codec properties of the stream that gets decoded
>>> +     */
>>> +    unsigned properties;
>>>  } AVCodecParameters;
>>>
>>
>> This field is severly underspecified/underdocumented. I realize you
>> just copied it, but if I'm looking at this without pre-existing
>> knowledge, then I have absolutely no idea what it might be for, or
>> what kind of values go in there. The old field had the defines of the
>> possible bits right there for context - this doesn't have this, so it
>> would definitely benefit from added documentation.
> 
> Hello Hendrik,
> 
> that's right, but what would you suggest? We can't duplicate the defines,
> though I could add a line in the help text like 
> 
>     /**
>      * Codec properties of the stream that gets decoded.
>      * Corresponds to @ref AVCodecContext.properties "properties".
>      */
>     unsigned properties;
> 
> Would that be OK?
> 

The defines could be moved to defs.h (while just at it, we could also
add properly prefixed alternatives and deprecate the FF_ ones).
The copying that you are adding in  avcodec_parameters_from_context is
problematic, as the relevant AVCodecContext field is marked as "set by
libavcodec" for decoding, which means that decoders expect it to be
blank initially. Imagine a scenario where the beginning (that gets read
in avformat_find_stream_info()) of an input stream has e.g. embedded cc
subtitles, but the part that lateron gets fed to the decoder does not.
If the decoder has been initialized from the AVStream's
AVCodecParameters, then said decoder would still indicate that there are
closed captions.* IIRC Anton already mentioned that this is actually a
frame/packet property, not a stream property.

- Andreas

*: It seems that the properties field is treated inconsistently:
LOSSLESS and CLOSED_CAPTIONS are only ever set, never unset, FILM_GRAIN
is also unset.