On 9/19/2021 1:14 AM, Soft Works wrote:
-----Original Message-----
From: ffmpeg-devel <ffmpeg-devel-boun...@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-boun...@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 <softwo...@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-requ...@ffmpeg.org with subject "unsubscribe".
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
