[PATCH] Added support for MB_INFO
Hi, patch attached to add support for passing down to libx264 information about which macroblock could be eligible for being coded as P_SKIP. NICE SRL, viale Monte Grappa 3/5, 20124 Milano, Italia, Registro delle Imprese di Milano Monza Brianza Lodi REA n. 2096882, Capitale Sociale: 10.329,14 EUR i.v., Cod. Fisc. e P.IVA 01133050052, Societa con Socio Unico
Hi all, any chance someone could possibly have a look at this patch, please? Thanks in advance On Fri, 2022-06-10 at 10:11 +0000, Carotti, Elias wrote:
Hi, patch attached to add support for passing down to libx264 information about which macroblock could be eligible for being coded as P_SKIP.
NICE SRL, viale Monte Grappa 3/5, 20124 Milano, Italia, Registro delle Imprese di Milano Monza Brianza Lodi REA n. 2096882, Capitale Sociale: 10.329,14 EUR i.v., Cod. Fisc. e P.IVA 01133050052, Societa con Socio Unico
On 17.06.2022 10:41, Carotti, Elias wrote:
Hi all, any chance someone could possibly have a look at this patch, please? Thanks in advance
On Fri, 2022-06-10 at 10:11 +0000, Carotti, Elias wrote:
Hi, patch attached to add support for passing down to libx264 information about which macroblock could be eligible for being coded as P_SKIP.
NICE SRL, viale Monte Grappa 3/5, 20124 Milano, Italia, Registro delle Imprese di Milano Monza Brianza Lodi REA n. 2096882, Capitale Sociale: 10.329,14 EUR i.v., Cod. Fisc. e P.IVA 01133050052, Societa con Socio Unico
There's a stray printf in the patch. Also, what if a frame with such side data is passed to not-x264? Won't it leak the memory? From the looks if it, this entirely relies on x264 to free it.
Hi, thanks for pointing out the printf. That's a left over which I removed. I am not clear on the possible leak you are hinting at. The new side information only passes two pointers to libx264, the first one being a buffer with the flags and a pointer to a deallocator which may be NULL. If the deallocator is not NULL libx264 we're yielding the deallocation responsibility to libx264, otherwise the ownership of the buffer and, as such, the responsibility for the deallocation remains with the caller. As such, the only possibility for a leak seems to me due to a programming error. Is that what you were asking or am I missing something else? Please find attached the updated patch. Elias On Fri, 2022-06-17 at 11:40 +0200, Timo Rothenpieler wrote:
CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
On 17.06.2022 10:41, Carotti, Elias wrote:
Hi all, any chance someone could possibly have a look at this patch, please? Thanks in advance
On Fri, 2022-06-10 at 10:11 +0000, Carotti, Elias wrote:
Hi, patch attached to add support for passing down to libx264 information about which macroblock could be eligible for being coded as P_SKIP.
NICE SRL, viale Monte Grappa 3/5, 20124 Milano, Italia, Registro delle Imprese di Milano Monza Brianza Lodi REA n. 2096882, Capitale Sociale: 10.329,14 EUR i.v., Cod. Fisc. e P.IVA 01133050052, Societa con Socio Unico
There's a stray printf in the patch.
Also, what if a frame with such side data is passed to not-x264? Won't it leak the memory? From the looks if it, this entirely relies on x264 to free it. _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
NICE SRL, viale Monte Grappa 3/5, 20124 Milano, Italia, Registro delle Imprese di Milano Monza Brianza Lodi REA n. 2096882, Capitale Sociale: 10.329,14 EUR i.v., Cod. Fisc. e P.IVA 01133050052, Societa con Socio Unico
On 17.06.2022 12:15, Carotti, Elias wrote:
Hi, thanks for pointing out the printf. That's a left over which I removed.
I am not clear on the possible leak you are hinting at. The new side information only passes two pointers to libx264, the first one being a buffer with the flags and a pointer to a deallocator which may be NULL.
If the deallocator is not NULL libx264 we're yielding the deallocation responsibility to libx264, otherwise the ownership of the buffer and, as such, the responsibility for the deallocation remains with the caller. As such, the only possibility for a leak seems to me due to a programming error. Is that what you were asking or am I missing something else?
Please find attached the updated patch.
Elias
Yes, exactly. It relies on x264 to free it. What happens if x264 is not involved, but some other encoder, which does not check for that kind of side data? How does the caller know that the data was consumed, and the ownership passed on? The only sane way would be for the caller to hand over the ownership to ffmpeg, which then takes care of making sure it gets freed.
Yes, exactly. It relies on x264 to free it.
Not really. It can rely on x264 if you explicitly want that behavior. If you do not set a deallocator, it remains the caller responsibility.
What happens if x264 is not involved, but some other encoder, which does not check for that kind of side data?
How does the caller know that the data was consumed, and the ownership passed on?
Again, you don't have to pass the ownership, and in fact in my use case I do not pass it since I actually recycle and update the same buffer for subsequent frames. If you do intentionally pass the ownership you need to be aware of what you are doing. As I said, I see a leak only in case of a programming error. Maybe we could explicitly state it in the comment section in mb_info.h: if you set the deallocator you're relinquishing ownership of the buffer. Plus, there's one more flag (b_mb_info_update) in libx264 which allows to pass back information to the caller about which macroblocks among the flagged ones were actually encoded as P_SKIP. I did not add support for that though but in the future somebody may want to. In principle I could've put the buffer into the side information and not just pass a pointer to it but I thought that it would require adding more semantics which would imply a stronger dependency on libx264. Right now, mb_info is a vector of uint8_t flags and the only possible value - to date - is X264_MBINFO_CONSTANT. What if, say, one day libx264 decides the buffer has to be a vector of uint16_t or still uint8_t but the flags are packed? On the other hand, this, AFAIK, is only supported by libx264. Other codecs may want to choose a different semantic for a similar field and the only possibility to make it generic is letting the caller handling the low level details.
The only sane way would be for the caller to hand over the ownership to ffmpeg, which then takes care of making sure it gets freed.
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
NICE SRL, viale Monte Grappa 3/5, 20124 Milano, Italia, Registro delle Imprese di Milano Monza Brianza Lodi REA n. 2096882, Capitale Sociale: 10.329,14 EUR i.v., Cod. Fisc. e P.IVA 01133050052, Societa con Socio Unico
On 17.06.2022 12:59, Carotti, Elias wrote:
Yes, exactly. It relies on x264 to free it.
Not really. It can rely on x264 if you explicitly want that behavior. If you do not set a deallocator, it remains the caller responsibility.
What happens if x264 is not involved, but some other encoder, which does not check for that kind of side data?
How does the caller know that the data was consumed, and the ownership passed on?
Again, you don't have to pass the ownership, and in fact in my use case I do not pass it since I actually recycle and update the same buffer for subsequent frames. If you do intentionally pass the ownership you need to be aware of what you are doing. As I said, I see a leak only in case of a programming error. Maybe we could explicitly state it in the comment section in mb_info.h: if you set the deallocator you're relinquishing ownership of the buffer.
I'm not sure if that's something you can sensibly do with side data? What if it gets buffered, copied, and so on?
Plus, there's one more flag (b_mb_info_update) in libx264 which allows to pass back information to the caller about which macroblocks among the flagged ones were actually encoded as P_SKIP. I did not add support for that though but in the future somebody may want to.
Yes, it's very x264 specific. But the side data is generic. If for some reason x264 does not process a frame, or any other encoder ends up getting used, the data will leak if it relied on x264 to free it.
In principle I could've put the buffer into the side information and not just pass a pointer to it but I thought that it would require adding more semantics which would imply a stronger dependency on libx264. Right now, mb_info is a vector of uint8_t flags and the only possible value - to date - is X264_MBINFO_CONSTANT. What if, say, one day libx264 decides the buffer has to be a vector of uint16_t or still uint8_t but the flags are packed? On the other hand, this, AFAIK, is only supported by libx264. Other codecs may want to choose a different semantic for a similar field and the only possibility to make it generic is letting the caller handling the low level details.
I'm not aware of any other side data with a similar semantic. And I'm really not sure if it's sane or even valid to do it like that. Can't the data be entirely contained within the side data?
On Fri, 2022-06-17 at 17:10 +0200, Timo Rothenpieler wrote:
CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
On 17.06.2022 12:59, Carotti, Elias wrote:
Yes, exactly. It relies on x264 to free it.
Not really. It can rely on x264 if you explicitly want that behavior. If you do not set a deallocator, it remains the caller responsibility.
What happens if x264 is not involved, but some other encoder, which does not check for that kind of side data?
How does the caller know that the data was consumed, and the ownership passed on?
Again, you don't have to pass the ownership, and in fact in my use case I do not pass it since I actually recycle and update the same buffer for subsequent frames. If you do intentionally pass the ownership you need to be aware of what you are doing. As I said, I see a leak only in case of a programming error. Maybe we could explicitly state it in the comment section in mb_info.h: if you set the deallocator you're relinquishing ownership of the buffer.
I'm not sure if that's something you can sensibly do with side data? What if it gets buffered, copied, and so on?
Plus, there's one more flag (b_mb_info_update) in libx264 which allows to pass back information to the caller about which macroblocks among the flagged ones were actually encoded as P_SKIP. I did not add support for that though but in the future somebody may want to.
Yes, it's very x264 specific. But the side data is generic. If for some reason x264 does not process a frame, or any other encoder ends up getting used, the data will leak if it relied on x264 to free it.
Honestly, I do not see how this could ever happen. If you don't use libx264 you're likely not be using this kind of side data either. If you use libx264 it's up to you to decide whether you want to reliquish control over it to libx264 or not. Otherwise, a solution perfectly compatible with what you suggest is to get rid of the deallocator field altogether which would make it clear it's the caller's responsibility to free the data. I just see it like the frame data or any other data you're passing to the encoder. You just pass the frame data to the library and if you don't free the memory it's not the library fault, right?
In principle I could've put the buffer into the side information and not just pass a pointer to it but I thought that it would require adding more semantics which would imply a stronger dependency on libx264. Right now, mb_info is a vector of uint8_t flags and the only possible value - to date - is X264_MBINFO_CONSTANT. What if, say, one day libx264 decides the buffer has to be a vector of uint16_t or still uint8_t but the flags are packed? On the other hand, this, AFAIK, is only supported by libx264. Other codecs may want to choose a different semantic for a similar field and the only possibility to make it generic is letting the caller handling the low level details.
I'm not aware of any other side data with a similar semantic. And I'm really not sure if it's sane or even valid to do it like that. Can't the data be entirely contained within the side data?
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
NICE SRL, viale Monte Grappa 3/5, 20124 Milano, Italia, Registro delle Imprese di Milano Monza Brianza Lodi REA n. 2096882, Capitale Sociale: 10.329,14 EUR i.v., Cod. Fisc. e P.IVA 01133050052, Societa con Socio Unico
On date Friday 2022-06-17 17:10:25 +0200, Timo Rothenpieler wrote:
On 17.06.2022 12:59, Carotti, Elias wrote: [...]
Again, you don't have to pass the ownership, and in fact in my use case I do not pass it since I actually recycle and update the same buffer for subsequent frames. If you do intentionally pass the ownership you need to be aware of what you are doing. As I said, I see a leak only in case of a programming error. Maybe we could explicitly state it in the comment section in mb_info.h: if you set the deallocator you're relinquishing ownership of the buffer.
I'm not sure if that's something you can sensibly do with side data? What if it gets buffered, copied, and so on?
This semantics is specific to libx264, but can be possibly extended to other encoders. AFAIU libx264 keeps the data in memory until it needs it, and it finally calls the free when it is finished with the data. I think the concerns are about: 1. How does the consumer (libx264 or another encoder) knows that the data is still valid? This is a problem of the application injecting the data, it might use reference counting, the deallocator in this case will unref the data on the user application. 2. How does the application knows that the data is still in use by the consumer (the encoder)? It doesn't have to, since this is handled by the consumer, when it is finished it will call the deallocator.
Plus, there's one more flag (b_mb_info_update) in libx264 which allows to pass back information to the caller about which macroblocks among the flagged ones were actually encoded as P_SKIP. I did not add support for that though but in the future somebody may want to.
Yes, it's very x264 specific. But the side data is generic. If for some reason x264 does not process a frame, or any other encoder ends up getting used, the data will leak if it relied on x264 to free it.
So, let's make up a use case for this. The application injects the data, it might be for example a filter computing the MB information and it might be unaware of what encoder is used down in the processing pipeline. For the simplest case the application knows that libx264 is the used encoder, and assumes that all data will be consumed by libx264. For the x264 API the problem does not exist, with generic side data the problem can raise since side data must be agnostic of its final destination. So I agree, for the generic use case we can have a leak. I see some possible ways to solve this: 1. We document the limitation and assume that when the MB metadata is injected down in the pipeline there is a single consumer, the application will leak otherwise. This works for the simple application -> libx264 pipeline, but it can't work for a more generic case. 2. We provide a mechanism *at the library level* to cleanup the unconsumed data. libavcodec sees that the encoder does not handle that kind of user data (it might be a capability flag) and automatically cleanups the data in that case. This would be still underkill though, since the AVFrame is not necessarily injested into an encoder (for example the MB info could be rendered or processed by an application or by a filter). This means that the AVFrame unref needs to call this side data deallocator (and that the consumer needs to "steal" the data from the AVFrame before it is deallocated by the AVFrame unref). But then what happens in case the AVFrame is shared by multiple endpoints (e.g. multiple encoders)? Then you need a ref count for the side data itself. I don't know if this is something which is feasible with the current side data API, and if it could be extended to support this kind of semantics. 3. The simpler approach, we copy the data into MB info and deallocate when we free it, but this might have a performance penalty because of the unneeded memcpy.
In principle I could've put the buffer into the side information and not just pass a pointer to it but I thought that it would require adding more semantics which would imply a stronger dependency on libx264. Right now, mb_info is a vector of uint8_t flags and the only possible value - to date - is X264_MBINFO_CONSTANT. What if, say, one day libx264 decides the buffer has to be a vector of uint16_t or still uint8_t but the flags are packed? On the other hand, this, AFAIK, is only supported by libx264. Other codecs may want to choose a different semantic for a similar field and the only possibility to make it generic is letting the caller handling the low level details.
I'm not aware of any other side data with a similar semantic. And I'm really not sure if it's sane or even valid to do it like that. Can't the data be entirely contained within the side data?
We can assume the same semantics of libx264 (at the moment the only use case for this).
Sorry to revive an old thread, but I updated the patch for ffmpeg 6 and this new patch should address the comments. Still this is a libx264-only patch, and provides a means to specify that only portions of the frame have changed from the previous one while the others should be P_SKIP-ped. More specifically, now the data is fully contained in the side data and the actual map of macroblocks which changed from the previous frame is created from the side information at encoding time, much like what is done for the region of interest. best, Elias On Fri, 2022-06-17 at 17:10 +0200, Timo Rothenpieler wrote:
CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
On 17.06.2022 12:59, Carotti, Elias wrote:
Yes, exactly. It relies on x264 to free it.
Not really. It can rely on x264 if you explicitly want that behavior. If you do not set a deallocator, it remains the caller responsibility.
What happens if x264 is not involved, but some other encoder, which does not check for that kind of side data?
How does the caller know that the data was consumed, and the ownership passed on?
Again, you don't have to pass the ownership, and in fact in my use case I do not pass it since I actually recycle and update the same buffer for subsequent frames. If you do intentionally pass the ownership you need to be aware of what you are doing. As I said, I see a leak only in case of a programming error. Maybe we could explicitly state it in the comment section in mb_info.h: if you set the deallocator you're relinquishing ownership of the buffer.
I'm not sure if that's something you can sensibly do with side data? What if it gets buffered, copied, and so on?
Plus, there's one more flag (b_mb_info_update) in libx264 which allows to pass back information to the caller about which macroblocks among the flagged ones were actually encoded as P_SKIP. I did not add support for that though but in the future somebody may want to.
Yes, it's very x264 specific. But the side data is generic. If for some reason x264 does not process a frame, or any other encoder ends up getting used, the data will leak if it relied on x264 to free it.
In principle I could've put the buffer into the side information and not just pass a pointer to it but I thought that it would require adding more semantics which would imply a stronger dependency on libx264. Right now, mb_info is a vector of uint8_t flags and the only possible value - to date - is X264_MBINFO_CONSTANT. What if, say, one day libx264 decides the buffer has to be a vector of uint16_t or still uint8_t but the flags are packed? On the other hand, this, AFAIK, is only supported by libx264. Other codecs may want to choose a different semantic for a similar field and the only possibility to make it generic is letting the caller handling the low level details.
I'm not aware of any other side data with a similar semantic. And I'm really not sure if it's sane or even valid to do it like that. Can't the data be entirely contained within the side data? _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
NICE SRL, viale Monte Grappa 3/5, 20124 Milano, Italia, Registro delle Imprese di Milano Monza Brianza Lodi REA n. 2096882, Capitale Sociale: 10.329,14 EUR i.v., Cod. Fisc. e P.IVA 01133050052, Societa con Socio Unico
Hi all, any chance someone could possibly have a look at this patch, please? Thanks in advance Elias On Wed, 2023-05-03 at 12:27 +0000, Carotti, Elias wrote:
Sorry to revive an old thread, but I updated the patch for ffmpeg 6 and this new patch should address the comments. Still this is a libx264-only patch, and provides a means to specify that only portions of the frame have changed from the previous one while the others should be P_SKIP-ped.
More specifically, now the data is fully contained in the side data and the actual map of macroblocks which changed from the previous frame is created from the side information at encoding time, much like what is done for the region of interest.
best, Elias
On Fri, 2022-06-17 at 17:10 +0200, Timo Rothenpieler wrote:
CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
On 17.06.2022 12:59, Carotti, Elias wrote:
Yes, exactly. It relies on x264 to free it.
Not really. It can rely on x264 if you explicitly want that behavior. If you do not set a deallocator, it remains the caller responsibility.
What happens if x264 is not involved, but some other encoder, which does not check for that kind of side data?
How does the caller know that the data was consumed, and the ownership passed on?
Again, you don't have to pass the ownership, and in fact in my use case I do not pass it since I actually recycle and update the same buffer for subsequent frames. If you do intentionally pass the ownership you need to be aware of what you are doing. As I said, I see a leak only in case of a programming error. Maybe we could explicitly state it in the comment section in mb_info.h: if you set the deallocator you're relinquishing ownership of the buffer.
I'm not sure if that's something you can sensibly do with side data? What if it gets buffered, copied, and so on?
Plus, there's one more flag (b_mb_info_update) in libx264 which allows to pass back information to the caller about which macroblocks among the flagged ones were actually encoded as P_SKIP. I did not add support for that though but in the future somebody may want to.
Yes, it's very x264 specific. But the side data is generic. If for some reason x264 does not process a frame, or any other encoder ends up getting used, the data will leak if it relied on x264 to free it.
In principle I could've put the buffer into the side information and not just pass a pointer to it but I thought that it would require adding more semantics which would imply a stronger dependency on libx264. Right now, mb_info is a vector of uint8_t flags and the only possible value - to date - is X264_MBINFO_CONSTANT. What if, say, one day libx264 decides the buffer has to be a vector of uint16_t or still uint8_t but the flags are packed? On the other hand, this, AFAIK, is only supported by libx264. Other codecs may want to choose a different semantic for a similar field and the only possibility to make it generic is letting the caller handling the low level details.
I'm not aware of any other side data with a similar semantic. And I'm really not sure if it's sane or even valid to do it like that. Can't the data be entirely contained within the side data? _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
NICE SRL, viale Monte Grappa 3/5, 20124 Milano, Italia, Registro delle Imprese di Milano Monza Brianza Lodi REA n. 2096882, Capitale Sociale: 10.329,14 EUR i.v., Cod. Fisc. e P.IVA 01133050052, Societa con Socio Unico
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
NICE SRL, viale Monte Grappa 3/5, 20124 Milano, Italia, Registro delle Imprese di Milano Monza Brianza Lodi REA n. 2096882, Capitale Sociale: 10.329,14 EUR i.v., Cod. Fisc. e P.IVA 01133050052, Societa con Socio Unico
Quoting Carotti, Elias (2022-06-17 10:41:30)
Hi all, any chance someone could possibly have a look at this patch, please? Thanks in advance
This looks like it belongs in AVVideoEncParams. -- Anton Khirnov
Hi, extending AVVideoEncParams was the first hypothesis I made but it didn't seem it was the proper place to add the mb_info flags. I may be wrong but my impression is that AVVideoEncParams is used to carry encoding parameters read from the bitstream at the decoder side while here were going the other direction, i.e., were passing information from the application to the encoder. Secondly, mb_info can't be strictly considered encoding parameters and it's not present in the bitstream at all. It's just a way to give hints to the libx264 encoder on which macroblock we know have not changed since the previous frame and could be coded as P_SKIPs. Libx264 however, may or may not oblige according to its logic, and this specific information is not transmitted in the bitstream nor can be recovered at the decoder. Elias ________________________________________ From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> on behalf of Anton Khirnov <anton@khirnov.net> Sent: Saturday, June 18, 2022 5:06 PM To: ffmpeg-devel@ffmpeg.org Subject: RE: [EXTERNAL][FFmpeg-devel] [PATCH] Added support for MB_INFO CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe. Quoting Carotti, Elias (2022-06-17 10:41:30)
Hi all, any chance someone could possibly have a look at this patch, please? Thanks in advance
This looks like it belongs in AVVideoEncParams. -- Anton Khirnov _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". NICE SRL, viale Monte Grappa 3/5, 20124 Milano, Italia, Registro delle Imprese di Milano Monza Brianza Lodi REA n. 2096882, Capitale Sociale: 10.329,14 EUR i.v., Cod. Fisc. e P.IVA 01133050052, Societa con Socio Unico
Quoting Carotti, Elias (2022-06-21 10:48:07)
Hi,
extending AVVideoEncParams was the first hypothesis I made but it didn't seem it was the proper place to add the mb_info flags.
I may be wrong but my impression is that AVVideoEncParams is used to carry encoding parameters read from the bitstream at the decoder side while here were going the other direction, i.e., were passing information from the application to the encoder.
Secondly, mb_info can't be strictly considered encoding parameters and it's not present in the bitstream at all. It's just a way to give hints to the libx264 encoder on which macroblock we know have not changed since the previous frame and could be coded as P_SKIPs. Libx264 however, may or may not oblige according to its logic, and this specific information is not transmitted in the bitstream nor can be recovered at the decoder.
Right, seems I was too hasty in reading your patch. But then I have to wonder whether this really needs a new installed header, with a struct and a destructor, given that it's specific to a single encoder for a single codec that is about 20 years old now. Wouldn't AV_FRAME_DATA_X264_MBINFO that would be just a raw array of uint8_t serve your needs just as fine? You could even get custom buffer management by using AVFrameSideData.buf. -- Anton Khirnov
Anton Khirnov:
Quoting Carotti, Elias (2022-06-21 10:48:07)
Hi,
extending AVVideoEncParams was the first hypothesis I made but it didn't seem it was the proper place to add the mb_info flags.
I may be wrong but my impression is that AVVideoEncParams is used to carry encoding parameters read from the bitstream at the decoder side while here were going the other direction, i.e., were passing information from the application to the encoder.
Secondly, mb_info can't be strictly considered encoding parameters and it's not present in the bitstream at all. It's just a way to give hints to the libx264 encoder on which macroblock we know have not changed since the previous frame and could be coded as P_SKIPs. Libx264 however, may or may not oblige according to its logic, and this specific information is not transmitted in the bitstream nor can be recovered at the decoder.
Right, seems I was too hasty in reading your patch.
But then I have to wonder whether this really needs a new installed header, with a struct and a destructor, given that it's specific to a single encoder for a single codec that is about 20 years old now.
Wouldn't AV_FRAME_DATA_X264_MBINFO that would be just a raw array of uint8_t serve your needs just as fine? You could even get custom buffer management by using AVFrameSideData.buf.
There is one problem though: libx264's free functions don't accept an opaque parameter, so one can't easily create a reference for libx264 to unref. I don't see a way around duplicating this buffer in the encoder. (Or is there a way to know when libx264 is done with using this buffer?) - Andreas
Quoting Andreas Rheinhardt (2022-06-23 16:21:18)
Anton Khirnov:
Quoting Carotti, Elias (2022-06-21 10:48:07)
Hi,
extending AVVideoEncParams was the first hypothesis I made but it didn't seem it was the proper place to add the mb_info flags.
I may be wrong but my impression is that AVVideoEncParams is used to carry encoding parameters read from the bitstream at the decoder side while here were going the other direction, i.e., were passing information from the application to the encoder.
Secondly, mb_info can't be strictly considered encoding parameters and it's not present in the bitstream at all. It's just a way to give hints to the libx264 encoder on which macroblock we know have not changed since the previous frame and could be coded as P_SKIPs. Libx264 however, may or may not oblige according to its logic, and this specific information is not transmitted in the bitstream nor can be recovered at the decoder.
Right, seems I was too hasty in reading your patch.
But then I have to wonder whether this really needs a new installed header, with a struct and a destructor, given that it's specific to a single encoder for a single codec that is about 20 years old now.
Wouldn't AV_FRAME_DATA_X264_MBINFO that would be just a raw array of uint8_t serve your needs just as fine? You could even get custom buffer management by using AVFrameSideData.buf.
There is one problem though: libx264's free functions don't accept an opaque parameter, so one can't easily create a reference for libx264 to unref. I don't see a way around duplicating this buffer in the encoder. (Or is there a way to know when libx264 is done with using this buffer?)
An ugly, but workable hack could be - user allocates the AVBuffer with extra space at the end - lavc/libx264.c checks that there is extra space AND the buffer is writable (so the same side data wasn't passed to multiple encoders), then stores its AVBufferRef* in there -- Anton Khirnov
Anton Khirnov:
Quoting Andreas Rheinhardt (2022-06-23 16:21:18)
Anton Khirnov:
Quoting Carotti, Elias (2022-06-21 10:48:07)
Hi,
extending AVVideoEncParams was the first hypothesis I made but it didn't seem it was the proper place to add the mb_info flags.
I may be wrong but my impression is that AVVideoEncParams is used to carry encoding parameters read from the bitstream at the decoder side while here were going the other direction, i.e., were passing information from the application to the encoder.
Secondly, mb_info can't be strictly considered encoding parameters and it's not present in the bitstream at all. It's just a way to give hints to the libx264 encoder on which macroblock we know have not changed since the previous frame and could be coded as P_SKIPs. Libx264 however, may or may not oblige according to its logic, and this specific information is not transmitted in the bitstream nor can be recovered at the decoder.
Right, seems I was too hasty in reading your patch.
But then I have to wonder whether this really needs a new installed header, with a struct and a destructor, given that it's specific to a single encoder for a single codec that is about 20 years old now.
Wouldn't AV_FRAME_DATA_X264_MBINFO that would be just a raw array of uint8_t serve your needs just as fine? You could even get custom buffer management by using AVFrameSideData.buf.
There is one problem though: libx264's free functions don't accept an opaque parameter, so one can't easily create a reference for libx264 to unref. I don't see a way around duplicating this buffer in the encoder. (Or is there a way to know when libx264 is done with using this buffer?)
An ugly, but workable hack could be - user allocates the AVBuffer with extra space at the end - lavc/libx264.c checks that there is extra space AND the buffer is writable (so the same side data wasn't passed to multiple encoders), then stores its AVBufferRef* in there
Ugly? Certainly. Workable? Questionable: Depending upon how the user uses the API the frame might not be writable when it reaches the encode callback of our libx264 wrapper even if the user actually supplies writable frames to avcodec_send_frame(). The reason is that if said callback is called in avcodec_send_frame(), both the user's AVFrame as well as the newly created reference to it (which is the frame that the callback receives) will each own a reference to each side data. The callback is called in avcodec_send_frame() if there is currently no buffered packet ready to be forwarded to the user. This is true if the user drains the encoder every time a frame has been sent. And that is how we ordinarily use the API (our examples use this pattern as well as ffmpeg.c). So I guess that this is the common way our API is used by others as well. (I think I once proposed adding a flag to muxers/encoders to select whether an ownership transfer of the packet/frame is intended or not.) - Andreas
participants (5)
-
Andreas Rheinhardt -
Anton Khirnov -
Carotti, Elias -
Stefano Sabatini -
Timo Rothenpieler