diff mbox series

[FFmpeg-devel,3/3] avformat/webpenc: Check seeks

Message ID HE1PR0301MB2154E5A8AF65ECA5095F63028F719@HE1PR0301MB2154.eurprd03.prod.outlook.com
State New
Headers show
Series [FFmpeg-devel,1/3] avcodec/libwebpenc_animencoder: Don't return pkt without data/side-data
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

Andreas Rheinhardt April 11, 2021, 12:43 a.m. UTC
When writing the trailer, the WebP muxer unconditionally seeks back
to the front to update some elements. Yet this doesn't work if
the seek failed, so check for this.

(This has been mentioned in ticket #9179.)

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

Comments

James Almer April 11, 2021, 1:18 a.m. UTC | #1
On 4/10/2021 9:43 PM, Andreas Rheinhardt wrote:
> When writing the trailer, the WebP muxer unconditionally seeks back
> to the front to update some elements. Yet this doesn't work if
> the seek failed, so check for this.
> 
> (This has been mentioned in ticket #9179.)
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
>   libavformat/webpenc.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/libavformat/webpenc.c b/libavformat/webpenc.c
> index 3962986c32..a24920d181 100644
> --- a/libavformat/webpenc.c
> +++ b/libavformat/webpenc.c
> @@ -174,8 +174,8 @@ static int webp_write_trailer(AVFormatContext *s)
>   
>       if (w->using_webp_anim_encoder) {
>           if ((w->frame_count > 1) && w->loop) {  // Write loop count.
> -            avio_seek(s->pb, 42, SEEK_SET);
> -            avio_wl16(s->pb, w->loop);
> +            if (avio_seek(s->pb, 42, SEEK_SET) == 42)

I think it's better if you also check for (s->pb->seekable & 
AVIO_SEEKABLE_NORMAL) before calling avio_seek() + avio_w* to begin with.

> +                avio_wl16(s->pb, w->loop);
>           }
>       } else {
>           int ret;
> @@ -183,8 +183,8 @@ static int webp_write_trailer(AVFormatContext *s)
>               return ret;
>   
>           filesize = avio_tell(s->pb);
> -        avio_seek(s->pb, 4, SEEK_SET);
> -        avio_wl32(s->pb, filesize - 8);
> +        if (avio_seek(s->pb, 4, SEEK_SET) == 4)
> +            avio_wl32(s->pb, filesize - 8);
>           // Note: without the following, avio only writes 8 bytes to the file.
>           avio_seek(s->pb, filesize, SEEK_SET);
>       }
>
Andreas Rheinhardt April 11, 2021, 1:21 a.m. UTC | #2
James Almer:
> On 4/10/2021 9:43 PM, Andreas Rheinhardt wrote:
>> When writing the trailer, the WebP muxer unconditionally seeks back
>> to the front to update some elements. Yet this doesn't work if
>> the seek failed, so check for this.
>>
>> (This has been mentioned in ticket #9179.)
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>> ---
>>   libavformat/webpenc.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/libavformat/webpenc.c b/libavformat/webpenc.c
>> index 3962986c32..a24920d181 100644
>> --- a/libavformat/webpenc.c
>> +++ b/libavformat/webpenc.c
>> @@ -174,8 +174,8 @@ static int webp_write_trailer(AVFormatContext *s)
>>         if (w->using_webp_anim_encoder) {
>>           if ((w->frame_count > 1) && w->loop) {  // Write loop count.
>> -            avio_seek(s->pb, 42, SEEK_SET);
>> -            avio_wl16(s->pb, w->loop);
>> +            if (avio_seek(s->pb, 42, SEEK_SET) == 42)
> 
> I think it's better if you also check for (s->pb->seekable &
> AVIO_SEEKABLE_NORMAL) before calling avio_seek() + avio_w* to begin with.
> 

Actually I intentionally didn't do that because the seek might work even
when said flag is not set (when the destination is still in the
AVIOContext's buffer).

>> +                avio_wl16(s->pb, w->loop);
>>           }
>>       } else {
>>           int ret;
>> @@ -183,8 +183,8 @@ static int webp_write_trailer(AVFormatContext *s)
>>               return ret;
>>             filesize = avio_tell(s->pb);
>> -        avio_seek(s->pb, 4, SEEK_SET);
>> -        avio_wl32(s->pb, filesize - 8);
>> +        if (avio_seek(s->pb, 4, SEEK_SET) == 4)
>> +            avio_wl32(s->pb, filesize - 8);
>>           // Note: without the following, avio only writes 8 bytes to
>> the file.
>>           avio_seek(s->pb, filesize, SEEK_SET);
>>       }
>>
>
James Almer April 11, 2021, 6:54 p.m. UTC | #3
On 4/10/2021 10:21 PM, Andreas Rheinhardt wrote:
> James Almer:
>> On 4/10/2021 9:43 PM, Andreas Rheinhardt wrote:
>>> When writing the trailer, the WebP muxer unconditionally seeks back
>>> to the front to update some elements. Yet this doesn't work if
>>> the seek failed, so check for this.
>>>
>>> (This has been mentioned in ticket #9179.)
>>>
>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>>> ---
>>>    libavformat/webpenc.c | 8 ++++----
>>>    1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/libavformat/webpenc.c b/libavformat/webpenc.c
>>> index 3962986c32..a24920d181 100644
>>> --- a/libavformat/webpenc.c
>>> +++ b/libavformat/webpenc.c
>>> @@ -174,8 +174,8 @@ static int webp_write_trailer(AVFormatContext *s)
>>>          if (w->using_webp_anim_encoder) {
>>>            if ((w->frame_count > 1) && w->loop) {  // Write loop count.
>>> -            avio_seek(s->pb, 42, SEEK_SET);
>>> -            avio_wl16(s->pb, w->loop);
>>> +            if (avio_seek(s->pb, 42, SEEK_SET) == 42)
>>
>> I think it's better if you also check for (s->pb->seekable &
>> AVIO_SEEKABLE_NORMAL) before calling avio_seek() + avio_w* to begin with.
>>
> 
> Actually I intentionally didn't do that because the seek might work even
> when said flag is not set (when the destination is still in the
> AVIOContext's buffer).

Ok, patch LGTM then.

Carl argued this value should not be set in webp_write_trailer() to 
begin with, but that's a separate change.

> 
>>> +                avio_wl16(s->pb, w->loop);
>>>            }
>>>        } else {
>>>            int ret;
>>> @@ -183,8 +183,8 @@ static int webp_write_trailer(AVFormatContext *s)
>>>                return ret;
>>>              filesize = avio_tell(s->pb);
>>> -        avio_seek(s->pb, 4, SEEK_SET);
>>> -        avio_wl32(s->pb, filesize - 8);
>>> +        if (avio_seek(s->pb, 4, SEEK_SET) == 4)
>>> +            avio_wl32(s->pb, filesize - 8);
>>>            // Note: without the following, avio only writes 8 bytes to
>>> the file.
>>>            avio_seek(s->pb, filesize, SEEK_SET);
>>>        }
>>>
>>
> _______________________________________________
> 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".
>
diff mbox series

Patch

diff --git a/libavformat/webpenc.c b/libavformat/webpenc.c
index 3962986c32..a24920d181 100644
--- a/libavformat/webpenc.c
+++ b/libavformat/webpenc.c
@@ -174,8 +174,8 @@  static int webp_write_trailer(AVFormatContext *s)
 
     if (w->using_webp_anim_encoder) {
         if ((w->frame_count > 1) && w->loop) {  // Write loop count.
-            avio_seek(s->pb, 42, SEEK_SET);
-            avio_wl16(s->pb, w->loop);
+            if (avio_seek(s->pb, 42, SEEK_SET) == 42)
+                avio_wl16(s->pb, w->loop);
         }
     } else {
         int ret;
@@ -183,8 +183,8 @@  static int webp_write_trailer(AVFormatContext *s)
             return ret;
 
         filesize = avio_tell(s->pb);
-        avio_seek(s->pb, 4, SEEK_SET);
-        avio_wl32(s->pb, filesize - 8);
+        if (avio_seek(s->pb, 4, SEEK_SET) == 4)
+            avio_wl32(s->pb, filesize - 8);
         // Note: without the following, avio only writes 8 bytes to the file.
         avio_seek(s->pb, filesize, SEEK_SET);
     }