diff mbox series

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

Message ID MN2PR04MB5981EC96E707D8EF1EEEDD16BADE9@MN2PR04MB5981.namprd04.prod.outlook.com
State Superseded, archived
Headers show
Series [FFmpeg-devel,v2,1/2] avformat/utils: Add av_stream_get_codec_properties()
Related show

Checks

Context Check Description
andriy/configurex86 warning Failed to apply patch
andriy/configureppc warning Failed to apply patch

Commit Message

Soft Works Sept. 18, 2021, 10:10 p.m. UTC
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@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(-)

 stream|index=1|codec_name=ac3|profile=unknown|codec_type=audio|codec_tag_string=[4][0][0][0]|codec_tag=0x0004|sample_fmt=fltp|sample_rate=48000|channels=6|channel_layout=5.1(side)|bits_per_sample=0|id=0x34|r_frame_rate=0/0|avg_frame_rate=0/0|time_base=1/90000|start_pts=3912633305|start_time=43473.703389|duration_ts=14400|duration=0.160000|bit_rate=384000|max_bit_rate=N/A|bits_per_raw_sample=N/A|nb_frames=N/A|nb_read_frames=N/A|nb_read_packets=5|disposition:default=0|disposition:dub=0|disposition:original=0|disposition:comment=0|disposition:lyrics=0|disposition:karaoke=0|disposition:forced=0|disposition:hearing_impaired=0|disposition:visual_impaired=0|disposition:clean_effects=0|disposition:attached_pic=0|disposition:timed_thumbnails=0|disposition:captions=0|disposition:descriptions=0|disposition:metadata=0|disposition:dependent=0|disposition:still_image=0|tag:language=eng
 stream|index=2|codec_name=ac3|profile=unknown|codec_type=audio|codec_tag_string=[4][0][0][0]|codec_tag=0x0004|sample_fmt=fltp|sample_rate=48000|channels=2|channel_layout=stereo|bits_per_sample=0|id=0x35|r_frame_rate=0/0|avg_frame_rate=0/0|time_base=1/90000|start_pts=3912634060|start_time=43473.711778|duration_ts=14400|duration=0.160000|bit_rate=192000|max_bit_rate=N/A|bits_per_raw_sample=N/A|nb_frames=N/A|nb_read_frames=N/A|nb_read_packets=5|disposition:default=0|disposition:dub=0|disposition:original=0|disposition:comment=0|disposition:lyrics=0|disposition:karaoke=0|disposition:forced=0|disposition:hearing_impaired=0|disposition:visual_impaired=0|disposition:clean_effects=0|disposition:attached_pic=0|disposition:timed_thumbnails=0|disposition:captions=0|disposition:descriptions=0|disposition:metadata=0|disposition:dependent=0|disposition:still_image=0|tag:language=es

Comments

Soft Works Sept. 18, 2021, 10:37 p.m. UTC | #1
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Andreas
> Rheinhardt
> Sent: Sunday, 19 September 2021 00:16
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v2 2/2] ffprobe: Fix incorrect display of
> closed_captions property
> 
> Soft Works:
> > 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@hotmail.com>
> > ---
> > v2: Fix test result. It was undiscovered locally as make fate
> >     does not seem to properly rebuild ffprobe.
> >
> 
> This test and several others in demux.mak are indeed missing an ffprobe
> dependency. Will fix it. (A full fate-run should have nevertheless
> rebuilt ffprobe, but maybe it only rebuilt it after having run these tests?)

I have a clean setup on a Uubuntu VM where I do nothing but running
FATE.

I did 'git pull' and then ran FATE which passed.


I had recently seen a similar issue which still reproduces for me:

make distclean
./configure
make -j8 fate SAMPLES=...

It errors at some point, but it doesn't error when I either
make ffprobe before or when I do not use the "-j8" parameter.

