diff mbox series

[FFmpeg-devel,4/4] avformat/asf: Use ff_add_attached_pic() to read attached pics

Message ID HE1PR0301MB2154B436631B6D18CD575B438F7E9@HE1PR0301MB2154.eurprd03.prod.outlook.com
State Accepted
Commit ec4c04aa7b5a0c7a91e4a65775826283d23e08ac
Headers show
Series [FFmpeg-devel,1/4] avformat: Add and use helper function to add attachment streams | expand

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

Andreas Rheinhardt March 29, 2021, 8:45 a.m. UTC 
Also removes a stack packet.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavformat/asf.c | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

Comments

James Almer March 30, 2021, 1:17 p.m. UTC | #1 
On 3/29/2021 5:45 AM, Andreas Rheinhardt wrote:
> Also removes a stack packet.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
>   libavformat/asf.c | 17 +++--------------
>   1 file changed, 3 insertions(+), 14 deletions(-)
> 
> diff --git a/libavformat/asf.c b/libavformat/asf.c
> index 204355abab..cef0f9f646 100644
> --- a/libavformat/asf.c
> +++ b/libavformat/asf.c
> @@ -20,6 +20,7 @@
>   
>   #include "asf.h"
>   #include "id3v2.h"
> +#include "internal.h"
>   
>   const ff_asf_guid ff_asf_header = {
>       0x30, 0x26, 0xB2, 0x75, 0x8E, 0x66, 0xCF, 0x11, 0xA6, 0xD9, 0x00, 0xAA, 0x00, 0x62, 0xCE, 0x6C
> @@ -176,7 +177,6 @@ const AVMetadataConv ff_asf_metadata_conv[] = {
>    * but in reality this is only loosely similar */
>   static int asf_read_picture(AVFormatContext *s, int len)
>   {
> -    AVPacket pkt          = { 0 };
>       const CodecMime *mime = ff_id3v2_mime_tags;
>       enum  AVCodecID id    = AV_CODEC_ID_NONE;
>       char mimetype[64];
> @@ -230,22 +230,12 @@ static int asf_read_picture(AVFormatContext *s, int len)
>           return AVERROR(ENOMEM);
>       len -= avio_get_str16le(s->pb, len - picsize, desc, desc_len);
>   
> -    ret = av_get_packet(s->pb, &pkt, picsize);
> +    ret = ff_add_attached_pic(s, NULL, s->pb, NULL, picsize);
>       if (ret < 0)
>           goto fail;
> +    st = s->streams[s->nb_streams - 1];
>   
> -    st  = avformat_new_stream(s, NULL);
> -    if (!st) {
> -        ret = AVERROR(ENOMEM);
> -        goto fail;
> -    }
> -
> -    st->disposition              |= AV_DISPOSITION_ATTACHED_PIC;
> -    st->codecpar->codec_type      = AVMEDIA_TYPE_VIDEO;
>       st->codecpar->codec_id        = id;
> -    st->attached_pic              = pkt;
> -    st->attached_pic.stream_index = st->index;
> -    st->attached_pic.flags       |= AV_PKT_FLAG_KEY;
>   
>       if (*desc) {
>           if (av_dict_set(&st->metadata, "title", desc, AV_DICT_DONT_STRDUP_VAL) < 0)
> @@ -260,7 +250,6 @@ static int asf_read_picture(AVFormatContext *s, int len)
>   
>   fail:
>       av_freep(&desc);
> -    av_packet_unref(&pkt);
>       return ret;
>   }

This patch could be squashed into 1/4. Just apply 2/4 first.

Should be ok either way.
Andreas Rheinhardt March 30, 2021, 1:28 p.m. UTC | #2 
James Almer:
> On 3/29/2021 5:45 AM, Andreas Rheinhardt wrote:
>> Also removes a stack packet.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>> ---
>>   libavformat/asf.c | 17 +++--------------
>>   1 file changed, 3 insertions(+), 14 deletions(-)
>>
>> diff --git a/libavformat/asf.c b/libavformat/asf.c
>> index 204355abab..cef0f9f646 100644
>> --- a/libavformat/asf.c
>> +++ b/libavformat/asf.c
>> @@ -20,6 +20,7 @@
>>     #include "asf.h"
>>   #include "id3v2.h"
>> +#include "internal.h"
>>     const ff_asf_guid ff_asf_header = {
>>       0x30, 0x26, 0xB2, 0x75, 0x8E, 0x66, 0xCF, 0x11, 0xA6, 0xD9,
>> 0x00, 0xAA, 0x00, 0x62, 0xCE, 0x6C
>> @@ -176,7 +177,6 @@ const AVMetadataConv ff_asf_metadata_conv[] = {
>>    * but in reality this is only loosely similar */
>>   static int asf_read_picture(AVFormatContext *s, int len)
>>   {
>> -    AVPacket pkt          = { 0 };
>>       const CodecMime *mime = ff_id3v2_mime_tags;
>>       enum  AVCodecID id    = AV_CODEC_ID_NONE;
>>       char mimetype[64];
>> @@ -230,22 +230,12 @@ static int asf_read_picture(AVFormatContext *s,
>> int len)
>>           return AVERROR(ENOMEM);
>>       len -= avio_get_str16le(s->pb, len - picsize, desc, desc_len);
>>   -    ret = av_get_packet(s->pb, &pkt, picsize);
>> +    ret = ff_add_attached_pic(s, NULL, s->pb, NULL, picsize);
>>       if (ret < 0)
>>           goto fail;
>> +    st = s->streams[s->nb_streams - 1];
>>   -    st  = avformat_new_stream(s, NULL);
>> -    if (!st) {
>> -        ret = AVERROR(ENOMEM);
>> -        goto fail;
>> -    }
>> -
>> -    st->disposition              |= AV_DISPOSITION_ATTACHED_PIC;
>> -    st->codecpar->codec_type      = AVMEDIA_TYPE_VIDEO;
>>       st->codecpar->codec_id        = id;
>> -    st->attached_pic              = pkt;
>> -    st->attached_pic.stream_index = st->index;
>> -    st->attached_pic.flags       |= AV_PKT_FLAG_KEY;
>>         if (*desc) {
>>           if (av_dict_set(&st->metadata, "title", desc,
>> AV_DICT_DONT_STRDUP_VAL) < 0)
>> @@ -260,7 +250,6 @@ static int asf_read_picture(AVFormatContext *s,
>> int len)
>>     fail:
>>       av_freep(&desc);
>> -    av_packet_unref(&pkt);
>>       return ret;
>>   }
> 
> This patch could be squashed into 1/4. Just apply 2/4 first.
> 
> Should be ok either way.

The reason for the current ordering is the question about what happens
in case of errors: The asf code currently creates the stream only if
av_get_packet() succeeds; other code does not, which I don't like.
I could squash this into 1/4, but given that I wanted to make separate
patches for adding and using the new function and for changing the
behaviour on error, this would mean that the behaviour on error for asf
would change for one commit before being reverted to the old behaviour.

- Andreas
diff mbox series

Patch

diff --git a/libavformat/asf.c b/libavformat/asf.c
index 204355abab..cef0f9f646 100644
--- a/libavformat/asf.c
+++ b/libavformat/asf.c
@@ -20,6 +20,7 @@ 
 
 #include "asf.h"
 #include "id3v2.h"
+#include "internal.h"
 
 const ff_asf_guid ff_asf_header = {
     0x30, 0x26, 0xB2, 0x75, 0x8E, 0x66, 0xCF, 0x11, 0xA6, 0xD9, 0x00, 0xAA, 0x00, 0x62, 0xCE, 0x6C
@@ -176,7 +177,6 @@  const AVMetadataConv ff_asf_metadata_conv[] = {
  * but in reality this is only loosely similar */
 static int asf_read_picture(AVFormatContext *s, int len)
 {
-    AVPacket pkt          = { 0 };
     const CodecMime *mime = ff_id3v2_mime_tags;
     enum  AVCodecID id    = AV_CODEC_ID_NONE;
     char mimetype[64];
@@ -230,22 +230,12 @@  static int asf_read_picture(AVFormatContext *s, int len)
         return AVERROR(ENOMEM);
     len -= avio_get_str16le(s->pb, len - picsize, desc, desc_len);
 
-    ret = av_get_packet(s->pb, &pkt, picsize);
+    ret = ff_add_attached_pic(s, NULL, s->pb, NULL, picsize);
     if (ret < 0)
         goto fail;
+    st = s->streams[s->nb_streams - 1];
 
-    st  = avformat_new_stream(s, NULL);
-    if (!st) {
-        ret = AVERROR(ENOMEM);
-        goto fail;
-    }
-
-    st->disposition              |= AV_DISPOSITION_ATTACHED_PIC;
-    st->codecpar->codec_type      = AVMEDIA_TYPE_VIDEO;
     st->codecpar->codec_id        = id;
-    st->attached_pic              = pkt;
-    st->attached_pic.stream_index = st->index;
-    st->attached_pic.flags       |= AV_PKT_FLAG_KEY;
 
     if (*desc) {
         if (av_dict_set(&st->metadata, "title", desc, AV_DICT_DONT_STRDUP_VAL) < 0)
@@ -260,7 +250,6 @@  static int asf_read_picture(AVFormatContext *s, int len)
 
 fail:
     av_freep(&desc);
-    av_packet_unref(&pkt);
     return ret;
 }