These should simply be set directly in the AVOption array.

These provide only default behavior.
Former one can be embeded in the AVOption array. The default behavior would not be changed.
Latter one depends when most of softwares (or major softwares) can handle multi-rectangle ASS subtitle. Thus, I prefer to put it out from the AVOption array.

If you get an allocation failure, you should return AVERROR(ENOMEM), rather than attempting to continue in a degraded state.

This does't continue in a degraded state, but return AVERROR(ENOMEM) after clean up.

            while (count)
                av_freep(&font_families[--count]);
            av_freep(&font_families);
            if (is_nomem) {
                aribcaption_close(avctx);
                return AVERROR(ENOMEM);
            }

If such clean up is not needed (i.e., automatically freed), I can simply return with AVERROR(ENOMEM).

You can add a label (e.g. fail:) and goto it on failure.

It is a matter of preference if it is not specified in coding rule.
I prefer to keep current way because same clean up routine runs regardless of error or not.

Instead of while (!is_nomem && *fonts) loop, I can change to add break; at allocation failure to quit from loop.

Sure, that's a bit cleaner.

Why not just make sans-serif the default for the AVOption?

The libaribcaption library has it's default font for bitmap depending on operating system. I would like to follow it if font option is not specified.
On the other hand, we have to set default font for ASS subtitle.

This isn't using a default from libaribcaption, though? It's just using DEFAULT_FONT_FAMILY, which is defined as sans-serif.

libaribcaption renders (with default font) when sub_type is bitmap only.
When sub_type is ass, libaribcaption parses ARIB caption, but does not render nor encode to ASS text.
The DEFAULT_FONT_FAMILY is set to ASS header only, but most players such as mpv set its own ASS header and specified font is ignored.

Ahh, maybe name the macro ASS_DEFAULT_FONT or the like for clarity?

ASS_DEFAULT_FONT is already defined in libavcodec/ass.h but is "Arial".
The font is not suitable for ARIB caption.
Which is your preference, redifine with same macro name or define another new (but ASS related) macro name?

Hmm, maybe DEFAULT_FONT_ASS? And add a comment indicating that it's only used for ASS, because the bitmap renderer has its own internal default.

This section isn't appropriate. Instead, provide an AVOption with type AV_OPT_TYPE_IMAGE_SIZE in your decoder, defaulting to the layout dimensions derived from the subtitle stream itself, and allow player apps to set it to an appropriate value (this is usually the output resolution) when using the bitmap renderer.

I'll consider it.

I add -canvas_size option to specify input video size.
The same option already exists in ffmpeg tool, and it set width and height in AVCodecContext. Since ffmpeg tool doesn't pass -canvas_size option to libaribcaption, thus I use the values in AVCodecContext. The -canvas_size option in AVOption is used by other tools such as ffplay.

Note that the canvas_size specifies frame size of input video (i.e., SAR basis). Actual canvas size is stretched to 16:9 to match DAR-derived video size.

This is meant to control libavcodec's built-in encoding conversion infrastructure. If you want to let callers control the encoding libaribcaption uses, you should provide an AVOption defaulting to ARIBCC_ENCODING_SCHEME_AUTO.

Encoding depends on broadcast source (i.e., country).
Most of cases, it should be determined automatically.
It is last resort when it cannot be determined automatically.
If sub_charenc_mode is not suitable means, I'll add one in AVOption.

Tentatively, I add -encoding_scheme option locally and specify it as same value as aribcc_encoding_scheme_t:

• auto
• jis
• utf8
• latin

I concern the option name might be too generic and it may conflct with other codecs.

Nice! If you want to be more specific, you can use arib_text_encoding or the like, but I don't think it's a big deal either way.

I determined the option name as -caption_encoding.
This should not be confused.

You have a lot of calls to aribcaption_close on error paths in this function. You can instead set FF_CODEC_CAP_INIT_CLEANUP in caps_internal, which will result in libavcodec automatically calling your close routine if initialization fails.

Thanks. I'll try it.

The {\r} shouldn't be necessary here; you should be able to just use an empty string.

What behavior are you seeing in mpv with this set to false? If this isn't working properly, it should be fixed (either in the decoder or in mpv).

There is a mpv implementation that can handle multi-rectangle ASS subtitle:
https://github.com/0p1pp1/mpv

You can find typical case in https://streams.videolan.org/streams/ts/ARIB/japan/osaka_15.ts around 00:03:35.

2 captions are displayed on each person at a same time.
You can see same scene with ffplay -sub_type bitmap osaka_15.ts after this patchset is applied.

Current plain mpv cannot handle multi-rectangle ASS subtitle.
If plain mpv plays it with -ass_workaround false, first caption is immediately clear after second caption is displayed; i.e., only second caption can be seen.

