Re: [FFmpeg-devel] [PATCH v2 01/13] avcodec/internal.h: create avpriv_start_code_is_valid()

[Andreas Rheinhardt <andreas.rheinhardt@outlook.com>]

Scott Theisen:
> ---
>  libavcodec/cavsdec.c               |  2 +-
>  libavcodec/cbs_mpeg2.c             |  4 ++--
>  libavcodec/extract_extradata_bsf.c |  2 +-
>  libavcodec/internal.h              | 15 +++++++++++++++
>  libavcodec/mpeg12.c                |  2 +-
>  libavcodec/mpeg12dec.c             |  2 +-
>  libavcodec/mpegvideo_parser.c      |  2 +-
>  libavcodec/remove_extradata_bsf.c  |  8 +++-----
>  libavcodec/vaapi_vc1.c             |  2 +-
>  libavcodec/vc1_common.h            |  4 +---
>  libavcodec/vc1dec.c                |  2 +-
>  libavformat/avs2dec.c              |  4 ++--
>  libavformat/avs3dec.c              |  4 ++--
>  libavformat/cavsvideodec.c         |  2 +-
>  libavformat/mpegvideodec.c         |  2 +-
>  libavformat/rtpenc_mpv.c           |  2 +-
>  16 files changed, 35 insertions(+), 24 deletions(-)
> 
> diff --git a/libavcodec/cavsdec.c b/libavcodec/cavsdec.c
> index 692c77eb39..a62177d520 100644
> --- a/libavcodec/cavsdec.c
> +++ b/libavcodec/cavsdec.c
> @@ -1249,7 +1249,7 @@ static int cavs_decode_frame(AVCodecContext *avctx, void *data, int *got_frame,
>      buf_end = buf + buf_size;
>      for(;;) {
>          buf_ptr = avpriv_find_start_code(buf_ptr, buf_end, &stc);
> -        if ((stc & 0xFFFFFE00) || buf_ptr == buf_end) {
> +        if (!avpriv_start_code_is_valid(stc) || buf_ptr == buf_end) {
>              if (!h->stc)
>                  av_log(h->avctx, AV_LOG_WARNING, "no frame decoded\n");
>              return FFMAX(0, buf_ptr - buf);
> diff --git a/libavcodec/cbs_mpeg2.c b/libavcodec/cbs_mpeg2.c
> index 26400f279f..eb45929132 100644
> --- a/libavcodec/cbs_mpeg2.c
> +++ b/libavcodec/cbs_mpeg2.c
> @@ -152,7 +152,7 @@ static int cbs_mpeg2_split_fragment(CodedBitstreamContext *ctx,
>  
>      start = avpriv_find_start_code(frag->data, frag->data + frag->data_size,
>                                     &start_code);
> -    if (start_code >> 8 != 0x000001) {
> +    if (!avpriv_start_code_is_valid(start_code)) {
>          // No start code found.
>          return AVERROR_INVALIDDATA;
>      }
> @@ -175,7 +175,7 @@ static int cbs_mpeg2_split_fragment(CodedBitstreamContext *ctx,
>          // (may be the last byte of fragment->data); end points to the byte
>          // following the byte containing the start code identifier (or to
>          // the end of fragment->data).
> -        if (start_code >> 8 == 0x000001) {
> +        if (avpriv_start_code_is_valid(start_code)) {
>              // Unit runs from start to the beginning of the start code
>              // pointed to by end (including any padding zeroes).
>              unit_size = (end - 4) - start;
> diff --git a/libavcodec/extract_extradata_bsf.c b/libavcodec/extract_extradata_bsf.c
> index dbcb8508b0..4df1c97139 100644
> --- a/libavcodec/extract_extradata_bsf.c
> +++ b/libavcodec/extract_extradata_bsf.c
> @@ -240,7 +240,7 @@ static int extract_extradata_vc1(AVBSFContext *ctx, AVPacket *pkt,
>          ptr = avpriv_find_start_code(ptr, end, &state);
>          if (state == VC1_CODE_SEQHDR || state == VC1_CODE_ENTRYPOINT) {
>              has_extradata = 1;
> -        } else if (has_extradata && IS_MARKER(state)) {
> +        } else if (has_extradata && avpriv_start_code_is_valid(state)) {
>              extradata_size = ptr - 4 - pkt->data;
>              break;
>          }
> diff --git a/libavcodec/internal.h b/libavcodec/internal.h
> index 72ca1553f6..005f308b70 100644
> --- a/libavcodec/internal.h
> +++ b/libavcodec/internal.h
> @@ -285,6 +285,21 @@ int ff_thread_can_start_frame(AVCodecContext *avctx);
>  
>  int avpriv_h264_has_num_reorder_frames(AVCodecContext *avctx);
>  
> +/**
> + * @brief Test whether a start code found by avpriv_find_start_code() is valid.
> + *
> + * Use this to test the validity of a start code especially if a start code can
> + * be at the end of the buffer, where testing the return value of avpriv_find_start_code()
> + * would incorrectly imply that the start code is invalid (since the returned value
> + * equals <b>@c end </b>).
> + *
> + * @param[in] start_code The start code to test.
> + * @return A boolean that is true if and only if <b>@p start_code</b> is valid
> + */
> +static av_always_inline int avpriv_start_code_is_valid(uint32_t start_code) {
> +    return (start_code & 0xFFFFFF00) == 0x100;
> +}
> +
>  const uint8_t *avpriv_find_start_code(const uint8_t *p,
>                                        const uint8_t *end,
>                                        uint32_t *state);
> diff --git a/libavcodec/mpeg12.c b/libavcodec/mpeg12.c
> index 58e03c05d4..e45bc74479 100644
> --- a/libavcodec/mpeg12.c
> +++ b/libavcodec/mpeg12.c
> @@ -217,7 +217,7 @@ int ff_mpeg1_find_frame_end(ParseContext *pc, const uint8_t *buf, int buf_size,
>                  pc->frame_start_found = 0;
>              if (pc->frame_start_found  < 4 && state == EXT_START_CODE)
>                  pc->frame_start_found++;
> -            if (pc->frame_start_found == 4 && (state & 0xFFFFFF00) == 0x100) {
> +            if (pc->frame_start_found == 4 && avpriv_start_code_is_valid(state)) {
>                  if (state < SLICE_MIN_START_CODE || state > SLICE_MAX_START_CODE) {
>                      pc->frame_start_found = 0;
>                      pc->state             = -1;
> diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c
> index 4a7bd6d466..65b66d11f8 100644
> --- a/libavcodec/mpeg12dec.c
> +++ b/libavcodec/mpeg12dec.c
> @@ -2477,7 +2477,7 @@ static int decode_chunks(AVCodecContext *avctx, AVFrame *picture,
>          /* find next start code */
>          uint32_t start_code = -1;
>          buf_ptr = avpriv_find_start_code(buf_ptr, buf_end, &start_code);
> -        if (start_code > 0x1ff) {
> +        if (!avpriv_start_code_is_valid(start_code)) {
>              if (!skip_frame) {
>                  if (HAVE_THREADS &&
>                      (avctx->active_thread_type & FF_THREAD_SLICE) &&
> diff --git a/libavcodec/mpegvideo_parser.c b/libavcodec/mpegvideo_parser.c
> index c5dc867d24..f0897e7e2c 100644
> --- a/libavcodec/mpegvideo_parser.c
> +++ b/libavcodec/mpegvideo_parser.c
> @@ -82,7 +82,7 @@ static int mpeg1_find_frame_end(ParseContext *pc, const uint8_t *buf,
>                  pc->frame_start_found = 0;
>              if (pc->frame_start_found  < 4 && state == EXT_START_CODE)
>                  pc->frame_start_found++;
> -            if (pc->frame_start_found == 4 && (state & 0xFFFFFF00) == 0x100) {
> +            if (pc->frame_start_found == 4 && avpriv_start_code_is_valid(state)) {
>                  if (state < SLICE_MIN_START_CODE || state > SLICE_MAX_START_CODE) {
>                      pc->frame_start_found = 0;
>                      pc->state             = -1;
> diff --git a/libavcodec/remove_extradata_bsf.c b/libavcodec/remove_extradata_bsf.c
> index 1d5f193f89..0e42174912 100644
> --- a/libavcodec/remove_extradata_bsf.c
> +++ b/libavcodec/remove_extradata_bsf.c
> @@ -35,8 +35,6 @@ enum RemoveFreq {
>      REMOVE_FREQ_NONKEYFRAME,
>  };
>  
> -#define START_CODE 0x000001
> -
>  typedef struct RemoveExtradataContext {
>      const AVClass *class;
>      int freq;
> @@ -73,7 +71,7 @@ static int h264_split(const uint8_t *buf, int buf_size)
>  
>      while (ptr < end) {
>          ptr = avpriv_find_start_code(ptr, end, &state);
> -        if ((state & 0xFFFFFF00) != 0x100)
> +        if (!avpriv_start_code_is_valid(state))
>              break;
>          nalu_type = state & 0x1F;
>          if (nalu_type == H264_NAL_SPS) {
> @@ -111,7 +109,7 @@ static int hevc_split(const uint8_t *buf, int buf_size)
>  
>      while (ptr < end) {
>          ptr = avpriv_find_start_code(ptr, end, &state);
> -        if ((state >> 8) != START_CODE)
> +        if (!avpriv_start_code_is_valid(state))
>              break;
>          nut = (state >> 1) & 0x3F;
>          if (nut == HEVC_NAL_VPS)
> @@ -171,7 +169,7 @@ static int vc1_split(const uint8_t *buf, int buf_size)
>          ptr = avpriv_find_start_code(ptr, end, &state);
>          if (state == VC1_CODE_SEQHDR || state == VC1_CODE_ENTRYPOINT) {
>              charged = 1;
> -        } else if (charged && IS_MARKER(state))
> +        } else if (charged && avpriv_start_code_is_valid(state))
>              return ptr - 4 - buf;
>      }
>  
> diff --git a/libavcodec/vaapi_vc1.c b/libavcodec/vaapi_vc1.c
> index 4e9607d9be..379104f688 100644
> --- a/libavcodec/vaapi_vc1.c
> +++ b/libavcodec/vaapi_vc1.c
> @@ -471,7 +471,7 @@ static int vaapi_vc1_decode_slice(AVCodecContext *avctx, const uint8_t *buffer,
>      int err;
>  
>      /* Current bit buffer is beyond any marker for VC-1, so skip it */
> -    if (avctx->codec_id == AV_CODEC_ID_VC1 && IS_MARKER(AV_RB32(buffer))) {
> +    if (avctx->codec_id == AV_CODEC_ID_VC1 && avpriv_start_code_is_valid(AV_RB32(buffer))) {
>          buffer += 4;
>          size -= 4;
>      }
> diff --git a/libavcodec/vc1_common.h b/libavcodec/vc1_common.h
> index b46c33f7e2..483f86a4ee 100644
> --- a/libavcodec/vc1_common.h
> +++ b/libavcodec/vc1_common.h
> @@ -41,8 +41,6 @@ enum VC1Code {
>  };
>  //@}
>  
> -#define IS_MARKER(x) (((x) & ~0xFF) == VC1_CODE_RES0)
> -
>  /** Available Profiles */
>  //@{
>  enum Profile {
> @@ -61,7 +59,7 @@ static av_always_inline const uint8_t* find_next_marker(const uint8_t *src, cons
>      if (end - src >= 4) {
>          uint32_t mrk = 0xFFFFFFFF;
>          src = avpriv_find_start_code(src, end, &mrk);
> -        if (IS_MARKER(mrk))
> +        if (avpriv_start_code_is_valid(mrk))
>              return src - 4;
>      }
>      return end;
> diff --git a/libavcodec/vc1dec.c b/libavcodec/vc1dec.c
> index 7ed5133cfa..86749e5973 100644
> --- a/libavcodec/vc1dec.c
> +++ b/libavcodec/vc1dec.c
> @@ -664,7 +664,7 @@ static int vc1_decode_frame(AVCodecContext *avctx, void *data,
>          if (!buf2)
>              return AVERROR(ENOMEM);
>  
> -        if (IS_MARKER(AV_RB32(buf))) { /* frame starts with marker and needs to be parsed */
> +        if (avpriv_start_code_is_valid(AV_RB32(buf))) { /* frame starts with marker and needs to be parsed */
>              const uint8_t *start, *end, *next;
>              int size;
>  
> diff --git a/libavformat/avs2dec.c b/libavformat/avs2dec.c
> index 51908d2b63..bdeb746fc7 100644
> --- a/libavformat/avs2dec.c
> +++ b/libavformat/avs2dec.c
> @@ -42,8 +42,8 @@ static int avs2_probe(const AVProbeData *p)
>  
>      while (ptr < end) {
>          ptr = avpriv_find_start_code(ptr, end, &code);
> -        state = code & 0xFF;
> -        if ((code & 0xffffff00) == 0x100) {
> +        if (avpriv_start_code_is_valid(code)) {
> +            state = code & 0xFF;
>              if (AVS2_ISUNIT(state)) {
>                  if (sqb && !hds) {
>                      hds = ptr - sqb;
> diff --git a/libavformat/avs3dec.c b/libavformat/avs3dec.c
> index 253caa7c1d..2daccd3d15 100644
> --- a/libavformat/avs3dec.c
> +++ b/libavformat/avs3dec.c
> @@ -36,8 +36,8 @@ static int avs3video_probe(const AVProbeData *p)
>  
>      while (ptr < end) {
>          ptr = avpriv_find_start_code(ptr, end, &code);
> -        state = code & 0xFF;
> -        if ((code & 0xFFFFFF00) == 0x100) {
> +        if (avpriv_start_code_is_valid(code)) {
> +            state = code & 0xFF;
>              if (state < AVS3_SEQ_START_CODE) {
>                  if (code < slice_pos)
>                      return 0;
> diff --git a/libavformat/cavsvideodec.c b/libavformat/cavsvideodec.c
> index 8900b97597..1d007708cc 100644
> --- a/libavformat/cavsvideodec.c
> +++ b/libavformat/cavsvideodec.c
> @@ -38,7 +38,7 @@ static int cavsvideo_probe(const AVProbeData *p)
>  
>      while (ptr < end) {
>          ptr = avpriv_find_start_code(ptr, end, &code);
> -        if ((code & 0xffffff00) == 0x100) {
> +        if (avpriv_start_code_is_valid(code)) {
>              if(code < CAVS_SEQ_START_CODE) {
>                  /* slices have to be consecutive */
>                  if(code < slice_pos)
> diff --git a/libavformat/mpegvideodec.c b/libavformat/mpegvideodec.c
> index 2d6f81aaa1..a9829dc1df 100644
> --- a/libavformat/mpegvideodec.c
> +++ b/libavformat/mpegvideodec.c
> @@ -44,7 +44,7 @@ static int mpegvideo_probe(const AVProbeData *p)
>  
>      while (ptr < end) {
>          ptr = avpriv_find_start_code(ptr, end, &code);
> -        if ((code & 0xffffff00) == 0x100) {
> +        if (avpriv_start_code_is_valid(code)) {
>              switch(code){
>              case     SEQ_START_CODE:
>                  if (!(ptr[3 + 1 + 2] & 0x20))
> diff --git a/libavformat/rtpenc_mpv.c b/libavformat/rtpenc_mpv.c
> index 4b45f51772..05a77fa11c 100644
> --- a/libavformat/rtpenc_mpv.c
> +++ b/libavformat/rtpenc_mpv.c
> @@ -57,7 +57,7 @@ void ff_rtp_send_mpegvideo(AVFormatContext *s1, const uint8_t *buf1, int size)
>              while (1) {
>                  start_code = -1;
>                  r = avpriv_find_start_code(r1, end, &start_code);
> -                if((start_code & 0xFFFFFF00) == 0x100) {
> +                if (avpriv_start_code_is_valid(start_code)) {
>                      /* New start code found */
>                      if (start_code == 0x100) {
>                          frame_type = (r[1] & 0x38) >> 3;

a) We use the avpriv prefix for things that should be exported from one
library to be used in other libraries, but not for public use. Therefore
the avpriv prefix is inappropriate here as this function is static. And
it makes a very long name.
b) internal.h is the wrong header for this: There are more start codes
than 00 00 01. That's why I sent the patch to move
avpriv_find_start_code() to libavcodec/startcode.h.
c) I am not sure that the new code is equivalent to the old one in all
instances: mpeg12dec.c checks for "start_code > 0x1ff" to mean "no valid
start code", yet if the buffer ended with anything in the range
0x00-0xff it would be considered a start code before this patch and now
it would no longer be a start code. I don't think this is a bad change,
though, but it should be noted in the commit message.

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