softworkz
Soft Works Sept. 18, 2021, 10:44 p.m. UTC | #2
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Soft Works
> Sent: Sunday, 19 September 2021 00:38
> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH v2 2/2] ffprobe: Fix incorrect display of
> closed_captions property
> 
> 
> 
> > -----Original Message-----
> > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Andreas
> > Rheinhardt
> > Sent: Sunday, 19 September 2021 00:16
> > To: ffmpeg-devel@ffmpeg.org
> > Subject: Re: [FFmpeg-devel] [PATCH v2 2/2] ffprobe: Fix incorrect display
> of
> > closed_captions property
> >
> > Soft Works:
> > > 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@hotmail.com>
> > > ---
> > > v2: Fix test result. It was undiscovered locally as make fate
> > >     does not seem to properly rebuild ffprobe.
> > >
> >
> > This test and several others in demux.mak are indeed missing an ffprobe
> > dependency. Will fix it. (A full fate-run should have nevertheless
> > rebuilt ffprobe, but maybe it only rebuilt it after having run these
> tests?)
> 
> I have a clean setup on a Uubuntu VM where I do nothing but running
> FATE.
> 
> I did 'git pull' and then ran FATE which passed.
> 
> 
> I had recently seen a similar issue which still reproduces for me:
> 
> make distclean
> ./configure
> make -j8 fate SAMPLES=...
> 
> It errors at some point, but it doesn't error when I either
> make ffprobe before or when I do not use the "-j8" parameter.
> 

That essentially aligns with what you said:

> > rebuilt ffprobe, but maybe it only rebuilt it after having run these
> tests?)

ffprobe might get re-built at some point but when not building 
sequentially it might be built too late.

Maybe just some but not all depending tests are marked to depend
on ffprobe?

softworkz
James Almer Sept. 19, 2021, 3:05 a.m. UTC | #3
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@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.

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

I'll send a patch for this later.
Soft Works Sept. 19, 2021, 3:28 a.m. UTC | #4
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of James Almer
> Sent: Sunday, 19 September 2021 05:05
> To: ffmpeg-devel@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@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.

> 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?
- It takes additional processing time, which could have a huge impact
  depending on how the input is being processed or a rather small 
  impact. 
  But even when the impact would be small - it could matter a lot
  when you are probing a library of like 10k or 100k files.

Kind regards,
softworkz
James Almer Sept. 19, 2021, 3:59 a.m. UTC | #5
On 9/19/2021 12:28 AM, Soft Works wrote:
> 
> 
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of James Almer
>> Sent: Sunday, 19 September 2021 05:05
>> To: ffmpeg-devel@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@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.

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.

> - It takes additional processing time, which could have a huge impact
>    depending on how the input is being processed or a rather small
>    impact.
>    But even when the impact would be small - it could matter a lot
>    when you are probing a library of like 10k or 100k files.
> 
> Kind regards,
> softworkz
> 
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
Soft Works Sept. 19, 2021, 4:14 a.m. UTC | #6
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of James Almer
> Sent: Sunday, 19 September 2021 05:59
> To: ffmpeg-devel@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@ffmpeg.org> On Behalf Of James
> Almer
> >> Sent: Sunday, 19 September 2021 05:05
> >> To: ffmpeg-devel@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@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
Soft Works Sept. 19, 2021, 4:46 a.m. UTC | #7
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of James Almer
> Sent: Sunday, 19 September 2021 05:59
> To: ffmpeg-devel@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@ffmpeg.org> On Behalf Of James
> Almer
> >> Sent: Sunday, 19 September 2021 05:05
> >> To: ffmpeg-devel@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@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?).

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 :-)

Kind regards,
softworkz
James Almer Sept. 19, 2021, 1:04 p.m. UTC | #8
On 9/19/2021 1:46 AM, Soft Works wrote:
> 
> 
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of James Almer
>> Sent: Sunday, 19 September 2021 05:59
>> To: ffmpeg-devel@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@ffmpeg.org> On Behalf Of James
>> Almer
>>>> Sent: Sunday, 19 September 2021 05:05
>>>> To: ffmpeg-devel@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@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. And this flag is essentially a "The decoder found a CC 
at some point" event flag rather than a stream parameter.

