diff mbox

[FFmpeg-devel,2/2] libx264: Pass the reordered_opaque field through the encoder

Message ID 20181025125858.31807-2-martin@martin.st
State Accepted
Headers show

Commit Message

Martin Storsjö Oct. 25, 2018, 12:58 p.m. UTC 
libx264 does have a field for opaque data to pass along with frames
through the encoder, but it is a pointer, while the libavcodec
reordered_opaque field is an int64_t. Therefore, allocate an array
within the libx264 wrapper, where reordered_opaque values in flight
are stored, and pass a pointer to this array to libx264.

Update the public libavcodec documentation for the AVCodecContext
field to explain this usage, and add a codec capability that allows
detecting whether an encoder handles this field.
---
 libavcodec/avcodec.h | 12 +++++++++++-
 libavcodec/libx264.c | 33 ++++++++++++++++++++++++++++++---
 libavcodec/version.h |  4 ++--
 3 files changed, 43 insertions(+), 6 deletions(-)

Comments

Derek Buitenhuis Oct. 29, 2018, 8:37 p.m. UTC | #1 
On 25/10/2018 13:58, Martin Storsjö wrote:
> +    x4->nb_reordered_opaque = x264_encoder_maximum_delayed_frames(x4->enc) + 1;

Is it possible this changes when the encoder is reconfigured (e.g. to interlaced)?

- Derek
Martin Storsjö Oct. 29, 2018, 9:06 p.m. UTC | #2 
On Mon, 29 Oct 2018, Derek Buitenhuis wrote:

> On 25/10/2018 13:58, Martin Storsjö wrote:
>> +    x4->nb_reordered_opaque = x264_encoder_maximum_delayed_frames(x4->enc) + 1;
>
> Is it possible this changes when the encoder is reconfigured (e.g. to interlaced)?

Good point. I'm sure it's possible that it changes, if reconfiguring.

As I guess there can be old frames in flight, the only safe option is to 
enlarge, not to shrink it. But in case a realloc moves the array, the old 
pointers end up pretty useless.

Tricky, I guess I'll have to think about it to see if I can come up with 
something which isn't completely terrible.

// Martin
Derek Buitenhuis Oct. 30, 2018, 4:39 p.m. UTC | #3 
On 29/10/2018 21:06, Martin Storsjö wrote:
> As I guess there can be old frames in flight, the only safe option is to
> enlarge, not to shrink it. But in case a realloc moves the array, the old
> pointers end up pretty useless.

Just always allocate the max (which is known for H.264), and adjust nb_reordered_opaque
as need be, on reconfig, no?

- Derek
Martin Storsjö Oct. 30, 2018, 7:49 p.m. UTC | #4 
On Tue, 30 Oct 2018, Derek Buitenhuis wrote:

> On 29/10/2018 21:06, Martin Storsjö wrote:
>> As I guess there can be old frames in flight, the only safe option is to
>> enlarge, not to shrink it. But in case a realloc moves the array, the old
>> pointers end up pretty useless.
>
> Just always allocate the max (which is known for H.264), and adjust nb_reordered_opaque
> as need be, on reconfig, no?

Hmm, that might make sense, but with a little twist. The max reordered 
frames for H.264 is known, but onto that you also get more delay due to 
frame threads and other details that this function within x264 knows 
about. So that would make it [H264 max reordering] + [threads] + 
[constant] or something such?

// Martin
Derek Buitenhuis Oct. 31, 2018, 2:04 p.m. UTC | #5 
On 30/10/2018 19:49, Martin Storsjö wrote:
> Hmm, that might make sense, but with a little twist. The max reordered
> frames for H.264 is known, but onto that you also get more delay due to
> frame threads and other details that this function within x264 knows
> about. So that would make it [H264 max reordering] + [threads] +
> [constant] or something such?

Looking at the source, it's more complicated than that, with e.g.:

     h->frames.i_delay = X264_MAX( h->frames.i_delay, h->param.rc.i_lookahead );

I think you're better off not trying to duplicate this logic.

- Derek
Martin Storsjö Oct. 31, 2018, 9:41 p.m. UTC | #6 
On Wed, 31 Oct 2018, Derek Buitenhuis wrote:

> On 30/10/2018 19:49, Martin Storsjö wrote:
>> Hmm, that might make sense, but with a little twist. The max reordered
>> frames for H.264 is known, but onto that you also get more delay due to
>> frame threads and other details that this function within x264 knows
>> about. So that would make it [H264 max reordering] + [threads] +
>> [constant] or something such?
>
> Looking at the source, it's more complicated than that, with e.g.:
>
>     h->frames.i_delay = X264_MAX( h->frames.i_delay, h->param.rc.i_lookahead );
>
> I think you're better off not trying to duplicate this logic.

