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

[Erik Miranda]

On 18/04/2012 2:25 AM, Erik Miranda wrote:
> On 17/04/2012 8:18 PM, Erik Miranda wrote:
>> On 17/04/2012 7:15 PM, Michael Niedermayer wrote:
>>> On Tue, Apr 17, 2012 at 06:52:00PM -0400, Erik Miranda wrote:
>>>> On 17/04/2012 10:50 AM, Michael Niedermayer wrote:
>>>>> On Mon, Apr 16, 2012 at 09:44:48PM +0200, Reimar Döffinger wrote:
>>>>>> On 16 Apr 2012, at 12:45, Joakim Plate<elupus at ecce.se>   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.
>>>>> Is there really a ABI/API issue with adding fields to 
>>>>> AVSubtitleRect ?
>>>>> rect is a array of pointers
>>>>>
>>>>> assuming theres no inherent problem with adding fields to
>>>>> AVSubtitleRect
>>>>> to be able to maintain compatibility with forks of ffmpeg (which we
>>>>> try to currently)
>>>>> a bit of extra red tape is needed see fef411ef for how it was done 
>>>>> for
>>>>> AVFrame
>>>>> Also just grep for avcodec_get_frame_class() to see code using this
>>>>>
>>>>> Code inside the lib declaring the AVSubtitleRect struct of course can
>>>>> do direct accesses
>>>>> its just applications which want to use shared libs that should avoid
>>>>> direct access
>>>>> This way shared libs can be exchanged between various forks in a 
>>>>> binary
>>>>> compatibile way.
>>>>>
>>>>> [...]
>>>> Just for clarification, I took a look at fef411ef, and I presume the
>>>> idea is to AVOption enable AVSubtitleRect. When I read the
>>>> documentation on AVOption, normally the struct would begin with a
>>>> "class" field, but this would break compatibility which is why I
>>>> understand the class field was not added when doing the same for
>>>> AVFrame in fef411ef, and instead a "get_frame_class" function was
>>>> added instead, correct?
>>> yes
>>>
>>>
>>>> Also, since this would be (from my
>>>> understanding) a "fake" object, would I need to make any changes to
>>>> follow the procedure of av_opt_set_defaults() and av_opt_free()?
>>> do you need these 2 ?
>>>
>>> [...]
>> No, I don't. I was mearly double-checking that it would be okay to 
>> write the code without using these two since we are deveating from 
>> the conventions specified in the AVOption documentation.
> This patch adds an AVClass for the AVSubtitleRect structure, and 
> should be applied in addition to my previous patch. I wasn't quite 
> sure what fields should be added to the AVClass, since fef411ef added 
> only a small subset of the fields in AVFrame. So I exposed the new 
> forced field, and the positional/dimensional fields (since those 
> seemed to be ones exposed in AVFrame). I had thought of adding all 
> fields, but I'm not quite sure how to add the AVPicture field, or even 
> if that is possible with the current implementation of AVOption. So, 
> let me know if you have any comments in this matter.

Are there any further comments on this pair of patches? As this is the 
first time I am submitting a patch, if the change looks okay, what is 
the process for getting the patches included? Do I push the changes 
myself, or does one of the core developers take care of the merging?