[FFmpeg-devel] [PATCH 1/4] avformat/dvdvideodec: Explicitly return 0 on success

[FFmpeg-devel] [PATCH 1/4] avformat/dvdvideodec: Explicitly return 0 on success

[Andreas Rheinhardt]

Don't "return ret" even when ret is zero on success.

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

diff --git a/libavformat/dvdvideodec.c b/libavformat/dvdvideodec.c
index 3355010356..fd1f640560 100644
--- a/libavformat/dvdvideodec.c
+++ b/libavformat/dvdvideodec.c
@@ -863,7 +863,7 @@ static int dvdvideo_video_stream_setup(AVFormatContext *s)
         return ret;
     }
 
-    return ret;
+    return 0;
 }
 
 static int dvdvideo_audio_stream_analyze(AVFormatContext *s, audio_attr_t audio_attr,
@@ -1032,7 +1032,7 @@ break_error:
         return ret;
     }
 
-    return ret;
+    return 0;
 }
 
 static int dvdvideo_subp_stream_analyze(AVFormatContext *s, uint32_t offset, subp_attr_t subp_attr,
@@ -1056,7 +1056,6 @@ static int dvdvideo_subp_stream_add(AVFormatContext *s, DVDVideoPGCSubtitleStrea
 {
     AVStream *st;
     FFStream *sti;
-    int ret = 0;
 
     st = avformat_new_stream(s, NULL);
     if (!st)
@@ -1080,7 +1079,7 @@ static int dvdvideo_subp_stream_add(AVFormatContext *s, DVDVideoPGCSubtitleStrea
     avpriv_set_pts_info(st, DVDVIDEO_PTS_WRAP_BITS,
                         DVDVIDEO_TIME_BASE_Q.num, DVDVIDEO_TIME_BASE_Q.den);
 
-    return ret;
+    return 0;
 }
 
 static int dvdvideo_subp_stream_add_internal(AVFormatContext *s, uint32_t offset,
@@ -1098,17 +1097,15 @@ static int dvdvideo_subp_stream_add_internal(AVFormatContext *s, uint32_t offset
     /* IFO structures can declare duplicate entries for the same startcode */
     for (int i = 0; i < s->nb_streams; i++)
         if (s->streams[i]->id == entry.startcode)
-            goto end;
+            return 0;
 
     if ((ret = dvdvideo_subp_stream_add(s, &entry, AVSTREAM_PARSE_HEADERS)) < 0)
         goto end_error;
 
-    goto end;
+    return 0;
 
 end_error:
     av_log(s, AV_LOG_ERROR, "Unable to add subtitle stream\n");
-
-end:
     return ret;
 }
 
@@ -1291,7 +1288,7 @@ static int dvdvideo_read_header(AVFormatContext *s)
     if (!c->opt_preindex)
         return dvdvideo_chapters_setup_simple(s);
 
-    return ret;
+    return 0;
 }
 
 static int dvdvideo_read_packet(AVFormatContext *s, AVPacket *pkt)
-- 
2.40.1

_______________________________________________
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".

[FFmpeg-devel] [PATCH 2/4] avformat/dvdvideodec: Don't store AVInputFormat*

[Andreas Rheinhardt]

The inner AVInputFormat* of the inner mpegps-demuxer
is only used once (in avformat_open_input()), so
don't even store it. In fact, just use ff_mpegps_demuxer
directly, as this demuxer has a configure dependency
on it.

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

diff --git a/libavformat/dvdvideodec.c b/libavformat/dvdvideodec.c
index fd1f640560..ef2d4e6df4 100644
--- a/libavformat/dvdvideodec.c
+++ b/libavformat/dvdvideodec.c
@@ -136,7 +136,6 @@ typedef struct DVDVideoDemuxContext {
     int                         opt_trim;           /* trim padding cells at beginning */
 
     /* subdemux */
-    const AVInputFormat         *mpeg_fmt;          /* inner MPEG-PS (VOB) demuxer */
     AVFormatContext             *mpeg_ctx;          /* context for inner demuxer */
     uint8_t                     *mpeg_buf;          /* buffer for inner demuxer */
     FFIOContext                 mpeg_pb;            /* buffer context for inner demuxer */
@@ -1210,12 +1209,9 @@ static void dvdvideo_subdemux_close(AVFormatContext *s)
 static int dvdvideo_subdemux_open(AVFormatContext *s)
 {
     DVDVideoDemuxContext *c = s->priv_data;
-
+    extern const AVInputFormat ff_mpegps_demuxer;
     int ret = 0;
 
-    if (!(c->mpeg_fmt = av_find_input_format("mpeg")))
-        return AVERROR_DEMUXER_NOT_FOUND;
-
     if (!(c->mpeg_ctx = avformat_alloc_context()))
         return AVERROR(ENOMEM);
 
@@ -1246,7 +1242,7 @@ static int dvdvideo_subdemux_open(AVFormatContext *s)
     c->mpeg_ctx->correct_ts_overflow = 0;
     c->mpeg_ctx->io_open = NULL;
 
-    return avformat_open_input(&c->mpeg_ctx, "", c->mpeg_fmt, NULL);
+    return avformat_open_input(&c->mpeg_ctx, "", &ff_mpegps_demuxer, NULL);
 }
 
 static int dvdvideo_read_header(AVFormatContext *s)
-- 
2.40.1

_______________________________________________
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".

[FFmpeg-devel] [PATCH 3/4] avformat/dvdvideodec: Only free allocated buffers

[Andreas Rheinhardt]

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
How has this slipped through?
Btw: This patchset is based upon code inspection, not on reading
actual files.

 libavformat/dvdvideodec.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/libavformat/dvdvideodec.c b/libavformat/dvdvideodec.c
index ef2d4e6df4..f2f23affb2 100644
--- a/libavformat/dvdvideodec.c
+++ b/libavformat/dvdvideodec.c
@@ -1202,7 +1202,6 @@ static void dvdvideo_subdemux_close(AVFormatContext *s)
     DVDVideoDemuxContext *c = s->priv_data;
 
     av_freep(&c->mpeg_pb.pub.buffer);
-    av_freep(&c->mpeg_pb);
     avformat_close_input(&c->mpeg_ctx);
 }
 
-- 
2.40.1

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 3/4] avformat/dvdvideodec: Only free allocated buffers

[Andreas Rheinhardt]

Andreas Rheinhardt:
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
> How has this slipped through?

Answer: AVIOContext starts with an AVClass* that is unset when using
ffio_init_context(). Therefore the av_freep() leads to freeing of a NULL
pointer which does not segfault.

> Btw: This patchset is based upon code inspection, not on reading
> actual files.
> 
>  libavformat/dvdvideodec.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/libavformat/dvdvideodec.c b/libavformat/dvdvideodec.c
> index ef2d4e6df4..f2f23affb2 100644
> --- a/libavformat/dvdvideodec.c
> +++ b/libavformat/dvdvideodec.c
> @@ -1202,7 +1202,6 @@ static void dvdvideo_subdemux_close(AVFormatContext *s)
>      DVDVideoDemuxContext *c = s->priv_data;
>  
>      av_freep(&c->mpeg_pb.pub.buffer);
> -    av_freep(&c->mpeg_pb);
>      avformat_close_input(&c->mpeg_ctx);
>  }
>  

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 3/4] avformat/dvdvideodec: Only free allocated buffers

