Re: [FFmpeg-devel] [PATCH V3 1/1] ensure closed caption info which is visible in default stream dump is also available in results when -show_streams is used

2020-04-25 Thread Marton Balint 



On Sat, 25 Apr 2020, vectronic wrote:





On 25 Apr 2020, at 09:08, Marton Balint  wrote:



On Fri, 24 Apr 2020, vectronic wrote:


Signed-off-by: vectronic 
---
doc/ffprobe.xsd   | 1 +
fftools/ffprobe.c | 2 ++
tests/ref/fate/concat-demuxer-extended-lavf-mxf   | 2 +-
tests/ref/fate/concat-demuxer-extended-lavf-mxf_d10   | 2 +-
tests/ref/fate/concat-demuxer-simple1-lavf-mxf| 2 +-
tests/ref/fate/concat-demuxer-simple1-lavf-mxf_d10| 2 +-
tests/ref/fate/concat-demuxer-simple2-lavf-ts | 2 +-
tests/ref/fate/ffprobe_compact| 4 ++--
tests/ref/fate/ffprobe_csv| 4 ++--
tests/ref/fate/ffprobe_default| 2 ++
tests/ref/fate/ffprobe_flat   | 2 ++
tests/ref/fate/ffprobe_ini| 2 ++
tests/ref/fate/ffprobe_json   | 2 ++
tests/ref/fate/ffprobe_xml| 4 ++--
tests/ref/fate/hapqa-extract-nosnappy-to-hapalphaonly-mov | 1 +
tests/ref/fate/hapqa-extract-nosnappy-to-hapq-mov | 1 +
tests/ref/fate/mov-zombie | 2 +-
tests/ref/fate/mxf-probe-d10  | 1 +
tests/ref/fate/mxf-probe-dnxhd| 1 +
tests/ref/fate/mxf-probe-dv25 | 1 +
20 files changed, 28 insertions(+), 12 deletions(-)

diff --git a/doc/ffprobe.xsd b/doc/ffprobe.xsd
index 97dc67def6..f88045232f 100644
--- a/doc/ffprobe.xsd
+++ b/doc/ffprobe.xsd
@@ -227,6 +227,7 @@
 
 
 
+  


xsd:boolean seems more appropriate, no?


I was copying off has_b_frames above which I believed was also a 0/1. The 
output in the stream info is 0/1, not true/false.


xsd:boolean can be 0/1 according ot this:
https://www.w3schools.com/xml/schema_dtypes_misc.asp
And I think boolean is more appropriate.




 
 
 
diff --git a/fftools/ffprobe.c b/fftools/ffprobe.c
index 840fcb71e2..f0916cbd70 100644
--- a/fftools/ffprobe.c
+++ b/fftools/ffprobe.c
@@ -2550,6 +2550,7 @@ static int show_stream(WriterContext *w, AVFormatContext 
*fmt_ctx, int stream_id
   }
#endif
   print_int("has_b_frames", par->video_delay);
+print_int("closed_captions", !!(dec_ctx->properties & 
FF_CODEC_PROPERTY_CLOSED_CAPTIONS));


Not strictly related to this patch, but maybe these FF_CODEC_PROPERTY_* 
constants should be promoted to AV_*?


I’m not sure what is meant by this, but happy to do so if you can elaborate a 
bit more…?


