[FFmpeg-devel] [PATCH v2 13/13] cbs_mpeg2.c: improve readability of start code search (part 3)

[Scott Theisen]

On 2/4/22 22:54, Andreas Rheinhardt wrote:
> I disagree that this is the true loop condition that just needs to be
> revealed; in fact, this is basically the loop condition from before
> fd93d5efe64206d5f1bce8c702602353444c0c1a, just with an added block to
> deal with size one units at the end. I considered this and chose the
> current one because it leads to less code duplication for this special case.
> Anyway, now that I have taken another look at this and I finally found
> the true loop condition: Is there a unit that we have not added to the
> fragment yet? This is easily implementable if one always resets the
> start code, so that there being an outstanding unit is equivalent to the
> start code being valid.

Looking at your patch series again, I agree your changes to cbs_mpeg2.c 
are clearer.

However, I think my changes from patch 11 are a further helpful 
clarification (ignoring the index and loop changes (since you already 
did that) and the "redundant" comment):

@@ -144,23 +144,24 @@  static int cbs_mpeg2_split_fragment(CodedBitstreamContext *ctx,
                                      CodedBitstreamFragment *frag,
                                      int header)
  {
- const uint8_t *start, *end;
+ const uint8_t *start = frag->data, *end;
+ const uint8_t * const buf_end = frag->data + frag->data_size;
      CodedBitstreamUnitType unit_type;
      uint32_t start_code = -1;
      size_t unit_size;
- int err, i = 0, final = 0;
+ int err, final = 0;
+ int i = -1; // offset for pre-increment
  
- start = avpriv_find_start_code(frag->data, frag->data + frag->data_size,
- &start_code, 1);
+ start = avpriv_find_start_code(start, buf_end, &start_code, 1);
      if (!avpriv_start_code_is_valid(start_code)) {
          // No start code found.
          return AVERROR_INVALIDDATA;
      }
  
- while (!final) {
+ do {
          unit_type = start_code & 0xff;
  
- if (start == frag->data + frag->data_size) {
+ if (start == buf_end) {
              // 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
@@ -168,10 +169,9 @@  static int cbs_mpeg2_split_fragment(CodedBitstreamContext *ctx,
              start_code = 0;
          }
  
- end = avpriv_find_start_code(start--, frag->data + frag->data_size,
- &start_code, 0);
-
- // start points to the byte containing the start_code_identifier
+ end = avpriv_find_start_code(start, buf_end, &start_code, 0);
+ start--;
+ // decrement so start points to the byte containing the 
start_code_identifier
          // (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).



Should I submit a v3 patch series which only includes patches 1-9? (That 
is only the avpriv_find_start_code() changes, since 10-13 were 
cbs_mpeg2.c and separate but related.)

Regards,
Scott