Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
 help / color / mirror / Atom feed
* [FFmpeg-devel] [PATCH 1/3] lavc/codec.h: add AV_CODEC_CAP_SINGLE_SUB_RECT
@ 2023-02-21  0:25 rcombs
  2023-02-21  0:25 ` [FFmpeg-devel] [PATCH 2/3] lavc/assenc: set AV_CODEC_CAP_SINGLE_SUB_RECT rcombs
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: rcombs @ 2023-02-21  0:25 UTC (permalink / raw)
  To: ffmpeg-devel

---
 doc/APIchanges       | 3 +++
 libavcodec/codec.h   | 5 +++++
 libavcodec/version.h | 2 +-
 3 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/doc/APIchanges b/doc/APIchanges
index bc52a07964..56f33aa25b 100644
--- a/doc/APIchanges
+++ b/doc/APIchanges
@@ -14,6 +14,9 @@ libavutil:     2021-04-27
 
 API changes, most recent first:
 
+2023-02-20 - xxxxxxxxxx - lavc 59.60.100 - codec.h
+  Add AV_CODEC_CAP_SINGLE_SUB_RECT.
+
 2023-01-29 - xxxxxxxxxx - lavc 59.59.100 - avcodec.h
   Add AV_CODEC_FLAG_COPY_OPAQUE and AV_CODEC_FLAG_FRAME_DURATION.
 
diff --git a/libavcodec/codec.h b/libavcodec/codec.h
index 77a1a3f5a2..c0df33ef3c 100644
--- a/libavcodec/codec.h
+++ b/libavcodec/codec.h
@@ -190,6 +190,11 @@
  */
 #define AV_CODEC_CAP_ENCODER_RECON_FRAME (1 << 22)
 
+/**
+ * This encoder requires a single rectangle per AVSubtitle.
+ */
+#define AV_CODEC_CAP_SINGLE_SUB_RECT (1 << 23)
+
 /**
  * AVProfile.
  */
diff --git a/libavcodec/version.h b/libavcodec/version.h
index 752adc81f8..2ed4ef5547 100644
--- a/libavcodec/version.h
+++ b/libavcodec/version.h
@@ -29,7 +29,7 @@
 
 #include "version_major.h"
 
-#define LIBAVCODEC_VERSION_MINOR  59
+#define LIBAVCODEC_VERSION_MINOR  60
 #define LIBAVCODEC_VERSION_MICRO 100
 
 #define LIBAVCODEC_VERSION_INT  AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
-- 
2.39.1

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [FFmpeg-devel] [PATCH 2/3] lavc/assenc: set AV_CODEC_CAP_SINGLE_SUB_RECT
  2023-02-21  0:25 [FFmpeg-devel] [PATCH 1/3] lavc/codec.h: add AV_CODEC_CAP_SINGLE_SUB_RECT rcombs
@ 2023-02-21  0:25 ` rcombs
  2023-02-21  7:48   ` Nicolas George
  2023-02-21  0:25 ` [FFmpeg-devel] [PATCH 3/3] ffmpeg: respect AV_CODEC_CAP_SINGLE_SUB_RECT rcombs
  2023-02-21  7:42 ` [FFmpeg-devel] [PATCH 1/3] lavc/codec.h: add AV_CODEC_CAP_SINGLE_SUB_RECT Nicolas George
  2 siblings, 1 reply; 11+ messages in thread
From: rcombs @ 2023-02-21  0:25 UTC (permalink / raw)
  To: ffmpeg-devel

This already gave garbled output when multiple rects were present,
so this is simply documenting an existing requirement.
---
 libavcodec/assenc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/libavcodec/assenc.c b/libavcodec/assenc.c
index db6fd25dd7..1c49a6685b 100644
--- a/libavcodec/assenc.c
+++ b/libavcodec/assenc.c
@@ -74,6 +74,7 @@ const FFCodec ff_ssa_encoder = {
     CODEC_LONG_NAME("ASS (Advanced SubStation Alpha) subtitle"),
     .p.type       = AVMEDIA_TYPE_SUBTITLE,
     .p.id         = AV_CODEC_ID_ASS,
+    .p.capabilities = AV_CODEC_CAP_SINGLE_SUB_RECT,
     .init         = ass_encode_init,
     FF_CODEC_ENCODE_SUB_CB(ass_encode_frame),
 };
@@ -85,6 +86,7 @@ const FFCodec ff_ass_encoder = {
     CODEC_LONG_NAME("ASS (Advanced SubStation Alpha) subtitle"),
     .p.type       = AVMEDIA_TYPE_SUBTITLE,
     .p.id         = AV_CODEC_ID_ASS,
+    .p.capabilities = AV_CODEC_CAP_SINGLE_SUB_RECT,
     .init         = ass_encode_init,
     FF_CODEC_ENCODE_SUB_CB(ass_encode_frame),
 };
-- 
2.39.1

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [FFmpeg-devel] [PATCH 3/3] ffmpeg: respect AV_CODEC_CAP_SINGLE_SUB_RECT
  2023-02-21  0:25 [FFmpeg-devel] [PATCH 1/3] lavc/codec.h: add AV_CODEC_CAP_SINGLE_SUB_RECT rcombs
  2023-02-21  0:25 ` [FFmpeg-devel] [PATCH 2/3] lavc/assenc: set AV_CODEC_CAP_SINGLE_SUB_RECT rcombs