[James Almer]

On 3/2/2024 12:47 PM, Andreas Rheinhardt wrote:
> Andreas Rheinhardt:
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>> ---
>> How has this slipped through?
> 
> Answer: AVIOContext starts with an AVClass* that is unset when using
> ffio_init_context(). Therefore the av_freep() leads to freeing of a NULL
> pointer which does not segfault.

Would setting s->av_class to &ff_avio_options in ffio_init_context() 
prevent this from happening again?

> 
>> Btw: This patchset is based upon code inspection, not on reading
>> actual files.
>>
>>   libavformat/dvdvideodec.c | 1 -
>>   1 file changed, 1 deletion(-)
>>
>> diff --git a/libavformat/dvdvideodec.c b/libavformat/dvdvideodec.c
>> index ef2d4e6df4..f2f23affb2 100644
>> --- a/libavformat/dvdvideodec.c
>> +++ b/libavformat/dvdvideodec.c
>> @@ -1202,7 +1202,6 @@ static void dvdvideo_subdemux_close(AVFormatContext *s)
>>       DVDVideoDemuxContext *c = s->priv_data;
>>   
>>       av_freep(&c->mpeg_pb.pub.buffer);
>> -    av_freep(&c->mpeg_pb);
>>       avformat_close_input(&c->mpeg_ctx);
>>   }
>>   
> 
> _______________________________________________
> 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".
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 3/4] avformat/dvdvideodec: Only free allocated buffers