The fact ffprobe looks at the decoder context to print stream values to 
begin with is questionable, and as mentioned, its output is not even 
guaranteed and depends on external factors, like other user-provided 
runtime options.

> 
> Kind regards,
> softworkz
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
James Almer Sept. 19, 2021, 1:11 p.m. UTC | #9
On 9/19/2021 1:14 AM, Soft Works wrote:
> 
> 
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of James Almer
>> Sent: Sunday, 19 September 2021 05:59
>> To: ffmpeg-devel@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@ffmpeg.org> On Behalf Of James
>> Almer
>>>> Sent: Sunday, 19 September 2021 05:05
>>>> To: ffmpeg-devel@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@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@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
Soft Works Sept. 19, 2021, 1:43 p.m. UTC | #10
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of James Almer
> Sent: Sunday, 19 September 2021 15:04
> To: ffmpeg-devel@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@ffmpeg.org> On Behalf Of James
> Almer
> >> Sent: Sunday, 19 September 2021 05:59
> >> To: ffmpeg-devel@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@ffmpeg.org> On Behalf Of James
> >> Almer
> >>>> Sent: Sunday, 19 September 2021 05:05
> >>>> To: ffmpeg-devel@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@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
Anton Khirnov Sept. 20, 2021, 11:07 a.m. UTC | #11
Quoting James Almer (2021-09-19 15:04:11)
> 
> 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. And this flag is essentially a "The decoder found a CC 
> at some point" event flag rather than a stream parameter.
> 
> The fact ffprobe looks at the decoder context to print stream values to 
> begin with is questionable, and as mentioned, its output is not even 
> guaranteed and depends on external factors, like other user-provided 
> runtime options.

I agree. ffprobe should stop printing that field as a part of the global
stream information, it simply cannot work reliably.

Print it as a part of the frame data instead.
Soft Works Sept. 20, 2021, 2:04 p.m. UTC | #12
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Anton
> Khirnov
> Sent: Monday, 20 September 2021 13:07
> To: James Almer <jamrial@gmail.com>; ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v2 2/2] ffprobe: Fix incorrect display of
> closed_captions property
> 
> Quoting James Almer (2021-09-19 15:04:11)
> >
> > 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. And this flag is essentially a "The decoder found a CC
> > at some point" event flag rather than a stream parameter.
> >
> > The fact ffprobe looks at the decoder context to print stream values to
> > begin with is questionable, and as mentioned, its output is not even
> > guaranteed and depends on external factors, like other user-provided
> > runtime options.
> 
> I agree. ffprobe should stop printing that field as a part of the global
> stream information, it simply cannot work reliably.

Well, but that's what the concept of "probing" is all about:

Take a selected amount of a subject (the "probe") and generate an 
aggregated summary result from that probe.

It's not only the small detail of closed_captions - many other essential
other information (like available streams and details) can (depending on 
the container) be subject to that procedure.
Without relying on those results, ffmpeg couldn't work like it does.

From practical experience, I can says that the closed_captions information
that ffmpeg/ffprobe gathers when executing avformat_find_stream_info()
is sufficiently reliable. 
That's exactly the information that we need (further downstream as a 
consumer of ffmpeg/ffprobe) - plus: that's the information that ffmpeg
has available internally. For being able to properly interoperate 
with ffmpeg, it's important that the information that we get from ffprobe
is exactly matching what ffmpeg has internally (given that the probesize
and analyzeduration used for ffprobe and ffmpeg are equal).

That’s why I think that there exists a legitimate interest for a 
consumer to be able to access this information, without needing to 
repeat and duplicate the probing procedure that has already been run.

