[FFmpeg-devel,3/8] avpriv_find_start_code(): make the state parameter output only

---
 libavcodec/cbs_mpeg2.c                | 8 --------
 libavcodec/mpeg12dec.c                | 5 ++---
 libavcodec/mpeg4_unpack_bframes_bsf.c | 1 -
 libavcodec/mpegvideo_parser.c         | 3 +--
 libavcodec/utils.c                    | 1 +
 libavformat/rtpenc_mpv.c              | 3 +--
 6 files changed, 5 insertions(+), 16 deletions(-)

Scott Theisen:
> ---
>  libavcodec/cbs_mpeg2.c                | 8 --------
>  libavcodec/mpeg12dec.c                | 5 ++---
>  libavcodec/mpeg4_unpack_bframes_bsf.c | 1 -
>  libavcodec/mpegvideo_parser.c         | 3 +--
>  libavcodec/utils.c                    | 1 +
>  libavformat/rtpenc_mpv.c              | 3 +--
>  6 files changed, 5 insertions(+), 16 deletions(-)
> 
> diff --git a/libavcodec/cbs_mpeg2.c b/libavcodec/cbs_mpeg2.c
> index 26400f279f..03ea49cbca 100644
> --- a/libavcodec/cbs_mpeg2.c
> +++ b/libavcodec/cbs_mpeg2.c
> @@ -160,14 +160,6 @@ static int cbs_mpeg2_split_fragment(CodedBitstreamContext *ctx,
>      for (i = 0;; i++) {
>          unit_type = start_code & 0xff;
>  
> -        if (start == frag->data + frag->data_size) {
> -            // The last four bytes form a start code which constitutes
> -            // a unit of its own.  In this situation avpriv_find_start_code
> -            // won't modify start_code at all so modify start_code so that
> -            // the next unit will be treated as the last unit.
> -            start_code = 0;
> -        }
> -
>          end = avpriv_find_start_code(start--, frag->data + frag->data_size,
>                                       &start_code);
>  
> diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c
> index 4a7bd6d466..1110fcb319 100644
> --- a/libavcodec/mpeg12dec.c
> +++ b/libavcodec/mpeg12dec.c
> @@ -1770,7 +1770,7 @@ static int mpeg_decode_slice(MpegEncContext *s, int mb_y,
>  
>      if (avctx->hwaccel && avctx->hwaccel->decode_slice) {
>          const uint8_t *buf_end, *buf_start = *buf - 4; /* include start_code */
> -        int start_code = -1;
> +        uint32_t start_code;
>          buf_end = avpriv_find_start_code(buf_start + 2, *buf + buf_size, &start_code);
>          if (buf_end < *buf + buf_size)
>              buf_end -= 4;
> @@ -2020,7 +2020,6 @@ static int slice_decode_thread(AVCodecContext *c, void *arg)
>          if (s->mb_y == s->end_mb_y)
>              return 0;
>  
> -        start_code = -1;
>          buf        = avpriv_find_start_code(buf, s->gb.buffer_end, &start_code);
>          if (start_code < SLICE_MIN_START_CODE || start_code > SLICE_MAX_START_CODE)
>              return AVERROR_INVALIDDATA;
> @@ -2475,7 +2474,7 @@ static int decode_chunks(AVCodecContext *avctx, AVFrame *picture,
>  
>      for (;;) {
>          /* find next start code */
> -        uint32_t start_code = -1;
> +        uint32_t start_code;
>          buf_ptr = avpriv_find_start_code(buf_ptr, buf_end, &start_code);
>          if (start_code > 0x1ff) {
>              if (!skip_frame) {
> diff --git a/libavcodec/mpeg4_unpack_bframes_bsf.c b/libavcodec/mpeg4_unpack_bframes_bsf.c
> index 6f8595713d..8b3fda0b03 100644
> --- a/libavcodec/mpeg4_unpack_bframes_bsf.c
> +++ b/libavcodec/mpeg4_unpack_bframes_bsf.c
> @@ -36,7 +36,6 @@ static void scan_buffer(const uint8_t *buf, int buf_size,
>      const uint8_t *end = buf + buf_size, *pos = buf;
>  
>      while (pos < end) {
> -        startcode = -1;
>          pos = avpriv_find_start_code(pos, end, &startcode);
>  
>          if (startcode == USER_DATA_STARTCODE && pos_p) {
> diff --git a/libavcodec/mpegvideo_parser.c b/libavcodec/mpegvideo_parser.c
> index c5dc867d24..c991a82405 100644
> --- a/libavcodec/mpegvideo_parser.c
> +++ b/libavcodec/mpegvideo_parser.c
> @@ -105,7 +105,6 @@ static void mpegvideo_extract_headers(AVCodecParserContext *s,
>  {
>      struct MpvParseContext *pc = s->priv_data;
>      const uint8_t *buf_end = buf + buf_size;
> -    uint32_t start_code;
>      int frame_rate_index, ext_type, bytes_left;
>      int frame_rate_ext_n, frame_rate_ext_d;
>      int top_field_first, repeat_first_field, progressive_frame;
> @@ -120,7 +119,7 @@ static void mpegvideo_extract_headers(AVCodecParserContext *s,
>      s->repeat_pict = 0;
>  
>      while (buf < buf_end) {
> -        start_code= -1;
> +        uint32_t start_code;
>          buf= avpriv_find_start_code(buf, buf_end, &start_code);
>          bytes_left = buf_end - buf;
>          switch(start_code) {
> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> index 882f90be79..cf88e0a759 100644
> --- a/libavcodec/utils.c
> +++ b/libavcodec/utils.c
> @@ -950,6 +950,7 @@ const uint8_t *avpriv_find_start_code(const uint8_t *av_restrict p,
>      if (p >= end)
>          return end;
>  
> +    *state = ~0;
>      for (i = 0; i < 3; i++) {
>          uint32_t tmp = *state << 8;
>          *state = tmp + *(p++);
> diff --git a/libavformat/rtpenc_mpv.c b/libavformat/rtpenc_mpv.c
> index 4b45f51772..bb63c9bdc6 100644
> --- a/libavformat/rtpenc_mpv.c
> +++ b/libavformat/rtpenc_mpv.c
> @@ -51,11 +51,10 @@ void ff_rtp_send_mpegvideo(AVFormatContext *s1, const uint8_t *buf1, int size)
>              end_of_slice = 1;
>          } else {
>              const uint8_t *r, *r1;
> -            int start_code;
>  
>              r1 = buf1;
>              while (1) {
> -                start_code = -1;
> +                uint32_t start_code;
>                  r = avpriv_find_start_code(r1, end, &start_code);
>                  if((start_code & 0xFFFFFF00) == 0x100) {
>                      /* New start code found */

It seems you didn't get why the code currently is as it is: It allows to
use this function with non-contiguous input (as happens in our parsers
which operate on data that is not cleanly split into packets yet (it is
their job to create proper packets out of the input)). As an example,
say you have a buffer that ends with 0x00 00 and the next buffer
starting with 0x01 01. Then avpriv_find_start_code() will detect a start
code at the beginning of the second buffer if the user passes in the
same state again (and does not modify it in the meantime).
You would have noticed this if you had run FATE on your patches (see
https://ffmpeg.org/fate.html#Using-FATE-from-your-FFmpeg-source-directory).

- Andreas

On 2/1/22 19:15, Andreas Rheinhardt wrote:
> It seems you didn't get why the code currently is as it is: It allows to
> use this function with non-contiguous input (as happens in our parsers
> which operate on data that is not cleanly split into packets yet (it is
> their job to create proper packets out of the input)). As an example,
> say you have a buffer that ends with 0x00 00 and the next buffer
> starting with 0x01 01. Then avpriv_find_start_code() will detect a start
> code at the beginning of the second buffer if the user passes in the
> same state again (and does not modify it in the meantime).

Without documentation or comments, in/out parameters are hard to infer 
and most uses were only out.

I might make a context free version for when the buffer is already 
packetized properly.

> You would have noticed this if you had run FATE on your patches (see
> https://ffmpeg.org/fate.html#Using-FATE-from-your-FFmpeg-source-directory).

Thanks for the link, I didn't see how to run FATE or get the samples for it.

Unfortunately, FATE fails before that:
```
CC    tests/api/api-threadmessage-test.o
LD    tests/api/api-threadmessage-test
TEST    api-threadmessage
Test api-threadmessage failed. Look at 
tests/data/fate/api-threadmessage.err for details.
make: *** [tests/Makefile:257: fate-api-threadmessage] Error 127
```

```
cat tests/data/fate/api-threadmessage.err
/home/htpc/build/mythtv-ffmpeg/FFmpeg/tests/api/api-threadmessage-test: 
error while loading shared libraries: libavutil.so.57: cannot open 
shared object file: No such file or directory
```

Which is strange because it does exist.
```
ls -l libavutil/libavutil*
-rw-rw-r-- 1 htpc htpc 4426450 Feb  1 22:06 libavutil/libavutil.a
-rw-rw-r-- 1 htpc htpc     353 Feb  1 21:06 libavutil/libavutil.pc
lrwxrwxrwx 1 htpc htpc      15 Feb  1 22:04 libavutil/libavutil.so -> 
libavutil.so.57
-rwxrwxr-x 1 htpc htpc 2602848 Feb  1 22:04 libavutil/libavutil.so.57
-rw-rw-r-- 1 htpc htpc      68 Nov  1 22:34 libavutil/libavutil.v
-rw-rw-r-- 1 htpc htpc      65 Feb  1 22:04 libavutil/libavutil.ver
-rw-rw-r-- 1 htpc htpc      82 Feb  1 21:05 libavutil/libavutil.version
```

I run configure with:
./configure --sysinclude=/usr/include --cc='ccache gcc' --cxx='ccache 
g++' --prefix=/usr/local --libdir=/usr/local/lib --enable-vdpau 
--enable-libxml2 --enable-libass --enable-libbluray --enable-vaapi 
--enable-libdrm --enable-gnutls --disable-stripping --disable-manpages 
--disable-podpages --disable-doc --disable-nvenc --enable-shared 
--disable-static --enable-gpl --enable-pic

Any idea why it's failing?

Thanks,
Scott

Scott Theisen:
> On 2/1/22 19:15, Andreas Rheinhardt wrote:
>> It seems you didn't get why the code currently is as it is: It allows to
>> use this function with non-contiguous input (as happens in our parsers
>> which operate on data that is not cleanly split into packets yet (it is
>> their job to create proper packets out of the input)). As an example,
>> say you have a buffer that ends with 0x00 00 and the next buffer
>> starting with 0x01 01. Then avpriv_find_start_code() will detect a start
>> code at the beginning of the second buffer if the user passes in the
>> same state again (and does not modify it in the meantime).
> 
> Without documentation or comments, in/out parameters are hard to infer
> and most uses were only out.
> 
> I might make a context free version for when the buffer is already
> packetized properly.
> 
>> You would have noticed this if you had run FATE on your patches (see
>> https://ffmpeg.org/fate.html#Using-FATE-from-your-FFmpeg-source-directory).
>>
> 
> Thanks for the link, I didn't see how to run FATE or get the samples for
> it.
> 
> Unfortunately, FATE fails before that:
> ```
> CC    tests/api/api-threadmessage-test.o
> LD    tests/api/api-threadmessage-test
> TEST    api-threadmessage
> Test api-threadmessage failed. Look at
> tests/data/fate/api-threadmessage.err for details.
> make: *** [tests/Makefile:257: fate-api-threadmessage] Error 127
> ```
> 
> ```
> cat tests/data/fate/api-threadmessage.err
> /home/htpc/build/mythtv-ffmpeg/FFmpeg/tests/api/api-threadmessage-test:
> error while loading shared libraries: libavutil.so.57: cannot open
> shared object file: No such file or directory
> ```
> 
> Which is strange because it does exist.
> ```
> ls -l libavutil/libavutil*
> -rw-rw-r-- 1 htpc htpc 4426450 Feb  1 22:06 libavutil/libavutil.a
> -rw-rw-r-- 1 htpc htpc     353 Feb  1 21:06 libavutil/libavutil.pc
> lrwxrwxrwx 1 htpc htpc      15 Feb  1 22:04 libavutil/libavutil.so ->
> libavutil.so.57
> -rwxrwxr-x 1 htpc htpc 2602848 Feb  1 22:04 libavutil/libavutil.so.57
> -rw-rw-r-- 1 htpc htpc      68 Nov  1 22:34 libavutil/libavutil.v
> -rw-rw-r-- 1 htpc htpc      65 Feb  1 22:04 libavutil/libavutil.ver
> -rw-rw-r-- 1 htpc htpc      82 Feb  1 21:05 libavutil/libavutil.version
> ```
> 
> I run configure with:
> ./configure --sysinclude=/usr/include --cc='ccache gcc' --cxx='ccache
> g++' --prefix=/usr/local --libdir=/usr/local/lib --enable-vdpau
> --enable-libxml2 --enable-libass --enable-libbluray --enable-vaapi
> --enable-libdrm --enable-gnutls --disable-stripping --disable-manpages
> --disable-podpages --disable-doc --disable-nvenc --enable-shared
> --disable-static --enable-gpl --enable-pic
> 
> Any idea why it's failing?
> 

libavutil.so.57 (and the other libraries) are probably not in your
library search path. You might e.g. set an rpath.

- Andreas