[FFmpeg-devel] [PATCH v3] libavcodec/utils: add avcodec_encode_subtitle2 and mark subtitle packets with FLAG_KEY

[Clément Bœsch]

On Tue, May 27, 2014 at 05:21:49PM -0700, Aman Gupta wrote:
> Took a stab at implementing avcodec_encode_subtitle2 which takes an AVPacket argument
> and sets AV_PKT_FLAG_KEY on the generated packets.
> 
> Signed-off-by: Aman Gupta <ffmpeg at tmm1.net>
> ---
>  configure            |  1 +
>  ffmpeg.c             | 30 +++++++++++++-----------------
>  libavcodec/avcodec.h |  1 +
>  libavcodec/utils.c   | 17 +++++++++++++++++
>  4 files changed, 32 insertions(+), 17 deletions(-)
> 
> diff --git a/configure b/configure
[...]
> +int avcodec_encode_subtitle2(AVCodecContext *avctx, AVPacket *avpkt, const AVSubtitle *sub)
> +{
> +    int ret;
> +    int subtitle_out_max_size = 1024 * 1024;
> +    uint8_t *buf = av_malloc(subtitle_out_max_size);
> +
> +    ret = avcodec_encode_subtitle(avctx, buf, subtitle_out_max_size, sub);
> +    if (ret < 0)
> +        return ret;
> +
> +    avpkt->flags = AV_PKT_FLAG_KEY;
> +    avpkt->data = buf;
> +    avpkt->size = ret;
> +
> +    return ret;
> +}
> +

Thanks for the proposition but I definitely think this need more thinking.
Adding a new encoding function just for a single flag but keeping all the
current problems of the sub encoding API is not a good idea.

First, you're now allocating a buffer for every subtitle. This doesn't
sound good, it's a regression. We probably need some kind of AVBufferRef
somehow that grows when necessary.

Second, I'd say that subtitles encoders should be responsible for the
allocation size they need (contrary to a/v we can't estimate the size in
advance). This require changes in the encoders API and their usage.

Third, the AVSubtitle should be moved and/or changed in libavutil. This is
hard, but it needs to be done at some point. This new structure would be
able to fix all the limitations we currently have, such as:

 - present in libavutil so it can be used in libavfilter
 - common markup for text subtitles (instead of the weird ASS hack)
 - flexible enough to support all kind of crazy bitmap subs
 - be associated with frames for closed captions?
 - being able to deal with subtitles replacing parts of the previous
   events, such as what we have with dvb subs

Anyway, if you're willing to improve that subtitles API that is extremely
welcome, but I believe this patch is more harmful than helpful since it
pollutes the namespace with a near-to-useless hack improvement.

-- 
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140604/0e53ad92/attachment.asc>