Kind regards,
softworkz
Soft Works Sept. 20, 2021, 2:58 p.m. UTC | #13
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Anton
> Khirnov
> Sent: Monday, 20 September 2021 13:07
> To: James Almer <jamrial@gmail.com>; ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v2 2/2] ffprobe: Fix incorrect display of
> closed_captions property
> 
> Quoting James Almer (2021-09-19 15:04:11)
> >
> > 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. And this flag is essentially a "The decoder found a CC
> > at some point" event flag rather than a stream parameter.
> >
> > The fact ffprobe looks at the decoder context to print stream values to
> > begin with is questionable, and as mentioned, its output is not even
> > guaranteed and depends on external factors, like other user-provided
> > runtime options.
> 
> I agree. ffprobe should stop printing that field as a part of the global
> stream information, it simply cannot work reliably.
> 
> Print it as a part of the frame data instead.

There's also a very practical new use case associated to that subject:

My latest patchset regarding subtitle filtering includes a new 
filter named 'split_cc'. It has a video input and output and
a second output providing subtitle data, which can be processed
in many ways that haven't been available yet.

Users need an easy and direct indication to know whether CC data is 
available and it makes sense to use the filter.

Removing that information from the codec string would rather be
counter-productive in this regard.

softworkz
Soft Works Oct. 3, 2021, 8:54 p.m. UTC | #14
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Soft Works
> Sent: Monday, 20 September 2021 16:58
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>; James Almer <jamrial@gmail.com>
> Subject: Re: [FFmpeg-devel] [PATCH v2 2/2] ffprobe: Fix incorrect
> display of closed_captions property
> 
> 
> 
> > -----Original Message-----
> > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Anton
> > Khirnov
> > Sent: Monday, 20 September 2021 13:07
> > To: James Almer <jamrial@gmail.com>; ffmpeg-devel@ffmpeg.org
> > Subject: Re: [FFmpeg-devel] [PATCH v2 2/2] ffprobe: Fix incorrect
> display of
> > closed_captions property

Hi James, hi Anton,

it seems we've got a bit stuck on this issue, so I would like to 
remind that I'm in no way insisting on a certain way for getting
at the required information. This patch was just suggesting one
possible way, but what I wouldn't find acceptable is when you'd 
want to say that there's no acceptable way at all.

The requirements are simple and clear:

- to get the results of the avformat_find_stream_info() run
- without repeating or replicating this procedure in ffprobe
  and without performing additional decoding in ffprobe

I'm pretty sure that when any of you had these requirements
as part of your own work, you would figure out a way to do
it while adhering to them.
As such it would be nice when you would be able to make
a suggestion for resolving this. 

Also, let's not forget that this is not a private requirement
that I have. It's about ffprobe. And its output with regard
to closed captions is currently wrong.

Thanks and kind regards,
softworkz
diff mbox series

Patch

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);
             print_int("coded_width",  dec_ctx->coded_width);
             print_int("coded_height", dec_ctx->coded_height);
-            print_int("closed_captions", !!(dec_ctx->properties & FF_CODEC_PROPERTY_CLOSED_CAPTIONS));
-            print_int("film_grain", !!(dec_ctx->properties & FF_CODEC_PROPERTY_FILM_GRAIN));
+            print_int("closed_captions", !!(codec_properties & FF_CODEC_PROPERTY_CLOSED_CAPTIONS));
+            print_int("film_grain", !!(codec_properties & FF_CODEC_PROPERTY_FILM_GRAIN));
         }
         print_int("has_b_frames", par->video_delay);
         sar = av_guess_sample_aspect_ratio(fmt_ctx, stream, NULL);