@ 2023-02-21  0:25 ` rcombs
  2023-02-21  7:45   ` Nicolas George
  2023-03-01 15:19   ` TADANO Tokumei
  2023-02-21  7:42 ` [FFmpeg-devel] [PATCH 1/3] lavc/codec.h: add AV_CODEC_CAP_SINGLE_SUB_RECT Nicolas George
  2 siblings, 2 replies; 11+ messages in thread
From: rcombs @ 2023-02-21  0:25 UTC (permalink / raw)
  To: ffmpeg-devel

Fixes ASS output when multiple rects are present.
---
 fftools/ffmpeg.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
index 9884e0c6c6..23eac52438 100644
--- a/fftools/ffmpeg.c
+++ b/fftools/ffmpeg.c
@@ -1021,6 +1021,7 @@ static void do_subtitle_out(OutputFile *of,
     AVCodecContext *enc;
     AVPacket *pkt = ost->pkt;
     int64_t pts;
+    int single_rect;
 
     if (sub->pts == AV_NOPTS_VALUE) {
         av_log(ost, AV_LOG_ERROR, "Subtitle packets must have a pts\n");
@@ -1031,11 +1032,15 @@ static void do_subtitle_out(OutputFile *of,
 
     enc = ost->enc_ctx;
 
+    single_rect = !!(enc->codec->capabilities & AV_CODEC_CAP_SINGLE_SUB_RECT);
+
     /* Note: DVB subtitle need one packet to draw them and one other
        packet to clear them */
     /* XXX: signal it in the codec context ? */
     if (enc->codec_id == AV_CODEC_ID_DVB_SUBTITLE)
         nb = 2;
+    else if (single_rect)
+        nb = FFMAX(sub->num_rects, 1);
     else
         nb = 1;
 
@@ -1044,7 +1049,7 @@ static void do_subtitle_out(OutputFile *of,
     if (output_files[ost->file_index]->start_time != AV_NOPTS_VALUE)
         pts -= output_files[ost->file_index]->start_time;
     for (i = 0; i < nb; i++) {
-        unsigned save_num_rects = sub->num_rects;
+        AVSubtitle local_sub = *sub;
 
         if (!check_recording_time(ost, pts, AV_TIME_BASE_Q))
             return;
@@ -1053,19 +1058,22 @@ static void do_subtitle_out(OutputFile *of,
         if (ret < 0)
             report_and_exit(AVERROR(ENOMEM));
 
-        sub->pts = pts;
+        local_sub.pts = pts;
         // start_display_time is required to be 0
-        sub->pts               += av_rescale_q(sub->start_display_time, (AVRational){ 1, 1000 }, AV_TIME_BASE_Q);
-        sub->end_display_time  -= sub->start_display_time;
-        sub->start_display_time = 0;
-        if (i == 1)
-            sub->num_rects = 0;
+        local_sub.pts               += av_rescale_q(sub->start_display_time, (AVRational){ 1, 1000 }, AV_TIME_BASE_Q);
+        local_sub.end_display_time  -= sub->start_display_time;
+        local_sub.start_display_time = 0;
+
+        if (enc->codec_id == AV_CODEC_ID_DVB_SUBTITLE && i == 1)
+            local_sub.num_rects = 0;
+        else if (single_rect && sub->num_rects > 0) {
+            local_sub.num_rects = 1;
+            local_sub.rects += i;
+        }
 
         ost->frames_encoded++;
 
-        subtitle_out_size = avcodec_encode_subtitle(enc, pkt->data, pkt->size, sub);
-        if (i == 1)
-            sub->num_rects = save_num_rects;
+        subtitle_out_size = avcodec_encode_subtitle(enc, pkt->data, pkt->size, &local_sub);
         if (subtitle_out_size < 0) {
             av_log(ost, AV_LOG_FATAL, "Subtitle encoding failed\n");
             exit_program(1);
-- 
2.39.1

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [FFmpeg-devel] [PATCH 1/3] lavc/codec.h: add AV_CODEC_CAP_SINGLE_SUB_RECT
  2023-02-21  0:25 [FFmpeg-devel] [PATCH 1/3] lavc/codec.h: add AV_CODEC_CAP_SINGLE_SUB_RECT rcombs
  2023-02-21  0:25 ` [FFmpeg-devel] [PATCH 2/3] lavc/assenc: set AV_CODEC_CAP_SINGLE_SUB_RECT rcombs
  2023-02-21  0:25 ` [FFmpeg-devel] [PATCH 3/3] ffmpeg: respect AV_CODEC_CAP_SINGLE_SUB_RECT rcombs
@ 2023-02-21  7:42 ` Nicolas George
  2023-02-21  8:07   ` Ridley Combs
  2 siblings, 1 reply; 11+ messages in thread
From: Nicolas George @ 2023-02-21  7:42 UTC (permalink / raw)
  To: FFmpeg development discussions and patches

rcombs (12023-02-20):
> ---
>  doc/APIchanges       | 3 +++
>  libavcodec/codec.h   | 5 +++++
>  libavcodec/version.h | 2 +-
>  3 files changed, 9 insertions(+), 1 deletion(-)

And... No change to the framework to make use of this flag? Like, return
AVERROR(EINVAL) if the flag is present but several rectangles are given?

Regards,

-- 
  Nicolas George
_______________________________________________
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".

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [FFmpeg-devel] [PATCH 3/3] ffmpeg: respect AV_CODEC_CAP_SINGLE_SUB_RECT
  2023-02-21  0:25 ` [FFmpeg-devel] [PATCH 3/3] ffmpeg: respect AV_CODEC_CAP_SINGLE_SUB_RECT rcombs
@ 2023-02-21  7:45   ` Nicolas George
  2023-02-21  8:09     ` Ridley Combs
  2023-03-01 15:19   ` TADANO Tokumei
  1 sibling, 1 reply; 11+ messages in thread
From: Nicolas George @ 2023-02-21  7:45 UTC (permalink / raw)
  To: FFmpeg development discussions and patches

rcombs (12023-02-20):
> Fixes ASS output when multiple rects are present.
> ---
>  fftools/ffmpeg.c | 28 ++++++++++++++++++----------
>  1 file changed, 18 insertions(+), 10 deletions(-)

Does this not belong to the framework?

> 
> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> index 9884e0c6c6..23eac52438 100644
> --- a/fftools/ffmpeg.c
> +++ b/fftools/ffmpeg.c
> @@ -1021,6 +1021,7 @@ static void do_subtitle_out(OutputFile *of,
>      AVCodecContext *enc;
>      AVPacket *pkt = ost->pkt;
>      int64_t pts;
> +    int single_rect;
>  
>      if (sub->pts == AV_NOPTS_VALUE) {
>          av_log(ost, AV_LOG_ERROR, "Subtitle packets must have a pts\n");
> @@ -1031,11 +1032,15 @@ static void do_subtitle_out(OutputFile *of,
>  
>      enc = ost->enc_ctx;
>  
> +    single_rect = !!(enc->codec->capabilities & AV_CODEC_CAP_SINGLE_SUB_RECT);
> +
>      /* Note: DVB subtitle need one packet to draw them and one other
>         packet to clear them */
>      /* XXX: signal it in the codec context ? */
>      if (enc->codec_id == AV_CODEC_ID_DVB_SUBTITLE)
>          nb = 2;
> +    else if (single_rect)
> +        nb = FFMAX(sub->num_rects, 1);
>      else
>          nb = 1;
>  
> @@ -1044,7 +1049,7 @@ static void do_subtitle_out(OutputFile *of,
>      if (output_files[ost->file_index]->start_time != AV_NOPTS_VALUE)
>          pts -= output_files[ost->file_index]->start_time;
>      for (i = 0; i < nb; i++) {
> -        unsigned save_num_rects = sub->num_rects;
> +        AVSubtitle local_sub = *sub;
>  
>          if (!check_recording_time(ost, pts, AV_TIME_BASE_Q))
>              return;
> @@ -1053,19 +1058,22 @@ static void do_subtitle_out(OutputFile *of,
>          if (ret < 0)
>              report_and_exit(AVERROR(ENOMEM));
>  
> -        sub->pts = pts;
> +        local_sub.pts = pts;
>          // start_display_time is required to be 0
> -        sub->pts               += av_rescale_q(sub->start_display_time, (AVRational){ 1, 1000 }, AV_TIME_BASE_Q);
> -        sub->end_display_time  -= sub->start_display_time;
> -        sub->start_display_time = 0;
> -        if (i == 1)
> -            sub->num_rects = 0;
> +        local_sub.pts               += av_rescale_q(sub->start_display_time, (AVRational){ 1, 1000 }, AV_TIME_BASE_Q);
> +        local_sub.end_display_time  -= sub->start_display_time;
> +        local_sub.start_display_time = 0;
> +
> +        if (enc->codec_id == AV_CODEC_ID_DVB_SUBTITLE && i == 1)
> +            local_sub.num_rects = 0;
> +        else if (single_rect && sub->num_rects > 0) {
> +            local_sub.num_rects = 1;
> +            local_sub.rects += i;
> +        }
>  
>          ost->frames_encoded++;
>  
> -        subtitle_out_size = avcodec_encode_subtitle(enc, pkt->data, pkt->size, sub);
> -        if (i == 1)
> -            sub->num_rects = save_num_rects;
> +        subtitle_out_size = avcodec_encode_subtitle(enc, pkt->data, pkt->size, &local_sub);
>          if (subtitle_out_size < 0) {
>              av_log(ost, AV_LOG_FATAL, "Subtitle encoding failed\n");
>              exit_program(1);

Regards,

-- 
  Nicolas George
_______________________________________________
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".

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [FFmpeg-devel] [PATCH 2/3] lavc/assenc: set AV_CODEC_CAP_SINGLE_SUB_RECT
  2023-02-21  0:25 ` [FFmpeg-devel] [PATCH 2/3] lavc/assenc: set AV_CODEC_CAP_SINGLE_SUB_RECT rcombs
@ 2023-02-21  7:48   ` Nicolas George
  2023-02-21  8:12     ` Ridley Combs
  0 siblings, 1 reply; 11+ messages in thread
From: Nicolas George @ 2023-02-21  7:48 UTC (permalink / raw)
  To: FFmpeg development discussions and patches

rcombs (12023-02-20):
> This already gave garbled output when multiple rects were present,
> so this is simply documenting an existing requirement.
> ---
>  libavcodec/assenc.c | 2 ++
>  1 file changed, 2 insertions(+)

NAK: the code has provisions for multiple rectangles, if you enforce a
single rectangle you need to remove the code that is now useless.

But I do not think pushing the issue onto the applications is a good way
to fix the problem. Or even pushing the issue onto the framework, since
the framework does not know the specifics. Better fix the code in ASS
that handles multiple rectangles than inventing yet another annoying
flag. A single frame can be encoded into multiple packets, so that
should not be hard.

Regards,

-- 
  Nicolas George
_______________________________________________
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".

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [FFmpeg-devel] [PATCH 1/3] lavc/codec.h: add AV_CODEC_CAP_SINGLE_SUB_RECT
  2023-02-21  7:42 ` [FFmpeg-devel] [PATCH 1/3] lavc/codec.h: add AV_CODEC_CAP_SINGLE_SUB_RECT Nicolas George
@ 2023-02-21  8:07   ` Ridley Combs
  0 siblings, 0 replies; 11+ messages in thread
From: Ridley Combs @ 2023-02-21  8:07 UTC (permalink / raw)
  To: ffmpeg-devel



> On Feb 21, 2023, at 01:42, Nicolas George <george@nsup.org> wrote:
> 
> rcombs (12023-02-20):
>> ---
>> doc/APIchanges       | 3 +++
>> libavcodec/codec.h   | 5 +++++
>> libavcodec/version.h | 2 +-
>> 3 files changed, 9 insertions(+), 1 deletion(-)
> 
> And... No change to the framework to make use of this flag? Like, return
> AVERROR(EINVAL) if the flag is present but several rectangles are given?

I didn't want to break existing applications further, though I don't feel strongly about it either way (since the existing behavior is already very poor).

> 
> Regards,
> 
> -- 
>  Nicolas George
> _______________________________________________
> 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".

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [FFmpeg-devel] [PATCH 3/3] ffmpeg: respect AV_CODEC_CAP_SINGLE_SUB_RECT
  2023-02-21  7:45   ` Nicolas George
@ 2023-02-21  8:09     ` Ridley Combs
  0 siblings, 0 replies; 11+ messages in thread
From: Ridley Combs @ 2023-02-21  8:09 UTC (permalink / raw)
  To: ffmpeg-devel



> On Feb 21, 2023, at 01:45, Nicolas George <george@nsup.org> wrote:
> 
> rcombs (12023-02-20):
>> Fixes ASS output when multiple rects are present.
>> ---
>> fftools/ffmpeg.c | 28 ++++++++++++++++++----------
>> 1 file changed, 18 insertions(+), 10 deletions(-)
> 
> Does this not belong to the framework?

I don't see a way it could be? avcodec_encode_subtitle takes an AVSubtitle and writes to a single caller-provided buffer; there's no way for it to generate multiple outputs for a single input.

> 
>> 
>> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
>> index 9884e0c6c6..23eac52438 100644
>> --- a/fftools/ffmpeg.c
>> +++ b/fftools/ffmpeg.c
>> @@ -1021,6 +1021,7 @@ static void do_subtitle_out(OutputFile *of,
>>     AVCodecContext *enc;
>>     AVPacket *pkt = ost->pkt;
>>     int64_t pts;
>> +    int single_rect;
>> 
>>     if (sub->pts == AV_NOPTS_VALUE) {
>>         av_log(ost, AV_LOG_ERROR, "Subtitle packets must have a pts\n");
>> @@ -1031,11 +1032,15 @@ static void do_subtitle_out(OutputFile *of,
>> 
>>     enc = ost->enc_ctx;
>> 
>> +    single_rect = !!(enc->codec->capabilities & AV_CODEC_CAP_SINGLE_SUB_RECT);
>> +
>>     /* Note: DVB subtitle need one packet to draw them and one other
>>        packet to clear them */
>>     /* XXX: signal it in the codec context ? */
>>     if (enc->codec_id == AV_CODEC_ID_DVB_SUBTITLE)
>>         nb = 2;
>> +    else if (single_rect)
>> +        nb = FFMAX(sub->num_rects, 1);
>>     else
>>         nb = 1;
>> 
>> @@ -1044,7 +1049,7 @@ static void do_subtitle_out(OutputFile *of,
>>     if (output_files[ost->file_index]->start_time != AV_NOPTS_VALUE)
>>         pts -= output_files[ost->file_index]->start_time;
>>     for (i = 0; i < nb; i++) {
>> -        unsigned save_num_rects = sub->num_rects;
>> +        AVSubtitle local_sub = *sub;
>> 
>>         if (!check_recording_time(ost, pts, AV_TIME_BASE_Q))
>>             return;
>> @@ -1053,19 +1058,22 @@ static void do_subtitle_out(OutputFile *of,
>>         if (ret < 0)
>>             report_and_exit(AVERROR(ENOMEM));
>> 
>> -        sub->pts = pts;
>> +        local_sub.pts = pts;
>>         // start_display_time is required to be 0
>> -        sub->pts               += av_rescale_q(sub->start_display_time, (AVRational){ 1, 1000 }, AV_TIME_BASE_Q);
>> -        sub->end_display_time  -= sub->start_display_time;
>> -        sub->start_display_time = 0;
>> -        if (i == 1)
>> -            sub->num_rects = 0;
>> +        local_sub.pts               += av_rescale_q(sub->start_display_time, (AVRational){ 1, 1000 }, AV_TIME_BASE_Q);
>> +        local_sub.end_display_time  -= sub->start_display_time;
>> +        local_sub.start_display_time = 0;
>> +
>> +        if (enc->codec_id == AV_CODEC_ID_DVB_SUBTITLE && i == 1)
>> +            local_sub.num_rects = 0;
>> +        else if (single_rect && sub->num_rects > 0) {
>> +            local_sub.num_rects = 1;
>> +            local_sub.rects += i;
>> +        }
>> 
>>         ost->frames_encoded++;
>> 
>> -        subtitle_out_size = avcodec_encode_subtitle(enc, pkt->data, pkt->size, sub);
>> -        if (i == 1)
>> -            sub->num_rects = save_num_rects;
>> +        subtitle_out_size = avcodec_encode_subtitle(enc, pkt->data, pkt->size, &local_sub);
>>         if (subtitle_out_size < 0) {
>>             av_log(ost, AV_LOG_FATAL, "Subtitle encoding failed\n");
>>             exit_program(1);
> 
> Regards,
> 
> -- 
>  Nicolas George
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org <mailto:ffmpeg-devel@ffmpeg.org>
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org <mailto:ffmpeg-devel-request@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-request@ffmpeg.org with subject "unsubscribe".

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [FFmpeg-devel] [PATCH 2/3] lavc/assenc: set AV_CODEC_CAP_SINGLE_SUB_RECT
  2023-02-21  7:48   ` Nicolas George
@ 2023-02-21  8:12     ` Ridley Combs
  2023-02-22 13:24       ` Nicolas George
  0 siblings, 1 reply; 11+ messages in thread
From: Ridley Combs @ 2023-02-21  8:12 UTC (permalink / raw)
  To: ffmpeg-devel



> On Feb 21, 2023, at 01:48, Nicolas George <george@nsup.org> wrote:
> 
> rcombs (12023-02-20):
>> This already gave garbled output when multiple rects were present,
>> so this is simply documenting an existing requirement.
>> ---
>> libavcodec/assenc.c | 2 ++
>> 1 file changed, 2 insertions(+)
> 
> NAK: the code has provisions for multiple rectangles, if you enforce a
> single rectangle you need to remove the code that is now useless.

Fair enough, if we're fine with breaking the existing case further. Should I simply drop rectangles after a first, or return an error?

> 
> But I do not think pushing the issue onto the applications is a good way
> to fix the problem. Or even pushing the issue onto the framework, since
> the framework does not know the specifics. Better fix the code in ASS
> that handles multiple rectangles than inventing yet another annoying
> flag. A single frame can be encoded into multiple packets, so that
> should not be hard.

This is only true for audio/video encoders; subtitle encoders still use a different API, which does not have M:N support. There's some long-ongoing work to change that, but for now, this seems like the only way to deal with this case before that API overhaul.

> 
> Regards,
> 
> -- 
>  Nicolas George
> _______________________________________________
> 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".

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [FFmpeg-devel] [PATCH 2/3] lavc/assenc: set AV_CODEC_CAP_SINGLE_SUB_RECT
  2023-02-21  8:12     ` Ridley Combs
@ 2023-02-22 13:24       ` Nicolas George
  0 siblings, 0 replies; 11+ messages in thread
From: Nicolas George @ 2023-02-22 13:24 UTC (permalink / raw)
  To: FFmpeg development discussions and patches

Ridley Combs (12023-02-21):
> Fair enough, if we're fine with breaking the existing case further.
> Should I simply drop rectangles after a first, or return an error?

You have to ask? Between reporting an error and silently corrupting
data, the answer is never in doubt.

> This is only true for audio/video encoders; subtitle encoders still
> use a different API, which does not have M:N support. There's some
> long-ongoing work to change that, but for now, this seems like the
> only way to deal with this case before that API overhaul.

Oh, I had forgotten this. In that cas, I would suggest to keep API
changes to a minimum:

- Have assenc print and return an error if there are several rectangles.

- Add a special case in the command-line tool ffmpeg based on the
  encoding codec. Or probably even better, for all text subtitles.

This will not interfere with overhauling the subtitles encoding API.

Of course, other developers might disagree, give it a few days.

Regards,

-- 
  Nicolas George
_______________________________________________
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".

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [FFmpeg-devel] [PATCH 3/3] ffmpeg: respect AV_CODEC_CAP_SINGLE_SUB_RECT
  2023-02-21  0:25 ` [FFmpeg-devel] [PATCH 3/3] ffmpeg: respect AV_CODEC_CAP_SINGLE_SUB_RECT rcombs
  2023-02-21  7:45   ` Nicolas George
@ 2023-03-01 15:19   ` TADANO Tokumei
  1 sibling, 0 replies; 11+ messages in thread
From: TADANO Tokumei @ 2023-03-01 15:19 UTC (permalink / raw)
  To: ffmpeg-devel

On 2023/02/21 9:25, rcombs wrote:
> Fixes ASS output when multiple rects are present.
> ---
>   fftools/ffmpeg.c | 28 ++++++++++++++++++----------
>   1 file changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> index 9884e0c6c6..23eac52438 100644
> --- a/fftools/ffmpeg.c
> +++ b/fftools/ffmpeg.c
> @@ -1021,6 +1021,7 @@ static void do_subtitle_out(OutputFile *of,
>       AVCodecContext *enc;
>       AVPacket *pkt = ost->pkt;
>       int64_t pts;
> +    int single_rect;
>   
>       if (sub->pts == AV_NOPTS_VALUE) {
>           av_log(ost, AV_LOG_ERROR, "Subtitle packets must have a pts\n");
> @@ -1031,11 +1032,15 @@ static void do_subtitle_out(OutputFile *of,
>   
>       enc = ost->enc_ctx;
>   
> +    single_rect = !!(enc->codec->capabilities & AV_CODEC_CAP_SINGLE_SUB_RECT);
> +
>       /* Note: DVB subtitle need one packet to draw them and one other
>          packet to clear them */
>       /* XXX: signal it in the codec context ? */
>       if (enc->codec_id == AV_CODEC_ID_DVB_SUBTITLE)
>           nb = 2;
> +    else if (single_rect)

If the problem exists on assenc only, how about using codec id to be hard-coded?

     else if (enc->codec_id == AV_CODEC_ID_ASS)

It meets your goal without changing framework (but may be dirty solution).
The current code already hard-codes AV_CODEC_ID_DVB_SUBTITLE as a special case.

> +        nb = FFMAX(sub->num_rects, 1);
>       else
>           nb = 1;
>   
> @@ -1044,7 +1049,7 @@ static void do_subtitle_out(OutputFile *of,
>       if (output_files[ost->file_index]->start_time != AV_NOPTS_VALUE)
>           pts -= output_files[ost->file_index]->start_time;
>       for (i = 0; i < nb; i++) {
> -        unsigned save_num_rects = sub->num_rects;
> +        AVSubtitle local_sub = *sub;

It is better to put it out from "for" loop.
It takes costs to copy entire structure on each loop.

>   
>           if (!check_recording_time(ost, pts, AV_TIME_BASE_Q))
>               return;
> @@ -1053,19 +1058,22 @@ static void do_subtitle_out(OutputFile *of,
>           if (ret < 0)
>               report_and_exit(AVERROR(ENOMEM));
>   
> -        sub->pts = pts;
> +        local_sub.pts = pts;
>           // start_display_time is required to be 0
> -        sub->pts               += av_rescale_q(sub->start_display_time, (AVRational){ 1, 1000 }, AV_TIME_BASE_Q);
> -        sub->end_display_time  -= sub->start_display_time;
> -        sub->start_display_time = 0;
> -        if (i == 1)
> -            sub->num_rects = 0;
> +        local_sub.pts               += av_rescale_q(sub->start_display_time, (AVRational){ 1, 1000 }, AV_TIME_BASE_Q);
> +        local_sub.end_display_time  -= sub->start_display_time;
> +        local_sub.start_display_time = 0;
> +
> +        if (enc->codec_id == AV_CODEC_ID_DVB_SUBTITLE && i == 1)
> +            local_sub.num_rects = 0;
> +        else if (single_rect && sub->num_rects > 0) {
> +            local_sub.num_rects = 1;
> +            local_sub.rects += i;
> +        }
>   
>           ost->frames_encoded++;
>   
> -        subtitle_out_size = avcodec_encode_subtitle(enc, pkt->data, pkt->size, sub);
> -        if (i == 1)
> -            sub->num_rects = save_num_rects;
> +        subtitle_out_size = avcodec_encode_subtitle(enc, pkt->data, pkt->size, &local_sub);
>           if (subtitle_out_size < 0) {
>               av_log(ost, AV_LOG_FATAL, "Subtitle encoding failed\n");
>               exit_program(1);
_______________________________________________
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".

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2023-03-01 15:19 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-21  0:25 [FFmpeg-devel] [PATCH 1/3] lavc/codec.h: add AV_CODEC_CAP_SINGLE_SUB_RECT rcombs
2023-02-21  0:25 ` [FFmpeg-devel] [PATCH 2/3] lavc/assenc: set AV_CODEC_CAP_SINGLE_SUB_RECT rcombs
2023-02-21  7:48   ` Nicolas George
2023-02-21  8:12     ` Ridley Combs
2023-02-22 13:24       ` Nicolas George
2023-02-21  0:25 ` [FFmpeg-devel] [PATCH 3/3] ffmpeg: respect AV_CODEC_CAP_SINGLE_SUB_RECT rcombs
2023-02-21  7:45   ` Nicolas George
2023-02-21  8:09     ` Ridley Combs
2023-03-01 15:19   ` TADANO Tokumei
2023-02-21  7:42 ` [FFmpeg-devel] [PATCH 1/3] lavc/codec.h: add AV_CODEC_CAP_SINGLE_SUB_RECT Nicolas George
2023-02-21  8:07   ` Ridley Combs

Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://master.gitmailbox.com/ffmpegdev/0 ffmpegdev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 ffmpegdev ffmpegdev/ https://master.gitmailbox.com/ffmpegdev \
		ffmpegdev@gitmailbox.com
	public-inbox-index ffmpegdev

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git