We typically use AV_ prefix for part-of-the-API constants and FF_ prefix 
for internal or not-part-of-the-API constants. Admittedly it is not very 
consistent, but I think that is the idea. Anyway, this can be changed 
later and in a separate patch.






   sar = av_guess_sample_aspect_ratio(fmt_ctx, stream, NULL);
   if (sar.num) {
   print_q("sample_aspect_ratio", sar, ':');
@@ -2950,6 +2951,7 @@ static int open_input_file(InputFile *ifile, const char 
*filename,

   ist->dec_ctx->pkt_timebase = stream->time_base;
   ist->dec_ctx->framerate = stream->avg_frame_rate;
+ist->dec_ctx->properties = stream->codec->properties;


This is using the depreacted stream->codec, so I think this should go under the 
#IF below.


#if FF_API_LAVF_AVCTX
   ist->dec_ctx->coded_width = stream->codec->coded_width;
   ist->dec_ctx->coded_height = stream->codec->coded_height;


OK, if this is the case, is there anything to worry to worry about if 
FF_API_LAVF_AVCTX is not defined? Should I be outputting closed_captions value 
in this case? Or am I thinking too much?


You are right, printing it should probably go to the block which prints 
the similar coded_height/coded_width values.


Regards,
Marton
___
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".

Re: [FFmpeg-devel] [PATCH V3 1/1] ensure closed caption info which is visible in default stream dump is also available in results when -show_streams is used

2020-04-25 Thread vectronic 


> On 25 Apr 2020, at 09:08, Marton Balint  wrote:
> 
> 
> 
> On Fri, 24 Apr 2020, vectronic wrote:
> 
>> Signed-off-by: vectronic 
>> ---
>> doc/ffprobe.xsd   | 1 +
>> fftools/ffprobe.c | 2 ++
>> tests/ref/fate/concat-demuxer-extended-lavf-mxf   | 2 +-
>> tests/ref/fate/concat-demuxer-extended-lavf-mxf_d10   | 2 +-
>> tests/ref/fate/concat-demuxer-simple1-lavf-mxf| 2 +-
>> tests/ref/fate/concat-demuxer-simple1-lavf-mxf_d10| 2 +-
>> tests/ref/fate/concat-demuxer-simple2-lavf-ts | 2 +-
>> tests/ref/fate/ffprobe_compact| 4 ++--
>> tests/ref/fate/ffprobe_csv| 4 ++--
>> tests/ref/fate/ffprobe_default| 2 ++
>> tests/ref/fate/ffprobe_flat   | 2 ++
>> tests/ref/fate/ffprobe_ini| 2 ++
>> tests/ref/fate/ffprobe_json   | 2 ++
>> tests/ref/fate/ffprobe_xml| 4 ++--
>> tests/ref/fate/hapqa-extract-nosnappy-to-hapalphaonly-mov | 1 +
>> tests/ref/fate/hapqa-extract-nosnappy-to-hapq-mov | 1 +
>> tests/ref/fate/mov-zombie | 2 +-
>> tests/ref/fate/mxf-probe-d10  | 1 +
>> tests/ref/fate/mxf-probe-dnxhd| 1 +
>> tests/ref/fate/mxf-probe-dv25 | 1 +
>> 20 files changed, 28 insertions(+), 12 deletions(-)
>> 
>> diff --git a/doc/ffprobe.xsd b/doc/ffprobe.xsd
>> index 97dc67def6..f88045232f 100644
>> --- a/doc/ffprobe.xsd
>> +++ b/doc/ffprobe.xsd
>> @@ -227,6 +227,7 @@
>>  
>>  
>>  
>> +  
> 
> xsd:boolean seems more appropriate, no?

I was copying off has_b_frames above which I believed was also a 0/1. The 
output in the stream info is 0/1, not true/false.

>>  
>>  
>>  
>> diff --git a/fftools/ffprobe.c b/fftools/ffprobe.c
>> index 840fcb71e2..f0916cbd70 100644
>> --- a/fftools/ffprobe.c
>> +++ b/fftools/ffprobe.c
>> @@ -2550,6 +2550,7 @@ static int show_stream(WriterContext *w, 
>> AVFormatContext *fmt_ctx, int stream_id
>>}
>> #endif
>>print_int("has_b_frames", par->video_delay);
>> +print_int("closed_captions", !!(dec_ctx->properties & 
>> FF_CODEC_PROPERTY_CLOSED_CAPTIONS));
> 
> Not strictly related to this patch, but maybe these FF_CODEC_PROPERTY_* 
> constants should be promoted to AV_*?

I’m not sure what is meant by this, but happy to do so if you can elaborate a 
bit more…?


>>sar = av_guess_sample_aspect_ratio(fmt_ctx, stream, NULL);
>>if (sar.num) {
>>print_q("sample_aspect_ratio", sar, ':');
>> @@ -2950,6 +2951,7 @@ static int open_input_file(InputFile *ifile, const 
>> char *filename,
>> 
>>ist->dec_ctx->pkt_timebase = stream->time_base;
>>ist->dec_ctx->framerate = stream->avg_frame_rate;
>> +ist->dec_ctx->properties = stream->codec->properties;
> 
> This is using the depreacted stream->codec, so I think this should go under 
> the #IF below.
> 
>> #if FF_API_LAVF_AVCTX
>>ist->dec_ctx->coded_width = stream->codec->coded_width;
>>ist->dec_ctx->coded_height = stream->codec->coded_height;

OK, if this is the case, is there anything to worry to worry about if 
FF_API_LAVF_AVCTX is not defined? Should I be outputting closed_captions value 
in this case? Or am I thinking too much?


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

Re: [FFmpeg-devel] [PATCH V3 1/1] ensure closed caption info which is visible in default stream dump is also available in results when -show_streams is used

2020-04-25 Thread Marton Balint 



On Fri, 24 Apr 2020, vectronic wrote:


Signed-off-by: vectronic 
---
doc/ffprobe.xsd   | 1 +
fftools/ffprobe.c | 2 ++
tests/ref/fate/concat-demuxer-extended-lavf-mxf   | 2 +-
tests/ref/fate/concat-demuxer-extended-lavf-mxf_d10   | 2 +-
tests/ref/fate/concat-demuxer-simple1-lavf-mxf| 2 +-
tests/ref/fate/concat-demuxer-simple1-lavf-mxf_d10| 2 +-
tests/ref/fate/concat-demuxer-simple2-lavf-ts | 2 +-
tests/ref/fate/ffprobe_compact| 4 ++--
tests/ref/fate/ffprobe_csv| 4 ++--
tests/ref/fate/ffprobe_default| 2 ++
tests/ref/fate/ffprobe_flat   | 2 ++
tests/ref/fate/ffprobe_ini| 2 ++
tests/ref/fate/ffprobe_json   | 2 ++
tests/ref/fate/ffprobe_xml| 4 ++--
tests/ref/fate/hapqa-extract-nosnappy-to-hapalphaonly-mov | 1 +
tests/ref/fate/hapqa-extract-nosnappy-to-hapq-mov | 1 +
tests/ref/fate/mov-zombie | 2 +-
tests/ref/fate/mxf-probe-d10  | 1 +
tests/ref/fate/mxf-probe-dnxhd| 1 +
tests/ref/fate/mxf-probe-dv25 | 1 +
20 files changed, 28 insertions(+), 12 deletions(-)

diff --git a/doc/ffprobe.xsd b/doc/ffprobe.xsd
index 97dc67def6..f88045232f 100644
--- a/doc/ffprobe.xsd
+++ b/doc/ffprobe.xsd
@@ -227,6 +227,7 @@
  
  
  
+  


xsd:boolean seems more appropriate, no?


  
  
  
diff --git a/fftools/ffprobe.c b/fftools/ffprobe.c
index 840fcb71e2..f0916cbd70 100644
--- a/fftools/ffprobe.c
+++ b/fftools/ffprobe.c
@@ -2550,6 +2550,7 @@ static int show_stream(WriterContext *w, AVFormatContext 
*fmt_ctx, int stream_id
}
#endif
print_int("has_b_frames", par->video_delay);
+print_int("closed_captions", !!(dec_ctx->properties & 
FF_CODEC_PROPERTY_CLOSED_CAPTIONS));


Not strictly related to this patch, but maybe these FF_CODEC_PROPERTY_* 
constants should be promoted to AV_*?



sar = av_guess_sample_aspect_ratio(fmt_ctx, stream, NULL);
if (sar.num) {
print_q("sample_aspect_ratio", sar, ':');
@@ -2950,6 +2951,7 @@ static int open_input_file(InputFile *ifile, const char 
*filename,

ist->dec_ctx->pkt_timebase = stream->time_base;
ist->dec_ctx->framerate = stream->avg_frame_rate;
+ist->dec_ctx->properties = stream->codec->properties;


This is using the depreacted stream->codec, so I think this should go 
under the #IF below.



#if FF_API_LAVF_AVCTX
ist->dec_ctx->coded_width = stream->codec->coded_width;
ist->dec_ctx->coded_height = stream->codec->coded_height;


Regards,
Marton
___
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".