diff --git a/tests/ref/fate/ts-demux b/tests/ref/fate/ts-demux
index 8e7a81da41..1d1382cf37 100644
--- a/tests/ref/fate/ts-demux
+++ b/tests/ref/fate/ts-demux
@@ -41,7 +41,7 @@  packet|codec_type=audio|stream_index=2|pts=3912642700|pts_time=43473.807778|dts=
 packet|codec_type=video|stream_index=0|pts=3912686363|pts_time=43474.292922|dts=3912686363|dts_time=43474.292922|duration=1501|duration_time=0.016678|size=4944|pos=506660|flags=__|data_hash=CRC32:54a86cbb
 packet|codec_type=audio|stream_index=1|pts=3912644825|pts_time=43473.831389|dts=3912644825|dts_time=43473.831389|duration=2880|duration_time=0.032000|size=906|pos=474888|flags=K_|data_hash=CRC32:0893d398
 packet|codec_type=audio|stream_index=2|pts=3912645580|pts_time=43473.839778|dts=3912645580|dts_time=43473.839778|duration=2880|duration_time=0.032000|size=354|pos=491808|flags=K_|data_hash=CRC32:f5963fa6
-stream|index=0|codec_name=mpeg2video|profile=4|codec_type=video|codec_tag_string=[2][0][0][0]|codec_tag=0x0002|width=1280|height=720|coded_width=0|coded_height=0|closed_captions=0|film_grain=0|has_b_frames=1|sample_aspect_ratio=1:1|display_aspect_ratio=16:9|pix_fmt=yuv420p|level=4|color_range=tv|color_space=unknown|color_transfer=unknown|color_primaries=unknown|chroma_location=left|field_order=progressive|refs=1|id=0x31|r_frame_rate=60000/1001|avg_frame_rate=60000/1001|time_base=1/90000|start_pts=3912669846|start_time=43474.109400|duration_ts=19519|duration=0.216878|bit_rate=15000000|max_bit_rate=N/A|bits_per_raw_sample=N/A|nb_frames=N/A|nb_read_frames=N/A|nb_read_packets=15|extradata_hash=CRC32:53134fa8|disposition:default=0|disposition:dub=0|disposition:original=0|disposition:comment=0|disposition:lyrics=0|disposition:karaoke=0|disposition:forced=0|disposition:hearing_impaired=0|disposition:visual_impaired=0|disposition:clean_effects=0|disposition:attached_pic=0|disposition:timed_
 thumbnails=0|disposition:captions=0|disposition:descriptions=0|disposition:metadata=0|disposition:dependent=0|disposition:still_image=0
+stream|index=0|codec_name=mpeg2video|profile=4|codec_type=video|codec_tag_string=[2][0][0][0]|codec_tag=0x0002|width=1280|height=720|coded_width=0|coded_height=0|closed_captions=1|film_grain=0|has_b_frames=1|sample_aspect_ratio=1:1|display_aspect_ratio=16:9|pix_fmt=yuv420p|level=4|color_range=tv|color_space=unknown|color_transfer=unknown|color_primaries=unknown|chroma_location=left|field_order=progressive|refs=1|id=0x31|r_frame_rate=60000/1001|avg_frame_rate=60000/1001|time_base=1/90000|start_pts=3912669846|start_time=43474.109400|duration_ts=19519|duration=0.216878|bit_rate=15000000|max_bit_rate=N/A|bits_per_raw_sample=N/A|nb_frames=N/A|nb_read_frames=N/A|nb_read_packets=15|extradata_hash=CRC32:53134fa8|disposition:default=0|disposition:dub=0|disposition:original=0|disposition:comment=0|disposition:lyrics=0|disposition:karaoke=0|disposition:forced=0|disposition:hearing_impaired=0|disposition:visual_impaired=0|disposition:clean_effects=0|disposition:attached_pic=0|disposition:timed_
 thumbnails=0|disposition:captions=0|disposition:descriptions=0|disposition:metadata=0|disposition:dependent=0|disposition:still_image=0
 side_data|side_data_type=CPB properties|max_bitrate=15000000|min_bitrate=0|avg_bitrate=0|buffer_size=9781248|vbv_delay=-1