New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix issues in BitstreamConverter #23174
base: master
Are you sure you want to change the base?
Conversation
…hmetic issue causing an overrun
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know there is a lot of legacy code going on here and a lot of C-isms but it would be great if you could update the PR to use current standards. I would also love it if we could improve the class as a whole by using human readable parsing, removing goto's and the lot.
@@ -726,6 +727,18 @@ bool CBitstreamConverter::BitstreamConvertInitAVC(void *in_extradata, int in_ext | |||
if (unit_nb) | |||
pps_seen = 1; | |||
} | |||
|
|||
if (!unit_nb && !mvc_done++) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know you were following the pattern but generally ints either need to be compared directly or casted to bool. I also recommend using a bool when wanting a boolean check.
if (!unit_nb && !mvc_done++) | |
if (unit_nb == 0 && !mvc_done) |
@@ -678,6 +678,7 @@ bool CBitstreamConverter::BitstreamConvertInitAVC(void *in_extradata, int in_ext | |||
uint16_t unit_size; | |||
uint32_t total_size = 0; | |||
uint8_t *out = NULL, unit_nb, sps_done = 0, sps_seen = 0, pps_seen = 0; | |||
uint8_t mvc_done = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uint8_t mvc_done = 0; | |
bool mvcDone{false}; |
unit_nb = *extradata++ & 0x1f; // number of sps unit(s) | ||
sps_done = 0; | ||
pps_seen = 0; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | |
mvcDone = true; | |
} |
|
||
if (!unit_nb && !mvc_done++) | ||
{ | ||
if (in_extrasize - total_size > 14 && memcmp(extradata + 8, "mvcC", 4) == 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally magic numbers are frowned upon. Please use constexpr int's to help with human readable values. In reality it would just be nice if we could define a structure for the extradata so we don't need to do all this magic number jumping around but I'll leave that as a future wish.
if (in_extrasize - total_size > 14 && memcmp(extradata + 8, "mvcC", 4) == 0) | |
constexpr int MINIMUM_SIZE{14}; | |
constexpr int HEADER_OFFSET{8}; | |
if (in_extrasize - total_size > MINIMUM_SIZE && memcmp(extradata + HEADER_OFFSET, "mvcC", strlen("mvcC")) == 0) |
// start over; take SPS and PPS from the mvcC atom | ||
extradata += 12 + 5; // skip over mvcC atom header | ||
unit_nb = *extradata++ & 0x1f; // number of sps unit(s) | ||
sps_done = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be a gold star if you could (possibly in a future PR) convert these to bool.
{ | ||
// start over; take SPS and PPS from the mvcC atom | ||
extradata += 12 + 5; // skip over mvcC atom header | ||
unit_nb = *extradata++ & 0x1f; // number of sps unit(s) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more magic?
if (in_extrasize - total_size > 14 && memcmp(extradata + 8, "mvcC", 4) == 0) | ||
{ | ||
// start over; take SPS and PPS from the mvcC atom | ||
extradata += 12 + 5; // skip over mvcC atom header |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
magic everywhere!
I've made some formatting changes to meet the current code style. The diffs are available in the following links:
For more information please see our current code style guidelines. |
Description
This fixes a pointer arithmetic issue and also parses mvcC atom for 3D playback.
Motivation and context
How has this been tested?
This has been tested on OSMC Vero 4K / 4K + devices since 2020.
What is the effect on users?
Screenshots (if appropriate):
Types of change
Checklist: