| Message ID | 20220208203214.161469-1-scott.the.elm@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] cbs_mpeg2_split_fragment(): cache the buffer end | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
| andriy/make_ppc | success | Make finished |
| andriy/make_fate_ppc | success | Make fate finished |
Ping; it still applies cleanly. -Scott Theisen On 2/8/22 15:32, Scott Theisen wrote: > Also add a few clarifying comments. > --- > libavcodec/cbs_mpeg2.c | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/libavcodec/cbs_mpeg2.c b/libavcodec/cbs_mpeg2.c > index 33bd3e0998..47732562d1 100644 > --- a/libavcodec/cbs_mpeg2.c > +++ b/libavcodec/cbs_mpeg2.c > @@ -144,12 +144,12 @@ static int cbs_mpeg2_split_fragment(CodedBitstreamContext *ctx, > CodedBitstreamFragment *frag, > int header) > { > - const uint8_t *start; > + const uint8_t *start = frag->data; > + const uint8_t * const buf_end = frag->data + frag->data_size; > uint32_t start_code = -1; > int err; > > - start = avpriv_find_start_code(frag->data, frag->data + frag->data_size, > - &start_code); > + start = avpriv_find_start_code(start, buf_end, &start_code); > if (start_code >> 8 != 0x000001) { > // No start code found. > return AVERROR_INVALIDDATA; > @@ -165,12 +165,11 @@ static int cbs_mpeg2_split_fragment(CodedBitstreamContext *ctx, > // start code in any way (as e.g. happens when there is a > // Sequence End unit at the very end of a packet). > start_code = UINT32_MAX; > - end = avpriv_find_start_code(start--, frag->data + frag->data_size, > - &start_code); > - > - // start points to the byte containing the start_code_identifier > + end = avpriv_find_start_code(start, buf_end, &start_code); > + 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 > + // following the byte containing the next start code identifier (or to > // the end of fragment->data). > if (start_code >> 8 == 0x000001) { > // Unit runs from start to the beginning of the start code > @@ -178,6 +177,7 @@ static int cbs_mpeg2_split_fragment(CodedBitstreamContext *ctx, > unit_size = (end - 4) - start; > } else { > // We didn't find a start code, so this is the final unit. > + // There is no start code to remove from end, hence not (end - 4). > unit_size = end - start; > } >
Ping; it still applies cleanly. -Scott Theisen On 5/15/22 14:50, Scott Theisen wrote: > Ping; it still applies cleanly. > > -Scott Theisen > > On 2/8/22 15:32, Scott Theisen wrote: >> Also add a few clarifying comments. >> --- >> libavcodec/cbs_mpeg2.c | 16 ++++++++-------- >> 1 file changed, 8 insertions(+), 8 deletions(-) >> >> diff --git a/libavcodec/cbs_mpeg2.c b/libavcodec/cbs_mpeg2.c >> index 33bd3e0998..47732562d1 100644 >> --- a/libavcodec/cbs_mpeg2.c >> +++ b/libavcodec/cbs_mpeg2.c >> @@ -144,12 +144,12 @@ static int >> cbs_mpeg2_split_fragment(CodedBitstreamContext *ctx, >> CodedBitstreamFragment *frag, >> int header) >> { >> - const uint8_t *start; >> + const uint8_t *start = frag->data; >> + const uint8_t * const buf_end = frag->data + frag->data_size; >> uint32_t start_code = -1; >> int err; >> - start = avpriv_find_start_code(frag->data, frag->data + >> frag->data_size, >> - &start_code); >> + start = avpriv_find_start_code(start, buf_end, &start_code); >> if (start_code >> 8 != 0x000001) { >> // No start code found. >> return AVERROR_INVALIDDATA; >> @@ -165,12 +165,11 @@ static int >> cbs_mpeg2_split_fragment(CodedBitstreamContext *ctx, >> // start code in any way (as e.g. happens when there is a >> // Sequence End unit at the very end of a packet). >> start_code = UINT32_MAX; >> - end = avpriv_find_start_code(start--, frag->data + >> frag->data_size, >> - &start_code); >> - >> - // start points to the byte containing the >> start_code_identifier >> + end = avpriv_find_start_code(start, buf_end, &start_code); >> + 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 >> + // following the byte containing the next start code >> identifier (or to >> // the end of fragment->data). >> if (start_code >> 8 == 0x000001) { >> // Unit runs from start to the beginning of the start code >> @@ -178,6 +177,7 @@ static int >> cbs_mpeg2_split_fragment(CodedBitstreamContext *ctx, >> unit_size = (end - 4) - start; >> } else { >> // We didn't find a start code, so this is the final unit. >> + // There is no start code to remove from end, hence not >> (end - 4). >> unit_size = end - start; >> } >
Quoting Scott Theisen (2022-09-16 19:41:43)
> Ping; it still applies cleanly.
Missing motivation for this change. Is it faster, or why is the new code
better?
On 9/22/22 10:59, Anton Khirnov wrote: > Quoting Scott Theisen (2022-09-16 19:41:43) >> Ping; it still applies cleanly. > Missing motivation for this change. Is it faster, or why is the new code > better? > To make it easier to read. `buf_end` is shorter than `frag->data + frag->data_size`. Initialize `start` to `frag->data` so the calls to avpriv_find_start_code are the same. I realize readability is an opinion, but I felt that saving the buffer end to a variable and tweaking the comments increased the readability of the code. Regards, Scott
diff --git a/libavcodec/cbs_mpeg2.c b/libavcodec/cbs_mpeg2.c index 33bd3e0998..47732562d1 100644 --- a/libavcodec/cbs_mpeg2.c +++ b/libavcodec/cbs_mpeg2.c @@ -144,12 +144,12 @@ static int cbs_mpeg2_split_fragment(CodedBitstreamContext *ctx, CodedBitstreamFragment *frag, int header) { - const uint8_t *start; + const uint8_t *start = frag->data; + const uint8_t * const buf_end = frag->data + frag->data_size; uint32_t start_code = -1; int err; - start = avpriv_find_start_code(frag->data, frag->data + frag->data_size, - &start_code); + start = avpriv_find_start_code(start, buf_end, &start_code); if (start_code >> 8 != 0x000001) { // No start code found. return AVERROR_INVALIDDATA; @@ -165,12 +165,11 @@ static int cbs_mpeg2_split_fragment(CodedBitstreamContext *ctx, // start code in any way (as e.g. happens when there is a // Sequence End unit at the very end of a packet). start_code = UINT32_MAX; - end = avpriv_find_start_code(start--, frag->data + frag->data_size, - &start_code); - - // start points to the byte containing the start_code_identifier + end = avpriv_find_start_code(start, buf_end, &start_code); + 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 + // following the byte containing the next start code identifier (or to // the end of fragment->data). if (start_code >> 8 == 0x000001) { // Unit runs from start to the beginning of the start code @@ -178,6 +177,7 @@ static int cbs_mpeg2_split_fragment(CodedBitstreamContext *ctx, unit_size = (end - 4) - start; } else { // We didn't find a start code, so this is the final unit. + // There is no start code to remove from end, hence not (end - 4). unit_size = end - start; }