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

[Soft Works]

> -----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".
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"?

Kind regards,
softworkz