Re: [FFmpeg-devel] [PATCH 2/4] avcodec/ccaption_dec: check the length of packet and return used length
On Thu, May 12, 2022 at 06:29:51PM +0200, Andreas Rheinhardt wrote:
> Andreas Rheinhardt:
> > lance.lmw...@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 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-requ...@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-requ...@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.lmw...@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 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-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 2/4] avcodec/ccaption_dec: check the length of packet and return used length
lance.lmw...@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 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-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 2/4] avcodec/ccaption_dec: check the length of packet and return used length
On Thu, May 12, 2022 at 08:25:29AM +0200, Paul B Mahol wrote:
> On Thu, May 12, 2022 at 1:39 AM 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-requ...@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-requ...@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-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 2/4] avcodec/ccaption_dec: check the length of packet and return used length
On Thu, May 12, 2022 at 1:39 AM 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-requ...@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-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 2/4] avcodec/ccaption_dec: check the length of packet and return used length
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-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 2/4] avcodec/ccaption_dec: check the length of packet and return used length
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-requ...@ffmpeg.org with subject "unsubscribe".
