[FFmpeg-devel] [PATCH] fftools/ffmpeg: use the correct size value for wrapped_avframe packets

[James Almer]

On 3/15/2021 12:53 AM, Andreas Rheinhardt wrote:
> James Almer:
>> pkt->size on packets wrapping an AVFrame signaled by the wrapped_avframe
>> codec_id are set to the size of the AVFrame structure and not the actual raw
>> data contained in it.
>> ffmpeg.c was using those values to print bogus stats about the resulting output
>> file.
>>
>> Before this patch:
>> frame=   24 fps=0.0 q=-0.0 Lsize=    7594kB time=00:00:01.00 bitrate=62209.8kbits/s speed=  27x
>> video:13kB audio:0kB subtitle:0kB other streams:0kB global headers:0kB muxing overhead: 60349.488281%
>>
>> After:
>> frame=   24 fps=0.0 q=-0.0 Lsize=    7594kB time=00:00:01.00 bitrate=62209.8kbits/s speed=  28x
>> video:7594kB audio:0kB subtitle:0kB other streams:0kB global headers:0kB muxing overhead: 0.002855%
>>
>> Signed-off-by: James Almer <jamrial at gmail.com>
>> ---
>> wrapped_avframe should be redefined to stop using sizeof(AVFrame) altogether.
>> I'll send a patch with a proposed approach for this.
>>
>>   fftools/ffmpeg.c | 14 ++++++++++++--
>>   1 file changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
>> index 2abbc0ff29..6dcf9006db 100644
>> --- a/fftools/ffmpeg.c
>> +++ b/fftools/ffmpeg.c
>> @@ -727,7 +727,7 @@ static void write_packet(OutputFile *of, AVPacket *pkt, OutputStream *ost, int u
>>   {
>>       AVFormatContext *s = of->ctx;
>>       AVStream *st = ost->st;
>> -    int ret;
>> +    int size, ret;
>>   
>>       /*
>>        * Audio encoders may split the packets --  #frames in != #packets out.
>> @@ -842,7 +842,17 @@ static void write_packet(OutputFile *of, AVPacket *pkt, OutputStream *ost, int u
>>       }
>>       ost->last_mux_dts = pkt->dts;
>>   
>> -    ost->data_size += pkt->size;
>> +    if (st->codecpar->codec_id == AV_CODEC_ID_WRAPPED_AVFRAME) {
>> +        AVFrame *frame = (AVFrame *)pkt->data;
>> +        if (!frame)
>> +            exit_program(1);
>> +        size = av_image_get_buffer_size(frame->format, frame->width, frame->height, 1);
>> +        if (size < 0)
>> +            exit_program(1);
>> +    } else
>> +        size = pkt->size;
>> +
>> +    ost->data_size += size;
>>       ost->packets_written++;
>>   
>>       pkt->stream_index = ost->index;
>>
> Is the format of wrapped_avframes actually publically documented
> anywhere? Are user applications supposed to look at the data of said
> frame or is this just something that allows users to pass AVFrames to
> muxers? Given that you sent this patch you probably believe the former,
> but is this generally accepted?

No, it's not documented. And no, I don't know if the user is meant to be 
allowed to look at it or not, precisely because this pseudo-codec is 
undocumented.
Maybe the idea was effectively to just use the encoder and send directly 
to muxers, or use a demuxer and send directly to the decoder, in an 
opaque way. Given how precarious this is and how a packet flag had to be 
added to such modules to signal a not very descriptive "this is from a 
trusted source", i wouldn't be surprised if it was an unspoken rule that 
nothing outside libav* was meant to manually handle these.
Marton mentioned users might just be looking at the frames, but i don't 
know if any project really does. And since packets don't report a 
codec_id (only whatever produced them does) but they do report a size, 
there's nothing stopping the user from doing so.

So i don't know, would this patch make people think looking at the 
wrapped frame is ok? Are they meant to, to begin with?
I can just drop this patch. It's just to stop printing absurd statistics 
like 60349.488281% overhead muxing. I'm more interested in making 
wrapped_frame less hacky.