Re: [FFmpeg-devel] [PATCHv3] avcodec/nvenc: Reconfigure resolution on-the-fly
So what's the final verdict here, can this be pushed or not? Timo - did you manage to test it over last weekend? I haven't found the time, sorry. I'm generally not opposed to this. It does not disrupt normal use, and spinning up nvenc does have a surprisingly hefty overhead, so it makes sense to have features like that. One request I'd have for the patch though, if one of the new max_ parameters is set, and dyn res changing is not supported, there should be an error. Likewise if the res does change and max_width/height were not set. Silently not doing anything seems bad. Also, someone needs to ack the non-nvenc changes. Not sure if that explanation is really needed, since this is an nvenc specific edge case. smime.p7s Description: S/MIME Cryptographic Signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCHv3] avcodec/nvenc: Reconfigure resolution on-the-fly
> >>> >>> >>> 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? >> >> 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. >> >> 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. >> >>> 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!” >> >> 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... >> > > Updated patch with added comment following Mark's feedback. > > Regards > > Oliver > So what's the final verdict here, can this be pushed or not? Timo - did you manage to test it over last weekend? Regards Oliver 0001-avcodec-nvenc-Reconfigure-resolution-on-the-fly.patch Description: Binary data ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCHv3] avcodec/nvenc: Reconfigure resolution on-the-fly
>> >> >> 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? > > 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. > > 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. > >> 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!” > > 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... > Updated patch with added comment following Mark's feedback. Regards Oliver 0001-avcodec-nvenc-Reconfigure-resolution-on-the-fly.patch Description: Binary data ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel