-
Notifications
You must be signed in to change notification settings - Fork 611
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
Add parsing of number of channels in JpegImage::PeekShapeImpl #1306
Add parsing of number of channels in JpegImage::PeekShapeImpl #1306
Conversation
!build |
CI MESSAGE: [918658]: BUILD STARTED |
CI MESSAGE: [918658]: BUILD PASSED |
!build |
CI MESSAGE: [918856]: BUILD STARTED |
CI MESSAGE: [918856]: BUILD FAILED |
53ccfca
to
5171dba
Compare
!build |
CI MESSAGE: [918888]: BUILD STARTED |
CI MESSAGE: [918888]: BUILD PASSED |
dali/image/jpeg.cc
Outdated
// Check for valid JPEG image | ||
unsigned int i = 0; // Keeps track of the position within the file | ||
if (data[i] == 0xFF && data[i + 1] == 0xD8) { | ||
i += 4; | ||
// Retrieve the block length of the first block since | ||
// the first block will not contain the size of file | ||
uint16_t block_length = data[i] * 256 + data[i + 1]; | ||
uint16_t block_length = (data[i] << 8) + data[i + 1]; |
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 would prefer to use '*' instead of >>
or <<
, as of someone silently change unsigned to signed of the target variable >>
will start to behave not as expected, while the compiler is smart enough to optimize *256
to appropriate shift operation.
@mzient - what is your opinion?
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.
We're not really multiplying here - we're inserting bytes to a desired position, so bit shift is IMO the right choice (BTW - left shift does not depend on signedness). What we could change is to replace +
with |
, to emphasize that we're dealing with bits.
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.
For the record I plan to use the utilities in https://github.com/NVIDIA/DALI/pull/1310/files as soon as that PR gets merged
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
5171dba
to
21e97b4
Compare
Signed-off-by: Joaquin Anton <janton@nvidia.com>
!build |
CI MESSAGE: [924774]: BUILD STARTED |
CI MESSAGE: [924774]: BUILD PASSED |
…#1306) Signed-off-by: Joaquin Anton <janton@nvidia.com> Signed-off-by: Jianjun Liu <00liujj@163.com>
Signed-off-by: Joaquin Anton janton@nvidia.com
Why we need this PR?
JpegImage::PeekShapeImpl harcodes C = 0 when libjpeg-turbo support is disabled
What happened in this PR?
Add parsing of number of channels
JIRA TASK: [DALI-XXXX]