diff mbox series

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

Message ID 20210313234406.3414-1-jamrial@gmail.com
State New
Headers show
Series [FFmpeg-devel] fftools/ffmpeg: use the correct size value for wrapped_avframe packets
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

James Almer March 13, 2021, 11:44 p.m. UTC
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@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(-)

Comments

James Almer March 14, 2021, 12:42 a.m. UTC | #1
On 3/13/2021 8:44 PM, James Almer wrote:
> 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@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);

Changed this locally to fallback to pkt->size to avoid breaking hardware 
pixel formats.

> +    } else
> +        size = pkt->size;
> +
> +    ost->data_size += size;
>       ost->packets_written++;
>   
>       pkt->stream_index = ost->index;
>
Andreas Rheinhardt March 15, 2021, 3:53 a.m. UTC | #2
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@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?

- Andreas
James Almer March 15, 2021, 1:39 p.m. UTC | #3
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@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.
diff mbox series

Patch

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;