[FFmpeg-devel] [PATCH 1/4] remove sccenc dependency on subtitles

[FFmpeg-devel] [PATCH 1/4] remove sccenc dependency on subtitles

[lance.lmwang]

From: Limin Wang <lance.lmwang@gmail.com>

Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
---
 libavformat/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavformat/Makefile b/libavformat/Makefile
index 3b9995a9d3..8e612b6cc7 100644
--- a/libavformat/Makefile
+++ b/libavformat/Makefile
@@ -514,7 +514,7 @@ OBJS-$(CONFIG_SBC_DEMUXER)               += sbcdec.o rawdec.o
 OBJS-$(CONFIG_SBC_MUXER)                 += rawenc.o
 OBJS-$(CONFIG_SBG_DEMUXER)               += sbgdec.o
 OBJS-$(CONFIG_SCC_DEMUXER)               += sccdec.o subtitles.o
-OBJS-$(CONFIG_SCC_MUXER)                 += sccenc.o subtitles.o
+OBJS-$(CONFIG_SCC_MUXER)                 += sccenc.o
 OBJS-$(CONFIG_SCD_DEMUXER)               += scd.o
 OBJS-$(CONFIG_SDP_DEMUXER)               += rtsp.o
 OBJS-$(CONFIG_SDR2_DEMUXER)              += sdr2.o
-- 
2.35.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] avcodec/ccaption_dec: check the length of packet and return used length

[lance.lmwang]

From: Limin Wang <lance.lmwang@gmail.com>

Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
---
 libavcodec/ccaption_dec.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/libavcodec/ccaption_dec.c b/libavcodec/ccaption_dec.c
index 34f0513b1a..8f61e8aa03 100644
--- a/libavcodec/ccaption_dec.c
+++ b/libavcodec/ccaption_dec.c
@@ -852,6 +852,11 @@ static int decode(AVCodecContext *avctx, AVSubtitle *sub,
     int i;
     unsigned nb_rect_allocated = 0;
 
+    if (len < 3) {
+        ff_dlog(avctx, "incomplete or broken packet");
+        return len;
+    }
+
     for (i = 0; i < len; i += 3) {
         uint8_t hi, cc_type = bptr[i] & 1;
 
@@ -922,7 +927,7 @@ static int decode(AVCodecContext *avctx, AVSubtitle *sub,
     }
 
     *got_sub = sub->num_rects > 0;
-    return ret;
+    return len;
 }
 
 #define OFFSET(x) offsetof(CCaptionSubContext, x)
-- 
2.35.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 2/4] avcodec/ccaption_dec: check the length of packet and return used length

[Paul B Mahol]

why?
_______________________________________________
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 2/4] avcodec/ccaption_dec: check the length of packet and return used length

[lance.lmwang]

On Wed, May 11, 2022 at 09:47:52PM +0200, Paul B Mahol wrote:
> why?

assuming the len is 1, the following code will access the next 3
array anymore, I think it's better to check the i with len -2.

