| Message ID | 20230221002516.25784-2-rcombs@rcombs.me |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/3] lavc/codec.h: add AV_CODEC_CAP_SINGLE_SUB_RECT | expand |
| Context | Check | Description |
|---|---|---|
| andriy/configure_x86 | warning | Failed to apply patch |
| yinshiyou/configure_loongarch64 | warning | Failed to apply patch |
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,
> 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".
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,
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), };