[Andreas Rheinhardt]

James Almer:
> On 3/2/2024 12:47 PM, Andreas Rheinhardt wrote:
>> Andreas Rheinhardt:
>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>>> ---
>>> How has this slipped through?
>>
>> Answer: AVIOContext starts with an AVClass* that is unset when using
>> ffio_init_context(). Therefore the av_freep() leads to freeing of a NULL
>> pointer which does not segfault.
> 
> Would setting s->av_class to &ff_avio_options in ffio_init_context()
> prevent this from happening again?
> 

ff_avio_options is not an AVClass, so this is wrong; setting it to
ff_avio_class would also be wrong, because said class is only to be used
with the URLProtocol-based AVIOContexts (the child_next and
child_class_iterate callbacks are designed for this; the former returns
the AVIOContext's opaque, believing it to point to an URLContext, yet in
general it need not point to an AVClass-enabled struct at all).

In fact, ff_avio_options etc. should be moved to avio.c. I'll look into
this.

>>
>>> Btw: This patchset is based upon code inspection, not on reading
>>> actual files.
>>>
>>>   libavformat/dvdvideodec.c | 1 -
>>>   1 file changed, 1 deletion(-)
>>>
>>> diff --git a/libavformat/dvdvideodec.c b/libavformat/dvdvideodec.c
>>> index ef2d4e6df4..f2f23affb2 100644
>>> --- a/libavformat/dvdvideodec.c
>>> +++ b/libavformat/dvdvideodec.c
>>> @@ -1202,7 +1202,6 @@ static void
>>> dvdvideo_subdemux_close(AVFormatContext *s)
>>>       DVDVideoDemuxContext *c = s->priv_data;
>>>         av_freep(&c->mpeg_pb.pub.buffer);
>>> -    av_freep(&c->mpeg_pb);
>>>       avformat_close_input(&c->mpeg_ctx);
>>>   }
>>>   
>>

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 3/4] avformat/dvdvideodec: Only free allocated buffers

[Marth64]

FYI: hls, dashdec do the same av_freep(). Doesn't make this right, but
saying that was where I used as an example (so they should probably fixed
too?)

On Sat, Mar 2, 2024 at 10:17 AM Andreas Rheinhardt <
andreas.rheinhardt@outlook.com> wrote:

