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

[Erik Miranda]

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.
-------------- next part --------------
>From a654449d4e47ffd27538238221577891f4020905 Mon Sep 17 00:00:00 2001
From: hakuya <erikmiranda at gmail.com>
Date: Wed, 18 Apr 2012 02:08:25 -0400
Subject: [PATCH 2/2] Added AVClass for AVSubtitleRect

---
 libavcodec/avcodec.h |    8 ++++++++
 libavcodec/options.c |   24 ++++++++++++++++++++++++
 2 files changed, 32 insertions(+), 0 deletions(-)

diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
index 187dba0..2a6424d 100644
--- a/libavcodec/avcodec.h
+++ b/libavcodec/avcodec.h
@@ -3281,6 +3281,14 @@ const AVClass *avcodec_get_class(void);
 const AVClass *avcodec_get_frame_class(void);
 
 /**
+ * Get the AVClass for AVSubtitleRect. It can be used in combination with
+ * AV_OPT_SEARCH_FAKE_OBJ for examining options.
+ *
+ * @see av_opt_find().
+ */
+const AVClass *avcodec_get_subtitle_rect_class(void);
+
+/**
  * Copy the settings of the source AVCodecContext into the destination
  * AVCodecContext. The resulting destination codec context will be
  * unopened, i.e. you are required to call avcodec_open2() before you
diff --git a/libavcodec/options.c b/libavcodec/options.c
index ef598d5..58a7e14 100644
--- a/libavcodec/options.c
+++ b/libavcodec/options.c
@@ -253,3 +253,27 @@ const AVClass *avcodec_get_frame_class(void)
 {
     return &av_frame_class;
 }
+
+#define SROFFSET(x) offsetof(AVSubtitleRect,x)
+
+static const AVOption subtitle_rect_options[]={
+{"x", "", SROFFSET(x), AV_OPT_TYPE_INT, {.dbl = 0 }, 0, INT_MAX, 0},
+{"y", "", SROFFSET(y), AV_OPT_TYPE_INT, {.dbl = 0 }, 0, INT_MAX, 0},
+{"w", "", SROFFSET(w), AV_OPT_TYPE_INT, {.dbl = 0 }, 0, INT_MAX, 0},
+{"h", "", SROFFSET(h), AV_OPT_TYPE_INT, {.dbl = 0 }, 0, INT_MAX, 0},
+{"type", "", SROFFSET(type), AV_OPT_TYPE_INT, {.dbl = 0 }, 0, INT_MAX, 0},
+{"forced", "", SROFFSET(forced), AV_OPT_TYPE_INT, {.dbl = 0}, 0, 1, 0},
+{NULL},
+};
+
+static const AVClass av_subtitle_rect_class = {
+    .class_name             = "AVSubtitleRect",
+    .item_name              = NULL,
+    .option                 = subtitle_rect_options,
+    .version                = LIBAVUTIL_VERSION_INT,
+};
+
+const AVClass *avcodec_get_subtitle_rect_class(void)
+{
+    return &av_subtitle_rect_class;
+}
-- 
1.7.3.4