for (i = 0; i < len; i += 3) {
to 
for (i = 0; i < len - 2; i += 3) {

for the return, I think it's correct to return the used length,
if it's error, have return already. right? 

-- 
Thanks,
Limin Wang
_______________________________________________
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 2/4] avcodec/ccaption_dec: check the length of packet and return used length

[Paul B Mahol]

On Thu, May 12, 2022 at 1:39 AM <lance.lmwang@gmail.com> wrote:

> On Wed, May 11, 2022 at 09:47:52PM +0200, Paul B Mahol wrote:
> > why?
>
> assuming the len is 1, the following code will access the next 3
> array anymore, I think it's better to check the i with len -2.
>
> for (i = 0; i < len; i += 3) {
> to
> for (i = 0; i < len - 2; i += 3) {
>
> for the return, I think it's correct to return the used length,
> if it's error, have return already. right?


I doubt so.

>
>
> --
> Thanks,
> Limin Wang
> _______________________________________________
> 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 2/4] avcodec/ccaption_dec: check the length of packet and return used length

[lance.lmwang]

On Thu, May 12, 2022 at 08:25:29AM +0200, Paul B Mahol wrote:
> On Thu, May 12, 2022 at 1:39 AM <lance.lmwang@gmail.com> wrote:
> 
> > On Wed, May 11, 2022 at 09:47:52PM +0200, Paul B Mahol wrote:
> > > why?
> >
> > assuming the len is 1, the following code will access the next 3
> > array anymore, I think it's better to check the i with len -2.
> >
> > for (i = 0; i < len; i += 3) {
> > to
> > for (i = 0; i < len - 2; i += 3) {
> >
> > for the return, I think it's correct to return the used length,
> > if it's error, have return already. right?
> 
> 
> I doubt so.

maybe I'm misunderstand it, but from the comments, the API claims that:
libavcodec/codec_internal.h
175          * @return amount of bytes read from the packet on success,
176          *         negative error code on failure
177          */
178         int (*decode)(struct AVCodecContext *avctx, struct AVFrame *frame,
179                       int *got_frame_ptr, struct AVPacket *avpkt);
180         /**
181          * Decode subtitle data to an AVSubtitle.
182          * cb is in this state if cb_type is FF_CODEC_CB_TYPE_DECODE_SUB.
183          *
184          * Apart from that this is like the decode callback.
185          */
186         int (*decode_sub)(struct AVCodecContext *avctx, struct AVSubtitle *sub,
187                           int *got_frame_ptr, const struct AVPacket *avpkt);

> >
> >
> > --
> > Thanks,
> > Limin Wang
> > _______________________________________________
> > 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".

-- 
Thanks,
Limin Wang
_______________________________________________
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 2/4] avcodec/ccaption_dec: check the length of packet and return used length

[Andreas Rheinhardt]

lance.lmwang@gmail.com:
> On Thu, May 12, 2022 at 08:25:29AM +0200, Paul B Mahol wrote:
>> On Thu, May 12, 2022 at 1:39 AM <lance.lmwang@gmail.com> wrote:
>>
>>> On Wed, May 11, 2022 at 09:47:52PM +0200, Paul B Mahol wrote:
>>>> why?
>>>
>>> assuming the len is 1, the following code will access the next 3
>>> array anymore, I think it's better to check the i with len -2.
>>>
>>> for (i = 0; i < len; i += 3) {
>>> to
>>> for (i = 0; i < len - 2; i += 3) {
>>>
>>> for the return, I think it's correct to return the used length,
>>> if it's error, have return already. right?
>>
>>
>> I doubt so.
> 
> maybe I'm misunderstand it, but from the comments, the API claims that:
> libavcodec/codec_internal.h
> 175          * @return amount of bytes read from the packet on success,
> 176          *         negative error code on failure
> 177          */
> 178         int (*decode)(struct AVCodecContext *avctx, struct AVFrame *frame,
> 179                       int *got_frame_ptr, struct AVPacket *avpkt);
> 180         /**
> 181          * Decode subtitle data to an AVSubtitle.
> 182          * cb is in this state if cb_type is FF_CODEC_CB_TYPE_DECODE_SUB.
> 183          *
> 184          * Apart from that this is like the decode callback.
> 185          */
> 186         int (*decode_sub)(struct AVCodecContext *avctx, struct AVSubtitle *sub,
> 187                           int *got_frame_ptr, const struct AVPacket *avpkt);
> 

This is correct. It is not only the internal API which claims that, but
the public API, too. And this just not honoured, in particular not in
case of subtitle recoding. See
https://github.com/mkver/FFmpeg/commit/ba1564c532654888015d67b70bf73d117c2d9f75

- Andreas
_______________________________________________
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 2/4] avcodec/ccaption_dec: check the length of packet and return used length

[Andreas Rheinhardt]

Andreas Rheinhardt:
> lance.lmwang@gmail.com:
>> On Thu, May 12, 2022 at 08:25:29AM +0200, Paul B Mahol wrote:
>>> On Thu, May 12, 2022 at 1:39 AM <lance.lmwang@gmail.com> wrote:
>>>
>>>> On Wed, May 11, 2022 at 09:47:52PM +0200, Paul B Mahol wrote:
>>>>> why?
>>>>
>>>> assuming the len is 1, the following code will access the next 3
>>>> array anymore, I think it's better to check the i with len -2.
>>>>
>>>> for (i = 0; i < len; i += 3) {
>>>> to
>>>> for (i = 0; i < len - 2; i += 3) {
>>>>
>>>> for the return, I think it's correct to return the used length,
>>>> if it's error, have return already. right?
>>>
>>>
>>> I doubt so.
>>
>> maybe I'm misunderstand it, but from the comments, the API claims that:
>> libavcodec/codec_internal.h
>> 175          * @return amount of bytes read from the packet on success,
>> 176          *         negative error code on failure
>> 177          */
>> 178         int (*decode)(struct AVCodecContext *avctx, struct AVFrame *frame,
>> 179                       int *got_frame_ptr, struct AVPacket *avpkt);
>> 180         /**
>> 181          * Decode subtitle data to an AVSubtitle.
>> 182          * cb is in this state if cb_type is FF_CODEC_CB_TYPE_DECODE_SUB.
>> 183          *
>> 184          * Apart from that this is like the decode callback.
>> 185          */
>> 186         int (*decode_sub)(struct AVCodecContext *avctx, struct AVSubtitle *sub,
>> 187                           int *got_frame_ptr, const struct AVPacket *avpkt);
>>
> 
> This is correct. It is not only the internal API which claims that, but
> the public API, too. And this just not honoured, in particular not in
> case of subtitle recoding. See
> https://github.com/mkver/FFmpeg/commit/ba1564c532654888015d67b70bf73d117c2d9f75
> 

It seems like there really are people who call this in a loop until all
the input is exhausted (as the documentation leads you to believe (The
internal uses of avcodec_decode_subtitle2 don't do this.)):
https://github.com/HandBrake/HandBrake/blob/a40fb6bce6755209461166140f131f26a2857eb9/libhb/decavsub.c#L335
I wonder if they ever got something meaningful the second time they
called it with the same packet (presuming there was a second time).

- Andreas
_______________________________________________
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 2/4] avcodec/ccaption_dec: check the length of packet and return used length

[lance.lmwang]

On Thu, May 12, 2022 at 06:29:51PM +0200, Andreas Rheinhardt wrote:
> Andreas Rheinhardt:
> > lance.lmwang@gmail.com:
> >> On Thu, May 12, 2022 at 08:25:29AM +0200, Paul B Mahol wrote:
> >>> On Thu, May 12, 2022 at 1:39 AM <lance.lmwang@gmail.com> wrote:
> >>>
> >>>> On Wed, May 11, 2022 at 09:47:52PM +0200, Paul B Mahol wrote:
> >>>>> why?
> >>>>
> >>>> assuming the len is 1, the following code will access the next 3
> >>>> array anymore, I think it's better to check the i with len -2.
> >>>>
> >>>> for (i = 0; i < len; i += 3) {
> >>>> to
> >>>> for (i = 0; i < len - 2; i += 3) {
> >>>>
> >>>> for the return, I think it's correct to return the used length,
> >>>> if it's error, have return already. right?
> >>>
> >>>
> >>> I doubt so.
> >>
> >> maybe I'm misunderstand it, but from the comments, the API claims that:
> >> libavcodec/codec_internal.h
> >> 175          * @return amount of bytes read from the packet on success,
> >> 176          *         negative error code on failure
> >> 177          */
> >> 178         int (*decode)(struct AVCodecContext *avctx, struct AVFrame *frame,
> >> 179                       int *got_frame_ptr, struct AVPacket *avpkt);
> >> 180         /**
> >> 181          * Decode subtitle data to an AVSubtitle.
> >> 182          * cb is in this state if cb_type is FF_CODEC_CB_TYPE_DECODE_SUB.
> >> 183          *
> >> 184          * Apart from that this is like the decode callback.
> >> 185          */
> >> 186         int (*decode_sub)(struct AVCodecContext *avctx, struct AVSubtitle *sub,
> >> 187                           int *got_frame_ptr, const struct AVPacket *avpkt);
> >>
> > 
> > This is correct. It is not only the internal API which claims that, but
> > the public API, too. And this just not honoured, in particular not in
> > case of subtitle recoding. See
> > https://github.com/mkver/FFmpeg/commit/ba1564c532654888015d67b70bf73d117c2d9f75
> > 
> 
> It seems like there really are people who call this in a loop until all
> the input is exhausted (as the documentation leads you to believe (The
> internal uses of avcodec_decode_subtitle2 don't do this.)):
> https://github.com/HandBrake/HandBrake/blob/a40fb6bce6755209461166140f131f26a2857eb9/libhb/decavsub.c#L335
> I wonder if they ever got something meaningful the second time they
> called it with the same packet (presuming there was a second time).

At first, I thought it was an unintentional return as all other subtitle decode
return packet size always. If the code don't support for that as claims by document,
then I prefer to fix the document. If not, we need to fix the code.


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

-- 
Thanks,
Limin Wang
_______________________________________________
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/sccenc: avoid potential invalid access

[lance.lmwang]

From: Limin Wang <lance.lmwang@gmail.com>

Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
---
 libavformat/sccenc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libavformat/sccenc.c b/libavformat/sccenc.c
index c8c4d097e4..2b924ba6e7 100644
--- a/libavformat/sccenc.c
+++ b/libavformat/sccenc.c
@@ -72,11 +72,11 @@ static int scc_write_packet(AVFormatContext *avf, AVPacket *pkt)
     s = (int)(pts /  1000) % 60;
     f = (int)(pts %  1000) / 33;
 
-    for (i = 0; i < pkt->size; i+=3) {
+    for (i = 0; i < pkt->size - 2; i+=3) {
         if (pkt->data[i] == 0xfc && ((pkt->data[i + 1] != 0x80 || pkt->data[i + 2] != 0x80)))
             break;
     }
-    if (i >= pkt->size)
+    if (i >= pkt->size - 2)
         return 0;
 
     if (!scc->inside && (scc->prev_h != h || scc->prev_m != m || scc->prev_s != s || scc->prev_f != f)) {
-- 
2.35.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 4/4] avcodec/dvdsubenc: return error if canvas_size is too small for subtitle render

[lance.lmwang]

From: Limin Wang <lance.lmwang@gmail.com>

Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
---
 libavcodec/dvdsubenc.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/libavcodec/dvdsubenc.c b/libavcodec/dvdsubenc.c
index fc3b7d1816..d29db7d49c 100644
--- a/libavcodec/dvdsubenc.c
+++ b/libavcodec/dvdsubenc.c
@@ -376,6 +376,12 @@ static int encode_dvd_subtitles(AVCodecContext *avctx,
     x2 = vrect.x + vrect.w - 1;
     y2 = vrect.y + vrect.h - 1;
 
+    if (x2 > avctx->width || y2 > avctx->height) {
+        av_log(avctx, AV_LOG_ERROR, "canvas_size(%d:%d) is too small(%d:%d) for render\n",
+               avctx->width, avctx->height, x2, y2);
+        ret = AVERROR(EINVAL);;
+        goto fail;
+    }
     *q++ = 0x05;
     // x1 x2 -> 6 nibbles
     *q++ = vrect.x >> 4;
-- 
2.35.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] avcodec/dvdsubenc: return error if canvas_size is too small for subtitle render

[lance.lmwang]

On Wed, May 11, 2022 at 10:48:18PM +0800, lance.lmwang@gmail.com wrote:
> From: Limin Wang <lance.lmwang@gmail.com>
> 
> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> ---
>  libavcodec/dvdsubenc.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/libavcodec/dvdsubenc.c b/libavcodec/dvdsubenc.c
> index fc3b7d1816..d29db7d49c 100644
> --- a/libavcodec/dvdsubenc.c
> +++ b/libavcodec/dvdsubenc.c
> @@ -376,6 +376,12 @@ static int encode_dvd_subtitles(AVCodecContext *avctx,
>      x2 = vrect.x + vrect.w - 1;
>      y2 = vrect.y + vrect.h - 1;
>  
> +    if (x2 > avctx->width || y2 > avctx->height) {
> +        av_log(avctx, AV_LOG_ERROR, "canvas_size(%d:%d) is too small(%d:%d) for render\n",
> +               avctx->width, avctx->height, x2, y2);
> +        ret = AVERROR(EINVAL);;
> +        goto fail;
> +    }
>      *q++ = 0x05;
>      // x1 x2 -> 6 nibbles
>      *q++ = vrect.x >> 4;
> -- 
> 2.35.1
> 
will apply the patchset 1,3,4 tomorrow if no other comments.

-- 
Thanks,
Limin Wang
_______________________________________________
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".