> James Almer:
> > On 3/2/2024 12:47 PM, Andreas Rheinhardt wrote:
> >> Andreas Rheinhardt:
> >>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> >>> ---
> >>> How has this slipped through?
> >>
> >> Answer: AVIOContext starts with an AVClass* that is unset when using
> >> ffio_init_context(). Therefore the av_freep() leads to freeing of a NULL
> >> pointer which does not segfault.
> >
> > Would setting s->av_class to &ff_avio_options in ffio_init_context()
> > prevent this from happening again?
> >
>
> ff_avio_options is not an AVClass, so this is wrong; setting it to
> ff_avio_class would also be wrong, because said class is only to be used
> with the URLProtocol-based AVIOContexts (the child_next and
> child_class_iterate callbacks are designed for this; the former returns
> the AVIOContext's opaque, believing it to point to an URLContext, yet in
> general it need not point to an AVClass-enabled struct at all).
>
> In fact, ff_avio_options etc. should be moved to avio.c. I'll look into
> this.
>
> >>
> >>> Btw: This patchset is based upon code inspection, not on reading
> >>> actual files.
> >>>
> >>>   libavformat/dvdvideodec.c | 1 -
> >>>   1 file changed, 1 deletion(-)
> >>>
> >>> diff --git a/libavformat/dvdvideodec.c b/libavformat/dvdvideodec.c
> >>> index ef2d4e6df4..f2f23affb2 100644
> >>> --- a/libavformat/dvdvideodec.c
> >>> +++ b/libavformat/dvdvideodec.c
> >>> @@ -1202,7 +1202,6 @@ static void
> >>> dvdvideo_subdemux_close(AVFormatContext *s)
> >>>       DVDVideoDemuxContext *c = s->priv_data;
> >>>         av_freep(&c->mpeg_pb.pub.buffer);
> >>> -    av_freep(&c->mpeg_pb);
> >>>       avformat_close_input(&c->mpeg_ctx);
> >>>   }
> >>>
> >>
>
> _______________________________________________
> 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".
>
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 3/4] avformat/dvdvideodec: Only free allocated buffers

[Marth64]

Nevermind, disregard. They do not, and I misread. Thanks for catching this.

On Sat, Mar 2, 2024 at 8:31 PM Marth64 <marth64@proxyid.net> wrote:

> FYI: hls, dashdec do the same av_freep(). Doesn't make this right, but
> saying that was where I used as an example (so they should probably fixed
> too?)
>
> On Sat, Mar 2, 2024 at 10:17 AM Andreas Rheinhardt <
> andreas.rheinhardt@outlook.com> wrote:
>
>> James Almer:
>> > On 3/2/2024 12:47 PM, Andreas Rheinhardt wrote:
>> >> Andreas Rheinhardt:
>> >>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>> >>> ---
>> >>> How has this slipped through?
>> >>
>> >> Answer: AVIOContext starts with an AVClass* that is unset when using
>> >> ffio_init_context(). Therefore the av_freep() leads to freeing of a
>> NULL
>> >> pointer which does not segfault.
>> >
>> > Would setting s->av_class to &ff_avio_options in ffio_init_context()
>> > prevent this from happening again?
>> >
>>
>> ff_avio_options is not an AVClass, so this is wrong; setting it to
>> ff_avio_class would also be wrong, because said class is only to be used
>> with the URLProtocol-based AVIOContexts (the child_next and
>> child_class_iterate callbacks are designed for this; the former returns
>> the AVIOContext's opaque, believing it to point to an URLContext, yet in
>> general it need not point to an AVClass-enabled struct at all).
>>
>> In fact, ff_avio_options etc. should be moved to avio.c. I'll look into
>> this.
>>
>> >>
>> >>> Btw: This patchset is based upon code inspection, not on reading
>> >>> actual files.
>> >>>
>> >>>   libavformat/dvdvideodec.c | 1 -
>> >>>   1 file changed, 1 deletion(-)
>> >>>
>> >>> diff --git a/libavformat/dvdvideodec.c b/libavformat/dvdvideodec.c
>> >>> index ef2d4e6df4..f2f23affb2 100644
>> >>> --- a/libavformat/dvdvideodec.c
>> >>> +++ b/libavformat/dvdvideodec.c
>> >>> @@ -1202,7 +1202,6 @@ static void
>> >>> dvdvideo_subdemux_close(AVFormatContext *s)
>> >>>       DVDVideoDemuxContext *c = s->priv_data;
>> >>>         av_freep(&c->mpeg_pb.pub.buffer);
>> >>> -    av_freep(&c->mpeg_pb);
>> >>>       avformat_close_input(&c->mpeg_ctx);
>> >>>   }
>> >>>
>> >>
>>
>> _______________________________________________
>> 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".
>>
>
_______________________________________________
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".

