Re: [FFmpeg-devel] [PATCH 2/2] Move AVSubtitle from lavc to lavu
On Sun, 14 Dec 2014 16:47:44 +0100
Clément Bœsch u...@pkh.me wrote:
---
doc/APIchanges | 5 +++
libavcodec/avcodec.h | 63 +++---
libavcodec/utils.c | 29 +++---
libavcodec/version.h | 3 ++
libavutil/Makefile | 2 +
libavutil/subtitle.c | 62 +
libavutil/subtitle.h | 107
+++
7 files changed, 191 insertions(+), 80 deletions(-)
create mode 100644 libavutil/subtitle.c
create mode 100644 libavutil/subtitle.h
diff --git a/doc/APIchanges b/doc/APIchanges
index ba7eae1..989794a 100644
--- a/doc/APIchanges
+++ b/doc/APIchanges
@@ -15,6 +15,11 @@ libavutil: 2014-08-09
API changes, most recent first:
+2014-12-xx - xxx - lavc 56.16.100 / lavu 54.17.100 - lavc/avcodec.h
lavu/picture.h
+ Move AVSubtitle definition from libavcodec to libavutil and add
+ av_subtitle_*() functions which should be used instead of stack allocation
+ and the now deprecated avsubtitle_free().
+
should?
Can this requirement be lifted/delayed until there's actually a reason
to do so? At that point, the struct should probably be renamed, to
avoid turning valid code into subtly broken code merely by adding API
compatible implementation details.
2014-12-xx - xxx - lavc 56.15.100 / lavu 54.16.100 - lavc/avcodec.h
lavu/picture.h
Move AVPicture definition from libavcodec to libavutil. This should be
transparent API and ABI wise.
diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
index 2c1918d..6ded06b 100644
--- a/libavcodec/avcodec.h
+++ b/libavcodec/avcodec.h
@@ -40,6 +40,7 @@
#include libavutil/picture.h
#include libavutil/pixfmt.h
#include libavutil/rational.h
+#include libavutil/subtitle.h
#include version.h
@@ -3149,8 +3150,6 @@ typedef struct AVProfile {
typedef struct AVCodecDefault AVCodecDefault;
-struct AVSubtitle;
-
/**
* AVCodec.
*/
@@ -3401,61 +3400,6 @@ typedef struct AVHWAccel {
* @}
*/
-enum AVSubtitleType {
-SUBTITLE_NONE,
-
-SUBTITLE_BITMAP,/// A bitmap, pict will be set
-
-/**
- * Plain text, the text field must be set by the decoder and is
- * authoritative. ass and pict fields may contain approximations.
- */
-SUBTITLE_TEXT,
-
-/**
- * Formatted text, the ass field must be set by the decoder and is
- * authoritative. pict and text fields may contain approximations.
- */
-SUBTITLE_ASS,
-};
-
-#define AV_SUBTITLE_FLAG_FORCED 0x0001
-
-typedef struct AVSubtitleRect {
-int x; /// top left corner of pict, undefined when pict is not
set
-int y; /// top left corner of pict, undefined when pict is not
set
-int w; /// widthof pict, undefined when pict is not
set
-int h; /// height of pict, undefined when pict is not
set
-int nb_colors; /// number of colors in pict, undefined when pict is not
set
-
-/**
- * data+linesize for the bitmap of this subtitle.
- * can be set for text/ass as well once they are rendered
- */
-AVPicture pict;
-enum AVSubtitleType type;
-
-char *text; /// 0 terminated plain UTF-8 text
-
-/**
- * 0 terminated ASS/SSA compatible event line.
- * The presentation of this is unaffected by the other values in this
- * struct.
- */
-char *ass;
-
-int flags;
-} AVSubtitleRect;
-
-typedef struct AVSubtitle {
-uint16_t format; /* 0 = graphics */
-uint32_t start_display_time; /* relative to packet pts, in ms */
-uint32_t end_display_time; /* relative to packet pts, in ms */
-unsigned num_rects;
-AVSubtitleRect **rects;
-int64_t pts;/// Same as packet pts, in AV_TIME_BASE
-} AVSubtitle;
-
/**
* If c is NULL, returns the first registered codec,
* if c is non-NULL, returns the next registered codec after c,
@@ -3652,12 +3596,17 @@ int avcodec_open2(AVCodecContext *avctx, const
AVCodec *codec, AVDictionary **op
*/
int avcodec_close(AVCodecContext *avctx);
+#if FF_API_AVSUBTITLE
/**
* Free all allocated data in the given subtitle struct.
+ * @deprecated use av_subtitle_*() functions (the immediate equivalent of
+ * avsubtitle_free() is av_subtitle_clear())
*
* @param sub AVSubtitle to free.
*/
+attribute_deprecated
void avsubtitle_free(AVSubtitle *sub);
+#endif
/**
* @}
diff --git a/libavcodec/utils.c b/libavcodec/utils.c
index db79b67..dc0c1ec 100644
--- a/libavcodec/utils.c
+++ b/libavcodec/utils.c
@@ -39,6 +39,7 @@
#include libavutil/pixdesc.h
#include libavutil/imgutils.h
#include libavutil/samplefmt.h
+#include libavutil/subtitle.h
#include libavutil/dict.h
#include avcodec.h
#include libavutil/opt.h
@@ -1270,12 +1271,6 @@ int av_codec_get_max_lowres(const AVCodec *codec)
return
Re: [FFmpeg-devel] [PATCH 2/2] Move AVSubtitle from lavc to lavu
On Mon, 15 Dec 2014 10:38:01 +0100 Clément Bœsch u...@pkh.me wrote: On Mon, Dec 15, 2014 at 10:23:33AM +0100, wm4 wrote: On Sun, 14 Dec 2014 16:47:44 +0100 Clément Bœsch u...@pkh.me wrote: --- doc/APIchanges | 5 +++ libavcodec/avcodec.h | 63 +++--- libavcodec/utils.c | 29 +++--- libavcodec/version.h | 3 ++ libavutil/Makefile | 2 + libavutil/subtitle.c | 62 + libavutil/subtitle.h | 107 +++ 7 files changed, 191 insertions(+), 80 deletions(-) create mode 100644 libavutil/subtitle.c create mode 100644 libavutil/subtitle.h diff --git a/doc/APIchanges b/doc/APIchanges index ba7eae1..989794a 100644 --- a/doc/APIchanges +++ b/doc/APIchanges @@ -15,6 +15,11 @@ libavutil: 2014-08-09 API changes, most recent first: +2014-12-xx - xxx - lavc 56.16.100 / lavu 54.17.100 - lavc/avcodec.h lavu/picture.h + Move AVSubtitle definition from libavcodec to libavutil and add + av_subtitle_*() functions which should be used instead of stack allocation + and the now deprecated avsubtitle_free(). + should? Can this requirement be lifted/delayed until there's actually a reason to do so? The structure is unchanged for now, so yeah it's should (as a prevention for the future). I don't plan to add a field that soon, but it's highly recommended. Maybe I can change the should into must and saying that for now it has no impact, but will in the future. At that point, the struct should probably be renamed, to avoid turning valid code into subtly broken code merely by adding API compatible implementation details. There are actually 2 changes in this code, and maybe I should split. But basically the addition of the functions is kind of unrelated to the move. It's just a security for us for later, when we will be willing to add a field (if we ever do). We should have created these functions a long time ago, but the fact that AVSubtitle actually belong into libavutil prevented us from doing it: basically, if we had added these helpers, the move to libavutil would have been way more complicated (moving a structure definition is way easier than moving a structure definition AND its functions). [...] My point is: barely anyone (applications) will notice this API change. It will go unnoticed, or there will be no perceived need to update the code. And then when you actually extend this stuff, you will start breaking application code that worked before. This is how it always goes. Even ffmpeg.org still links tutorials based on ancient practices that over time became invalid, but everyone keeps doing it because it still appears to work (but crashes in slightly different situations). So I think such implicit changes should be avoided. If the API breaks, make sure applications using the old assumptions won't compile. (There are various ways to achieve this goal; I'm just saying that it should be a goal.) ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/2] Move AVSubtitle from lavc to lavu
On Mon, Dec 15, 2014 at 10:49:47AM +0100, wm4 wrote: On Mon, 15 Dec 2014 10:38:01 +0100 Clément Bœsch u...@pkh.me wrote: On Mon, Dec 15, 2014 at 10:23:33AM +0100, wm4 wrote: On Sun, 14 Dec 2014 16:47:44 +0100 Clément Bœsch u...@pkh.me wrote: --- doc/APIchanges | 5 +++ libavcodec/avcodec.h | 63 +++--- libavcodec/utils.c | 29 +++--- libavcodec/version.h | 3 ++ libavutil/Makefile | 2 + libavutil/subtitle.c | 62 + libavutil/subtitle.h | 107 +++ 7 files changed, 191 insertions(+), 80 deletions(-) create mode 100644 libavutil/subtitle.c create mode 100644 libavutil/subtitle.h diff --git a/doc/APIchanges b/doc/APIchanges index ba7eae1..989794a 100644 --- a/doc/APIchanges +++ b/doc/APIchanges @@ -15,6 +15,11 @@ libavutil: 2014-08-09 API changes, most recent first: +2014-12-xx - xxx - lavc 56.16.100 / lavu 54.17.100 - lavc/avcodec.h lavu/picture.h + Move AVSubtitle definition from libavcodec to libavutil and add + av_subtitle_*() functions which should be used instead of stack allocation + and the now deprecated avsubtitle_free(). + should? Can this requirement be lifted/delayed until there's actually a reason to do so? The structure is unchanged for now, so yeah it's should (as a prevention for the future). I don't plan to add a field that soon, but it's highly recommended. Maybe I can change the should into must and saying that for now it has no impact, but will in the future. At that point, the struct should probably be renamed, to avoid turning valid code into subtly broken code merely by adding API compatible implementation details. There are actually 2 changes in this code, and maybe I should split. But basically the addition of the functions is kind of unrelated to the move. It's just a security for us for later, when we will be willing to add a field (if we ever do). We should have created these functions a long time ago, but the fact that AVSubtitle actually belong into libavutil prevented us from doing it: basically, if we had added these helpers, the move to libavutil would have been way more complicated (moving a structure definition is way easier than moving a structure definition AND its functions). [...] My point is: barely anyone (applications) will notice this API change. It will go unnoticed, or there will be no perceived need to update the code. And then when you actually extend this stuff, you will start breaking application code that worked before. What do you suggest to make people notice the API change? Currently AVSubtitle has one single function, which I deprecated. So you think we should introduce a AVSubtitle2 into libavutil instead? I don't like that that much TBH, especially since it will require me to name the new functions av_subtitle2_* to make it consistent. [...] -- Clément B. pgpJJ3OY1BFc_.pgp Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/2] Move AVSubtitle from lavc to lavu
On Mon, 15 Dec 2014 11:38:58 +0100 Clément Bœsch u...@pkh.me wrote: On Mon, Dec 15, 2014 at 10:49:47AM +0100, wm4 wrote: On Mon, 15 Dec 2014 10:38:01 +0100 Clément Bœsch u...@pkh.me wrote: On Mon, Dec 15, 2014 at 10:23:33AM +0100, wm4 wrote: On Sun, 14 Dec 2014 16:47:44 +0100 Clément Bœsch u...@pkh.me wrote: --- doc/APIchanges | 5 +++ libavcodec/avcodec.h | 63 +++--- libavcodec/utils.c | 29 +++--- libavcodec/version.h | 3 ++ libavutil/Makefile | 2 + libavutil/subtitle.c | 62 + libavutil/subtitle.h | 107 +++ 7 files changed, 191 insertions(+), 80 deletions(-) create mode 100644 libavutil/subtitle.c create mode 100644 libavutil/subtitle.h diff --git a/doc/APIchanges b/doc/APIchanges index ba7eae1..989794a 100644 --- a/doc/APIchanges +++ b/doc/APIchanges @@ -15,6 +15,11 @@ libavutil: 2014-08-09 API changes, most recent first: +2014-12-xx - xxx - lavc 56.16.100 / lavu 54.17.100 - lavc/avcodec.h lavu/picture.h + Move AVSubtitle definition from libavcodec to libavutil and add + av_subtitle_*() functions which should be used instead of stack allocation + and the now deprecated avsubtitle_free(). + should? Can this requirement be lifted/delayed until there's actually a reason to do so? The structure is unchanged for now, so yeah it's should (as a prevention for the future). I don't plan to add a field that soon, but it's highly recommended. Maybe I can change the should into must and saying that for now it has no impact, but will in the future. At that point, the struct should probably be renamed, to avoid turning valid code into subtly broken code merely by adding API compatible implementation details. There are actually 2 changes in this code, and maybe I should split. But basically the addition of the functions is kind of unrelated to the move. It's just a security for us for later, when we will be willing to add a field (if we ever do). We should have created these functions a long time ago, but the fact that AVSubtitle actually belong into libavutil prevented us from doing it: basically, if we had added these helpers, the move to libavutil would have been way more complicated (moving a structure definition is way easier than moving a structure definition AND its functions). [...] My point is: barely anyone (applications) will notice this API change. It will go unnoticed, or there will be no perceived need to update the code. And then when you actually extend this stuff, you will start breaking application code that worked before. What do you suggest to make people notice the API change? Currently AVSubtitle has one single function, which I deprecated. That could be ok. You can't use the current API without using avsubtitle_free(). (Although I'd still prefer something more explicit.) The deprecated function must be completely removed as soon as using the new functions is mandatory. So you think we should introduce a AVSubtitle2 into libavutil instead? I don't like that that much TBH, especially since it will require me to name the new functions av_subtitle2_* to make it consistent. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 2/2] Move AVSubtitle from lavc to lavu
---
doc/APIchanges | 5 +++
libavcodec/avcodec.h | 63 +++---
libavcodec/utils.c | 29 +++---
libavcodec/version.h | 3 ++
libavutil/Makefile | 2 +
libavutil/subtitle.c | 62 +
libavutil/subtitle.h | 107 +++
7 files changed, 191 insertions(+), 80 deletions(-)
create mode 100644 libavutil/subtitle.c
create mode 100644 libavutil/subtitle.h
diff --git a/doc/APIchanges b/doc/APIchanges
index ba7eae1..989794a 100644
--- a/doc/APIchanges
+++ b/doc/APIchanges
@@ -15,6 +15,11 @@ libavutil: 2014-08-09
API changes, most recent first:
+2014-12-xx - xxx - lavc 56.16.100 / lavu 54.17.100 - lavc/avcodec.h
lavu/picture.h
+ Move AVSubtitle definition from libavcodec to libavutil and add
+ av_subtitle_*() functions which should be used instead of stack allocation
+ and the now deprecated avsubtitle_free().
+
2014-12-xx - xxx - lavc 56.15.100 / lavu 54.16.100 - lavc/avcodec.h
lavu/picture.h
Move AVPicture definition from libavcodec to libavutil. This should be
transparent API and ABI wise.
diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
index 2c1918d..6ded06b 100644
--- a/libavcodec/avcodec.h
+++ b/libavcodec/avcodec.h
@@ -40,6 +40,7 @@
#include libavutil/picture.h
#include libavutil/pixfmt.h
#include libavutil/rational.h
+#include libavutil/subtitle.h
#include version.h
@@ -3149,8 +3150,6 @@ typedef struct AVProfile {
typedef struct AVCodecDefault AVCodecDefault;
-struct AVSubtitle;
-
/**
* AVCodec.
*/
@@ -3401,61 +3400,6 @@ typedef struct AVHWAccel {
* @}
*/
-enum AVSubtitleType {
-SUBTITLE_NONE,
-
-SUBTITLE_BITMAP,/// A bitmap, pict will be set
-
-/**
- * Plain text, the text field must be set by the decoder and is
- * authoritative. ass and pict fields may contain approximations.
- */
-SUBTITLE_TEXT,
-
-/**
- * Formatted text, the ass field must be set by the decoder and is
- * authoritative. pict and text fields may contain approximations.
- */
-SUBTITLE_ASS,
-};
-
-#define AV_SUBTITLE_FLAG_FORCED 0x0001
-
-typedef struct AVSubtitleRect {
-int x; /// top left corner of pict, undefined when pict is not
set
-int y; /// top left corner of pict, undefined when pict is not
set
-int w; /// widthof pict, undefined when pict is not
set
-int h; /// height of pict, undefined when pict is not
set
-int nb_colors; /// number of colors in pict, undefined when pict is not
set
-
-/**
- * data+linesize for the bitmap of this subtitle.
- * can be set for text/ass as well once they are rendered
- */
-AVPicture pict;
-enum AVSubtitleType type;
-
-char *text; /// 0 terminated plain UTF-8 text
-
-/**
- * 0 terminated ASS/SSA compatible event line.
- * The presentation of this is unaffected by the other values in this
- * struct.
- */
-char *ass;
-
-int flags;
-} AVSubtitleRect;
-
-typedef struct AVSubtitle {
-uint16_t format; /* 0 = graphics */
-uint32_t start_display_time; /* relative to packet pts, in ms */
-uint32_t end_display_time; /* relative to packet pts, in ms */
-unsigned num_rects;
-AVSubtitleRect **rects;
-int64_t pts;/// Same as packet pts, in AV_TIME_BASE
-} AVSubtitle;
-
/**
* If c is NULL, returns the first registered codec,
* if c is non-NULL, returns the next registered codec after c,
@@ -3652,12 +3596,17 @@ int avcodec_open2(AVCodecContext *avctx, const AVCodec
*codec, AVDictionary **op
*/
int avcodec_close(AVCodecContext *avctx);
+#if FF_API_AVSUBTITLE
/**
* Free all allocated data in the given subtitle struct.
+ * @deprecated use av_subtitle_*() functions (the immediate equivalent of
+ * avsubtitle_free() is av_subtitle_clear())
*
* @param sub AVSubtitle to free.
*/
+attribute_deprecated
void avsubtitle_free(AVSubtitle *sub);
+#endif
/**
* @}
diff --git a/libavcodec/utils.c b/libavcodec/utils.c
index db79b67..dc0c1ec 100644
--- a/libavcodec/utils.c
+++ b/libavcodec/utils.c
@@ -39,6 +39,7 @@
#include libavutil/pixdesc.h
#include libavutil/imgutils.h
#include libavutil/samplefmt.h
+#include libavutil/subtitle.h
#include libavutil/dict.h
#include avcodec.h
#include libavutil/opt.h
@@ -1270,12 +1271,6 @@ int av_codec_get_max_lowres(const AVCodec *codec)
return codec-max_lowres;
}
-static void get_subtitle_defaults(AVSubtitle *sub)
-{
-memset(sub, 0, sizeof(*sub));
-sub-pts = AV_NOPTS_VALUE;
-}
-
static int get_bit_rate(AVCodecContext *ctx)
{
int bit_rate;
@@ -2706,7 +2701,7 @@ int avcodec_decode_subtitle2(AVCodecContext *avctx,
AVSubtitle *sub,
}
*got_sub_ptr = 0;
-get_subtitle_defaults(sub);
+av_subtitle_get_defaults(sub);
if ((avctx-codec-capabilities CODEC_CAP_DELAY) || avpkt-size) {