Indeed, I don't want to duplicate that.

Even though we do allow reconfiguration, it doesn't look like we support 
changing any parameters which would actually affect the delay, only RC 
method and targets (CRF, bitrate, etc). So given that, the current patch 
probably should be safe - what do you think? Or the current patch, with an 
added margin of 16 on top just in case?

// Martin
Derek Buitenhuis Nov. 1, 2018, noon UTC | #7 
On 31/10/2018 21:41, Martin Storsjö wrote:
> Even though we do allow reconfiguration, it doesn't look like we support
> changing any parameters which would actually affect the delay, only RC
> method and targets (CRF, bitrate, etc). So given that, the current patch
> probably should be safe - what do you think? Or the current patch, with an
> added margin of 16 on top just in case?

We allow reconfiguring to/from interlaced. I'm not sure if this can modify delay?

Othwewise, probably safe, I guess.

- Derek
Martin Storsjö Nov. 1, 2018, 12:37 p.m. UTC | #8 
On Thu, 1 Nov 2018, Derek Buitenhuis wrote:

> On 31/10/2018 21:41, Martin Storsjö wrote:
>> Even though we do allow reconfiguration, it doesn't look like we support
>> changing any parameters which would actually affect the delay, only RC
>> method and targets (CRF, bitrate, etc). So given that, the current patch
>> probably should be safe - what do you think? Or the current patch, with an
>> added margin of 16 on top just in case?
>
> We allow reconfiguring to/from interlaced. I'm not sure if this can modify delay?

Not really sure either... So perhaps it'd be safest with some bit of extra 
margin/overestimate of the delay here? It just costs a couple bytes in the 
mapping array anyway.

// Martin
Martin Storsjö Nov. 5, 2018, 2:05 p.m. UTC | #9 
On Thu, 1 Nov 2018, Martin Storsjö wrote:

> On Thu, 1 Nov 2018, Derek Buitenhuis wrote:
>
>> On 31/10/2018 21:41, Martin Storsjö wrote:
>>> Even though we do allow reconfiguration, it doesn't look like we support
>>> changing any parameters which would actually affect the delay, only RC
>>> method and targets (CRF, bitrate, etc). So given that, the current patch
>>> probably should be safe - what do you think? Or the current patch, with an
>>> added margin of 16 on top just in case?
>>
>> We allow reconfiguring to/from interlaced. I'm not sure if this can modify 
> delay?
>
> Not really sure either... So perhaps it'd be safest with some bit of extra 
> margin/overestimate of the delay here? It just costs a couple bytes in the 
> mapping array anyway.

Pushed, with a bit over overestimation of the buffer size, just in case.

// Martin
diff mbox

Patch

diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
index 705a3ce4f3..69c5a82757 100644
--- a/libavcodec/avcodec.h
+++ b/libavcodec/avcodec.h
@@ -1070,6 +1070,13 @@  typedef struct RcOverride{
  */
 #define AV_CODEC_CAP_HYBRID              (1 << 19)
 
+/**
+ * This codec takes the reordered_opaque field from input AVFrames
+ * and returns it in the corresponding field in AVCodecContext after
+ * encoding.
+ */
+#define AV_CODEC_CAP_ENCODER_REORDERED_OPAQUE (1 << 20)
+
 /**
  * Pan Scan area.
  * This specifies the area which should be displayed.
@@ -2676,7 +2683,10 @@  typedef struct AVCodecContext {
     /**
      * opaque 64-bit number (generally a PTS) that will be reordered and
      * output in AVFrame.reordered_opaque
-     * - encoding: unused
+     * - encoding: Set by libavcodec to the reordered_opaque of the input
+     *             frame corresponding to the last returned packet. Only
+     *             supported by encoders with the
+     *             AV_CODEC_CAP_ENCODER_REORDERED_OPAQUE capability.
      * - decoding: Set by user.
      */
     int64_t reordered_opaque;
diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
index d6367bf557..13c86a9451 100644
--- a/libavcodec/libx264.c
+++ b/libavcodec/libx264.c
@@ -92,6 +92,9 @@  typedef struct X264Context {
     int noise_reduction;
 
     char *x264_params;
+
+    int nb_reordered_opaque, next_reordered_opaque;
+    int64_t *reordered_opaque;
 } X264Context;
 
 static void X264_log(void *p, int level, const char *fmt, va_list args)
@@ -278,6 +281,7 @@  static int X264_frame(AVCodecContext *ctx, AVPacket *pkt, const AVFrame *frame,
     int nnal, i, ret;
     x264_picture_t pic_out = {0};
     int pict_type;
+    int64_t *out_opaque;
 
     x264_picture_init( &x4->pic );
     x4->pic.img.i_csp   = x4->params.i_csp;
@@ -297,6 +301,11 @@  static int X264_frame(AVCodecContext *ctx, AVPacket *pkt, const AVFrame *frame,
 
         x4->pic.i_pts  = frame->pts;
 
+        x4->reordered_opaque[x4->next_reordered_opaque] = frame->reordered_opaque;
+        x4->pic.opaque = &x4->reordered_opaque[x4->next_reordered_opaque];
+        x4->next_reordered_opaque++;
+        x4->next_reordered_opaque %= x4->nb_reordered_opaque;
+
         switch (frame->pict_type) {
         case AV_PICTURE_TYPE_I:
             x4->pic.i_type = x4->forced_idr > 0 ? X264_TYPE_IDR
@@ -350,6 +359,14 @@  static int X264_frame(AVCodecContext *ctx, AVPacket *pkt, const AVFrame *frame,
     pkt->pts = pic_out.i_pts;
     pkt->dts = pic_out.i_dts;
 
+    out_opaque = pic_out.opaque;
+    if (out_opaque >= x4->reordered_opaque &&
+        out_opaque < &x4->reordered_opaque[x4->nb_reordered_opaque]) {
+        ctx->reordered_opaque = *out_opaque;
+    } else {
+        // Unexpected opaque pointer on picture output
+        ctx->reordered_opaque = 0;
+    }
 
     switch (pic_out.i_type) {
     case X264_TYPE_IDR:
@@ -393,6 +410,7 @@  static av_cold int X264_close(AVCodecContext *avctx)
 
     av_freep(&avctx->extradata);
     av_freep(&x4->sei);
+    av_freep(&x4->reordered_opaque);
 
     if (x4->enc) {
         x264_encoder_close(x4->enc);
@@ -846,6 +864,12 @@  FF_ENABLE_DEPRECATION_WARNINGS
     cpb_props->max_bitrate = x4->params.rc.i_vbv_max_bitrate * 1000;
     cpb_props->avg_bitrate = x4->params.rc.i_bitrate         * 1000;
 
+    x4->nb_reordered_opaque = x264_encoder_maximum_delayed_frames(x4->enc) + 1;
+    x4->reordered_opaque    = av_malloc_array(x4->nb_reordered_opaque,
+                                              sizeof(*x4->reordered_opaque));
+    if (!x4->reordered_opaque)
+        return AVERROR(ENOMEM);
+
     return 0;
 }
 
@@ -1059,7 +1083,8 @@  AVCodec ff_libx264_encoder = {
     .init             = X264_init,
     .encode2          = X264_frame,
     .close            = X264_close,
-    .capabilities     = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_AUTO_THREADS,
+    .capabilities     = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_AUTO_THREADS |
+                        AV_CODEC_CAP_ENCODER_REORDERED_OPAQUE,
     .priv_class       = &x264_class,
     .defaults         = x264_defaults,
     .init_static_data = X264_init_static,
@@ -1085,7 +1110,8 @@  AVCodec ff_libx264rgb_encoder = {
     .init           = X264_init,
     .encode2        = X264_frame,
     .close          = X264_close,
-    .capabilities   = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_AUTO_THREADS,
+    .capabilities   = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_AUTO_THREADS |
+                      AV_CODEC_CAP_ENCODER_REORDERED_OPAQUE,
     .priv_class     = &rgbclass,
     .defaults       = x264_defaults,
     .pix_fmts       = pix_fmts_8bit_rgb,
@@ -1110,7 +1136,8 @@  AVCodec ff_libx262_encoder = {
     .init             = X264_init,
     .encode2          = X264_frame,
     .close            = X264_close,
-    .capabilities     = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_AUTO_THREADS,
+    .capabilities     = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_AUTO_THREADS |
+                        AV_CODEC_CAP_ENCODER_REORDERED_OPAQUE,
     .priv_class       = &X262_class,
     .defaults         = x264_defaults,
     .pix_fmts         = pix_fmts_8bit,
diff --git a/libavcodec/version.h b/libavcodec/version.h
index 9098882f47..91809641b4 100644
--- a/libavcodec/version.h
+++ b/libavcodec/version.h
@@ -28,8 +28,8 @@ 
 #include "libavutil/version.h"
 
 #define LIBAVCODEC_VERSION_MAJOR  58
-#define LIBAVCODEC_VERSION_MINOR  33
-#define LIBAVCODEC_VERSION_MICRO 102
+#define LIBAVCODEC_VERSION_MINOR  34
+#define LIBAVCODEC_VERSION_MICRO 100
 
 #define LIBAVCODEC_VERSION_INT  AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
                                                LIBAVCODEC_VERSION_MINOR, \