[FFmpeg-devel] [PATCH 4/4] avformat/dvdvideodec: Reorder allocations to simplify freeing

[Andreas Rheinhardt]

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

diff --git a/libavformat/dvdvideodec.c b/libavformat/dvdvideodec.c
index f2f23affb2..7f4ecf2b61 100644
--- a/libavformat/dvdvideodec.c
+++ b/libavformat/dvdvideodec.c
@@ -1211,20 +1211,16 @@ static int dvdvideo_subdemux_open(AVFormatContext *s)
     extern const AVInputFormat ff_mpegps_demuxer;
     int ret = 0;
 
-    if (!(c->mpeg_ctx = avformat_alloc_context()))
-        return AVERROR(ENOMEM);
-
-    if (!(c->mpeg_buf = av_mallocz(DVDVIDEO_BLOCK_SIZE))) {
-        avformat_free_context(c->mpeg_ctx);
-        c->mpeg_ctx = NULL;
-
+    if (!(c->mpeg_buf = av_mallocz(DVDVIDEO_BLOCK_SIZE)))
         return AVERROR(ENOMEM);
-    }
 
     ffio_init_context(&c->mpeg_pb, c->mpeg_buf, DVDVIDEO_BLOCK_SIZE, 0, s,
                       dvdvideo_subdemux_read_data, NULL, NULL);
     c->mpeg_pb.pub.seekable = 0;
 
+    if (!(c->mpeg_ctx = avformat_alloc_context()))
+        return AVERROR(ENOMEM);
+
     if ((ret = ff_copy_whiteblacklists(c->mpeg_ctx, s)) < 0) {
         avformat_free_context(c->mpeg_ctx);
         c->mpeg_ctx = NULL;
-- 
2.40.1

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 4/4] avformat/dvdvideodec: Reorder allocations to simplify freeing

[Marth64]

Patchset LGTM.

On Sat, Mar 2, 2024 at 9:42 AM Andreas Rheinhardt <
andreas.rheinhardt@outlook.com> wrote:

> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
>  libavformat/dvdvideodec.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/libavformat/dvdvideodec.c b/libavformat/dvdvideodec.c
> index f2f23affb2..7f4ecf2b61 100644
> --- a/libavformat/dvdvideodec.c
> +++ b/libavformat/dvdvideodec.c
> @@ -1211,20 +1211,16 @@ static int dvdvideo_subdemux_open(AVFormatContext
> *s)
>      extern const AVInputFormat ff_mpegps_demuxer;
>      int ret = 0;
>
> -    if (!(c->mpeg_ctx = avformat_alloc_context()))
> -        return AVERROR(ENOMEM);
> -
> -    if (!(c->mpeg_buf = av_mallocz(DVDVIDEO_BLOCK_SIZE))) {
> -        avformat_free_context(c->mpeg_ctx);
> -        c->mpeg_ctx = NULL;
> -
> +    if (!(c->mpeg_buf = av_mallocz(DVDVIDEO_BLOCK_SIZE)))
>          return AVERROR(ENOMEM);
> -    }
>
>      ffio_init_context(&c->mpeg_pb, c->mpeg_buf, DVDVIDEO_BLOCK_SIZE, 0, s,
>                        dvdvideo_subdemux_read_data, NULL, NULL);
>      c->mpeg_pb.pub.seekable = 0;
>
> +    if (!(c->mpeg_ctx = avformat_alloc_context()))
> +        return AVERROR(ENOMEM);
> +
>      if ((ret = ff_copy_whiteblacklists(c->mpeg_ctx, s)) < 0) {
>          avformat_free_context(c->mpeg_ctx);
>          c->mpeg_ctx = NULL;
> --
> 2.40.1
>
> _______________________________________________
> 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".
>
_______________________________________________
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".