[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 15:04
> 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 1:46 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.
> >>
> >>>
> >>>> So the correct approach here is to make ffprobe decode a few frames when
> >>>> you request show_streams, even if you don't request show_frames.
> >>>
> >>> This is something that I would want to avoid. It has already happened
> >>> and the result is available already. The "Closed Captions" part in
> >>> the video codec string is coming from that result.
> >>>
> >>> It doesn't make sense IMO, to perform another decoding run to replicate
> >>> that result, because:
> >>>
> >>> - It can't be taken for granted that this will lead to the same result
> >>>     that already exists
> >>>     How many frames do you think would be the right amount to process?
> >>>     What if the file start with a bunch of audio frames?
> >>
> >> You decode until you get the info you need, which is what
> >> avformat_find_stream_info() does to fill its own internal
> >> AVCodecContext. The simplest way would be to ensure one frame per stream
> >> is decoded.
> >
> > Sometimes, you could process hours of data without
> > seeing a frame for certain streams, so you would need a timeout
> > and maybe a limit for the amount of decoded data (maybe
> > naming them analyyzeduration and probesize?).
> 
> Yes, something like that could be forced (ReadInterval in ffprobe).
> 
> >
> > What you're essentially suggesting is more or less a duplication
> > of avformat_find_stream_info() and to run it right after
> > avformat_find_stream_info() has just been run.
> >
> > I'm sure we can find a better solution :-)
> 
> We could always not print codec level information, like the presence of
> closed captions in a bitstream, when container level information is
> requested, as is the case of show_streams.
> It is in fact a per-frame property. One could even not show up until
> halfways into the video and then not even avformat_find_stream_info()
> would reflect it. 

Sure, everything agreed up to this point.

But a user is running ffprobe to get results and he can control the
way how those results are being collected by using parameters like
probesize and analyzeduration (and others, for example about stream 
selection, handling of unknown streams, etc).

That procedure - controlled by the user's parameters is internally
carried out by avformat_find_stream_info(). 
And exactly those are the results that ffprobe needs to return: 

The results gathered during the probing - which is what
ffprobe is all about.


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

Yes, but AVCodecParameters doesn't include that information.

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

We're dealing a lot with TV streams (MPEGTS).
Repeating the decoding could add a lot of time for processing, specifically
when these are online live streams.

> If you want codec level values beyond what's shared with container level
> values, then you should fire your own decoder.

I don't find that acceptable, given that the information already exists 
and the procedure that is intended to collect it has already been run.

An API never has an intrinsic purpose and it doesn't make sense to me
to present this as a hindering reason.
An API needs to expose the functionality that a library provides in the
best possible and most efficient way. Surely it needs to follow a certain 
design and strategy. That's OK, even when it might make its use sometimes
more inconvenient as it could be.
But an API that is requiring a consumer to duplicate its functionality
not only in code but also in processing time is wrong from my point of
view.

There are surely multiple ways to solve this, but it can't be that it
can't be exposed for the API's sake.

I'm wondering a bit why that would even need a debate (I mean the 'if',
not the 'how').
Are you essentially saying that even though ffprobe gets the information 
as string (in the video codec description), the API cannot expose it
(in whatever way) because it would be bad for the API?


Kind regards,
softworkz