[FFmpeg-devel] [PATCH] Exposing forced flag for DVD and PGS subtitles

[Reimar Döffinger]

On 19 Apr 2012, at 12:02, Erik Miranda <erikmiranda at gmail.com> wrote:
> On 18/04/2012 5:48 PM, Reimar Döffinger wrote:
>> On Tue, Apr 17, 2012 at 04:53:57AM -0400, Erik Miranda wrote:
>>> On Apr 16 21:44:48 CEST 2012, Reimar Döffinger<Reimar.Doeffinger at gmx.de>   wrote:
>>>> On 16 Apr 2012, at 12:45, Joakim Plate<elupus at ecce.se<http://ffmpeg.org/mailman/listinfo/ffmpeg-devel>>   wrote:
>>>>> /  Erik Miranda<erikmiranda<at>   gmail.com>   writes:
>>> />>>/  This patch adds a "forced" member to the AVSubtitleRect structure and
>>> />>>/  implements setting this flag in the DVD and PGS subtitle decoders, since
>>> />>>/  these are the formats I'm aware of that include such a function. Since
>>> />>>/  this patch changes AVSubtitleRect, it leads to a public API change. As
>>> />>>/  far as I can tell, this is the best method to allow access to this
>>> />>>/  information. Please advise me if otherwise, or if further changes are
>>> />>>/  required as part of changing AVSubtitleRect. I think the libavcodec
>>> />>>/  version would need a bump, correct?
>>> />>/
>>> />>/  There is already a content dispostion type for forced. That is stream
>>> />>/  global thou. So one option would be to duplicate forced subs into a
>>> />>/  secondary subtitle stream.
>>> />
>>>> What would be the point? What would someone do with that information except drop all non-forced ones?
>>>> I guess it might be relevant if you want to preserve the info when reencoding.
>>>> It should be possible to avoid the API/ABI breakage by adding an extra array to AVSubtitle instead of putting it into the AVSubtitleRect.
>>>> Though we might consider changing the whole API so extending the rect is possible without changing the API in the future.
>>> In addition to reencoding, if I were to write, for example, a program which allows one to open and
>>> analyze or edit subtitles, it would be useful to decode all subtitles while having access to the forced
>>> flag. I'm sure there are other use cases as well. I find having to duplicate the stream and compare which
>>> subtitles are present in both the forced-only stream and the all subtitles stream a very hackish
>>> approach to access a piece of information that is so readily available. I chose AVSubtitleRect because
>>> it seemed the most logical location to look for such a piece of information. I don't quite understand
>>> how one would add an array to AVSubtitle without encountering similar breakage.
>> I missed that we do not use an allocation function for AVSubtitle, so
>> that would probably leave the only option to add it without breaking
>> API to add it to the AVCodecContext, which is a bit ugly.
> 
> We don't use an allocation function for AVSubtitle, but the AVSubtitleRect structures are allocated within the decoders (for example pgssubdec.c:405), and the function avsubtitle_free() is used to deallocate the rects. So decoding-wise, I don't believe there should be any problem with adding a field. But now that you mention it, it may be a problem if AVSubtitleRect was allocated externally for for encoding. Perhaps we could include a subtitle_rect_version/spec/size field in AVCodecContext structure or another location which must be set and which an encoder should check before it attempts to access the "forced" field (or any future fields).

Actually, I mistakenly believed we were using AVSubtitleRect *, but we actually use **.
So there actually shouldn't be any issue at all with adding something to AVSubtitleRect.
Sorry for that.