This works fine in current mpv (mpv-player/mpv@7990dd8) for me?

Oh, my knowledge is little bit old.
I found mpv release/0.35 can handle multi-rectangle ASS subtitles.

I would like change default value of ass_workaround to 0, but stay default definition to be outside of AVOption.

I don't understand why you want the default to be a macro.

Also, I'd suggest naming the option single_rect to clarify its behavior.

The reason is explained in later description (but should amend the explanation to revert default value).

The default is @var{true}.

If a player can handle multi-rectangle ASS subtitle and you prefer
more suitable positioning, set this option to @var{false} or define
@env{ASS_WORKAROUND=0} to change default behavior at compilation.

This can easily change default behavior without change the code.

I will change the option name to ass_single_rect.

You don't need to if around this; av_freep(&x) where x is null is a no-op.

This should check if the alpha is different; if it changes to 0, we should set that.

Actual ISDB broadcasting doesn't allways set alpha value, and 0 is set in many cases.
Alpha value 0 means transparent, and as the result subtitle cannot be seen if we strictly obey it.

0 in ASS means fully opaque; 0xFF means fully transparent. I think you need to invert the value here.

I don't see any corresponding code in libaribcaption's renderer to ignore 0 values?

Is it true?
If so, above line 559 should not changed. Instead, ASS style override code number would be 0xFF - alpha.

Correct, though I'm still confused about why this check is necessary when libaribcaption's renderer doesn't seem to have anything equivalent (unless I'm missing something?). In any case, it at least needs a comment explaining why.

I checked some TS files on my hand. I can't find text color with alpha 0. So, I might see apparition.
I'll remove the check.

This patch doesn't build as-is, because FF_CODEC_CAP_INIT_THREADSAFE is now the default, and the macro has been removed.

This line only makes sense on the ass_workaround path, and the {\\r} is only necessary if inserting the newline, so this could be:

The {\\r} is diverted from former libaribb24.
It seems it is not required for multi-rect ASS, I will change the code as you suggested.

"can't handle multiple ASS rectangles in a single AVSubtitle, or multiple ASS rectangles of indeterminate duration with the same start timestamp"

also, "in a single rectangle"

"cannot handle AVSubtitles with multiple ASS rectangles properly,"

"Specify the resolution of the canvas to render subtitles to; usually, this should be either the resolution of the video or the resolution it's being displayed at. This only applies when @var{subtitle_type} is set to @var{bitmap}."

First sentence is still under considering as discussed below.

This should be an int, since enum aribcc_encoding_scheme_t isn't guaranteed to have the same size as int across all platforms.

"below"

Why's this?

The profile C is defined for devices that have very limited resources.
Please refer ARIB TR-B14 version 3.8 Fascicle 1-(2/2) Volume 3 [Section 4] "Operational provisions related to C-profile". The section has been removed from version 6.7. The Table 3-1 explains that the caption may be up to 4 lines, and Table 3-2 explains the position is not provided by contents.
Thus, I think it is better to treat it as single ASS rectangle.

At present, mpv doesn't display ASS subtitle of profile C correctly even if this change is applied. I don't know the reason. Output of ffmpeg -i INPUT.TS -map STREM_ID_OF_PROFILE_C OUTPUT.ass seems to be correct, but mpv displays garbled characters.
In addition, similar change should be made for bitmap subtiles, but I don't have easy resolution.

Do you have a sample file I can take a look at?

If you decide to keep this, you should include a comment noting that profile C doesn't allow for positioning.

You can see it in the same TS file:
https://streams.videolan.org/streams/ts/ARIB/japan/osaka_15.ts

ffmpeg -fix_sub_duration -i osaka_15.ts -map 0:14 osaka_15.ass
mpv osaka_15.ts --vid=2 --aid=2 --sid=2

Recently @jeeb found the root cause and sent a patch to mpv:
mpv-player/mpv#11340
You can see the subtitles by mpv with this patch.

Would definitely be worth reporting to @xqq. In any case, worth a comment for now :)

I already shared such information with @xqq.

With latest patch for this PR, you can see bitmap subtitle of profile C by ffplay.

ffplay -sub_type bitmap osaka_15.ts -vst 13 -ast 12 -sst 14

This flag should only affect setting readorder; the rest of the function should run regardless.

• none of these checks should be required; these functions are safe to pass NULL to
• it's not necessary to zero the pointers, since the ctx is about to be freed anyway.

these functions are safe to pass NULL to

I'm not sure but it depends on current libaribcaption's implementation (not on the spec).
I woule like to keep the checks.

I'll accept latter suggestion.