[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
>> Andreas Rheinhardt
>> Sent: Wednesday, October 6, 2021 11:30 AM
>> To: ffmpeg-devel at ffmpeg.org
>> Subject: Re: [FFmpeg-devel] [PATCH v3 1/2] avcodec/codec_par: Add
>> codec properties field to AVCodecParameters
>>
>> 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. 
> 
> Why do you think that decoders would expect it to be blank initially?
> 
> What about the other fields then, like format, profile, level, width,
> height, etc. - these are copied in the same way.
> Are you saying that decoders expect all other fields to be filled but 
> the properties field to be zero?
> 

Of course not. E.g. the documentation of the dimension explicitly says
so: "May be set by the user before opening the decoder if known e.g.
from the container. Some decoders will require the dimensions to be set
by the caller. During decoding, the decoder may overwrite those values
as required while parsing the data."
The semantics of profile and level are closer, yet the difference is
that if the decoder sets these fields, it overwrites any garbage it may
have had; this is not so with a bitfield like properties.

> Also, the docs say that it's SET by decoders, not READ by decoders,
> which means that no decoder could rely on this field. There's also
> no undefined-state defined (like -1), which means that a decoder 
> couldn't know whether a 0-value indicates a valid value or a not-yet
> initialized value.
> 

You completely misunderstood what my concern was about: In the scenario
described above an API user querying the properties field would get the
information that this stream contains CC data; even when it doesn't.

And your other claims are also wrong: A decoder may read anything from
an AVCodecContext or from the AVPackets and AVFrames with the exception
of data that is opaque to it. And if a field is described as
out-of-touch by the user, then the codec may rely on it not having
arbitrary values, but only the values it has set itself (or the default
value). Notice that if the user initializes the AVCodecContext via
avcodec_parameters_from_context(), then it still counts as user-set even
though said function lives in libavcodec, too.

>> 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.
> 
> That statement was not thought through. Of course it IS a frame 
> property as well, but there's no difference to other properties like
> width, height or aspect ratio:
> 
> - as a frame property, it is indicating whether a frame carries 
>   closed caption data
> - as a stream property, it indicates the summary result from the 
>   probing process
> 
> Just the same like some other fields in this struct.
>