[FFmpeg-devel] [PATCHv2] avcodec/nvenc: Reconfigure resolution on-the-fly

[Mark Thompson]

On 13/03/2019 01:01, Oliver Collyer wrote:
>> On 12 Mar 2019, at 23:40, Mark Thompson <sw at jkqxz.net> wrote:
>>
>>> On 08/03/2019 07:44, Oliver Collyer wrote:
>>> From c704e3535f866d9f89535b9df59db9ca9811bcf9 Mon Sep 17 00:00:00 2001
>>> From: Oliver Collyer <ovcollyer at mac.com>
>>> Date: Fri, 8 Mar 2019 07:42:41 +0000
>>> Subject: [PATCH 1/1] avcodec/nvenc: Reconfigure resolution on-the-fly
>>>
>>> ---
>>> libavcodec/nvenc.c      | 31 ++++++++++++++++++++++++++++---
>>> libavcodec/nvenc.h      |  3 +++
>>> libavcodec/nvenc_h264.c |  4 ++++
>>> libavcodec/nvenc_hevc.c |  4 ++++
>>> 4 files changed, 39 insertions(+), 3 deletions(-)
>>
>> Can you explain the actual intended use-case for this?
>>
>> The current way to handle resolution changes (or any other stream change of similar magnitude, like a pixel format change) is to flush the existing encoder and then make a new one with the new parameters.  Even ignoring the trivial benefit that all encoders handle this with no additional code, it has the significant advantage that all of the stream metadata, including parameter sets, can be handled correctly.  The handling here does rather badly at this - stream metadata will be misleading, and if you take one of these streams and try to put it into some container with global headers you may well end up with a quite broken file.
>>
> 
> I’m not sure I follow; your logic seems contradictory here - clearly if you are trying to do this in a stream with global headers you’re going to run into trouble, but during writing to such a stream whether you 1) flush, delete and re-create, or 2) reconfigure the encoder is going to have the same effect iand won’t change anything since it’s not the encoder writing any such global stream headers it’s the muxer? Or did you mean something else?

Yeah, that wasn't very clear, sorry.  The main point I'm trying to make on the stream metadata is that if you flush and re-create then all of that /is/ available to the user.  They can of course then throw it all away to get back to the state you have if they wish, but there was at least the option to make use of it (e.g. if new global headers can be handled, maybe by starting a new fragment).

I guess it's not clear to me that adding this new external interface for the special case is really a good trade-off.

> I’m using it in a server app where I want to quickly and efficiently change the video size/bitrate of a transport stream sent over long distance either when the remote user requests or in response to changing network conditions, with as little disruption to the viewing experience as possible.

Does the stop-start actually take longer?  Hardware encoders generally try to minimise the amount of state they need to initialise and carry (because TDM), so recreating it should not be a costly operation.  (Of course, I have no idea what proprietary blobs will do.)

> For the record when this patch is used in conjunction with encoding into an mpegts stream it plays fine in VLC/libVLC, which defects the video changes in the stream and recreates the vout/resizes the video accordingly.

Yes, I can see that it should be fine in MPEG-TS.

>> Given that, I think there should be some justification for why you might want to do this rather than following the existing approach.  Some mention in the API (avcodec.h) to explain what's going on might be a good idea, since that is currently assuming that parameters like width/height are fixed and must be set before opening the encoder.  Relatedly, if there isn't any support for retrieving new metadata then it should probably display a big warning if the user tries to make a stream like this with global headers, since the global headers provided to the user on startup won't actually work for the whole stream.
>>
> 
> I think the fact this functionality is only accessible programmatically means that the bar would be quite high, ie the user likely knows what they are doing, but I can certainly put a comment next to the width/height avcodecctx members along the lines of “In some limited scenarios such as when using nvenc the width/height can be changed during encoding and will be detected by the encoder causing it to reconfigure itself to the new picture sIze. Extreme care should be taken when doing this with a format that uses global headers as the global headers will no longer correspond to the adjusted picture size!”

Yeah, that's probably more sensible than what I suggested.

> Alternatively, maybe this is all a bit too obscure and it’s better left in my own customised ffmpeg branch? That would be quite ok, although the code does already handle dynamic bitrate and DAR changes so I figured to offer you the patch...

Maybe.  If your use-case ends up genuinely better somehow then I think it probably would be sensible to include (and maybe investigate doing it in other cases so that nvenc isn't a surprising special-case here), but adding complexity to the library interface to save a little bit of code on the user side in one case seems a bit excessive.

Thanks,

- Mark