[FFmpeg-devel] [PATCH v2 2/2] ffprobe: Fix incorrect display of closed_captions property

[James Almer]

On 9/19/2021 1:14 AM, Soft Works wrote:
> 
> 
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of James Almer
>> Sent: Sunday, 19 September 2021 05:59
>> To: ffmpeg-devel at ffmpeg.org
>> Subject: Re: [FFmpeg-devel] [PATCH v2 2/2] ffprobe: Fix incorrect display of
>> closed_captions property
>>
>> On 9/19/2021 12:28 AM, Soft Works wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of James
>> Almer
>>>> Sent: Sunday, 19 September 2021 05:05
>>>> To: ffmpeg-devel at ffmpeg.org
>>>> Subject: Re: [FFmpeg-devel] [PATCH v2 2/2] ffprobe: Fix incorrect display
>> of
>>>> closed_captions property
>>>>
>>>> On 9/18/2021 7:10 PM, Soft Works wrote:
>>>>> Repro Example:
>>>>> ffprobe -show_entries stream=closed_captions:disposition=:side_data=
>>>> "http://streams.videolan.org/streams/ts/CC/NewsStream-608-ac3.ts"
>>>>>
>>>>> While the codec string includes "Closed Captions",
>>>>> the stream data is showing: closed_captions=0
>>>>>
>>>>> The test ref was incorrect as the test media file
>>>>> actually does have cc.
>>>>>
>>>>> Signed-off-by: softworkz <softworkz at hotmail.com>
>>>>> ---
>>>>> v2: Fix test result. It was undiscovered locally as make fate
>>>>>        does not seem to properly rebuild ffprobe.
>>>>>
>>>>>     fftools/ffprobe.c       | 5 +++--
>>>>>     tests/ref/fate/ts-demux | 2 +-
>>>>>     2 files changed, 4 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/fftools/ffprobe.c b/fftools/ffprobe.c
>>>>> index 880f05a6c2..9425c1a2e3 100644
>>>>> --- a/fftools/ffprobe.c
>>>>> +++ b/fftools/ffprobe.c
>>>>> @@ -2678,10 +2678,11 @@ static int show_stream(WriterContext *w,
>>>> AVFormatContext *fmt_ctx, int stream_id
>>>>>             print_int("width",        par->width);
>>>>>             print_int("height",       par->height);
>>>>>             if (dec_ctx) {
>>>>> +            unsigned codec_properties =
>>>> av_stream_get_codec_properties(ist->st);
>>>>
>>>> Library users are meant to use their own decoder contexts if they want
>>>> this kind of information. AVStream->internal->avctx is internal to lavf.
>>>> Accessors like this should be avoided.
>>>
>>> If you look further up in util
>>>>
>>>> ffprobe is already decoding frames for the purpose of show_frames, and
>>>> in fact if you add -show_frames to the above command line example you
>>>> gave, the output of show_streams will report closed_captions=1.
>>>
>>> Yes, I know that, but if you don't - the output is wrong.
>>
>> Yeah, and fixing that would be ideal. The output of show_streams should
>> (if possible) not depend on other options also being passed.
>> Your patch will only do it for properties, and it requires new public
>> API, whereas my suggestion will do it for all other fields, like
>> coded_{width,height}, and requires changes contained entirely within
>> ffprobe with proper usage of existing API.
> 
> Please don't do this. This should not be driven by what might be "proper
> use of existing API".

What else could drive this? Adding new API to achieve what can be done 
with existing one by using it as intended is not ok. Library users are 
meant to open a file into an AVFormatContext, and if needed, copy its 
parameters into an AVCodecContext and fire up a decoder.

You want to add a new external symbol for the sole purpose of having a 
single value printed by a single tool that already has considerations to 
fetch that same value by other means.

> Instead of decoding additional frames, it would even be a better option to
> check for the "Closed Captions" string (even though being somewhat ridiculous)
> 
> 
> Generally, also from a larger perspective, this isolation doesn't appear
> to make sense to me.
> 
> The tool is named ffprobe - obviously intended for probing.
> It takes parameters like analyzeduration and probesize to control the
> probing, which is internally done by avformat_find_stream_info().
> But then, ffprobe should not be allowed to access (all) the results
> of that probing?
> 
> 
>> I'm not going to approve more accessors to specific fields in
>> AVStream->internal->avctx (Ideally, we'd remove the few that
>> unfortunately already exist) because they will just not stop being added
>> after that. There will always be another avctx field someone wants to
>> use, and eventually, we'll just end up with AVStream->codec all over again.
> 
> Then, why not add a an API method for copying the complete avctx data
> to a supplied one? This would prevent external API users from manipulating
> the internal avctx. Besides that, would there be any other reason why the
> data would need to be kept "secret"?

Because it's a decoder context, not a parameters struct. It has codec 
parameters but also decoding state. We used to have such a function, but 
then removed it because juggling with that state was a pain.
And if you're going to suggest to copy a subset of those values, that's 
precisely what AVCodecParameters was added for.

avformat_find_stream_info() is a helper to fill an AVFormatContext with 
info that's not available by simply reading container level info, by 
making it internally decode a few frames to fetch it. It is mainly meant 
for raw bitstreams or barebone containers, where for a remux scenario 
there's no other way to actually get that kind of info.
If you want codec level values beyond what's shared with container level 
values, then you should fire your own decoder.

> 
> Kind regards,
> softworkz
> 
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>