[FFmpeg-devel] [PATCH 2/4] avcodec/ccaption_dec: check the length of packet and return used length

[lance.lmwang at gmail.com]

On Thu, May 12, 2022 at 06:29:51PM +0200, Andreas Rheinhardt wrote:
> Andreas Rheinhardt:
> > lance.lmwang at 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 at 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 at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".

-- 
Thanks,
Limin Wang