Skip to content
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

ImageDecoder libtiff implementation #1264

Merged
merged 14 commits into from
Sep 20, 2019

Conversation

jantonguirao
Copy link
Contributor

@jantonguirao jantonguirao commented Sep 17, 2019

Signed-off-by: Joaquin Anton janton@nvidia.com

Why we need this PR?

  • Need to support multichannel TIFFs

What happened in this PR?

  • Added a dependency with libtiff
  • Added copyright notices for libtiff
  • Added a libtiff based implementation of TIFF decoder
  • Determine whether to use libtiff based implementation or fallback to OpenCV depending of the image

JIRA TASK: [DALI-1038]

Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
@jantonguirao jantonguirao changed the title [WIP] ImageDecoder libtiff implementation ImageDecoder libtiff implementation Sep 19, 2019
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [907590]: BUILD STARTED

Signed-off-by: Joaquin Anton <janton@nvidia.com>
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [907594]: BUILD STARTED

Signed-off-by: Joaquin Anton <janton@nvidia.com>
@awolant awolant mentioned this pull request Sep 19, 2019
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [907594]: BUILD FAILED

@jantonguirao
Copy link
Contributor Author

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [907791]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [907791]: BUILD FAILED

Signed-off-by: Joaquin Anton <janton@nvidia.com>
Copy link
Member

@szalpal szalpal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's incorrect design here.
IMHO You should leverage the object-oriented design and do it like:

image_factory.cc

std::unique_ptr<Image>
ImageFactory::CreateImage(const uint8_t *encoded_image, size_t length, DALIImageType image_type) {
  [...]
  } else if (CheckIsTiff(encoded_image, length)) {
    if (DALI_USE_LIBTIFF)
            return std::make_unique<TiffImage_LibTiff>(encoded_image, length, image_type);
    return std::make_unique<TiffImage>(encoded_image, length, image_type);
  }
[...]
}

And create class TiffImage_LibTiff : public TiffImage. That way, PeekDims is the same for both implementation (which I think it should be), fallback for OpenCV is guaranteed and you only write DecodeImpl function.

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [907825]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [907825]: BUILD FAILED

Signed-off-by: Joaquin Anton <janton@nvidia.com>
@jantonguirao
Copy link
Contributor Author

That's incorrect design here.
IMHO You should leverage the object-oriented design and do it like:

image_factory.cc

std::unique_ptr<Image>
ImageFactory::CreateImage(const uint8_t *encoded_image, size_t length, DALIImageType image_type) {
  [...]
  } else if (CheckIsTiff(encoded_image, length)) {
    if (DALI_USE_LIBTIFF)
            return std::make_unique<TiffImage_LibTiff>(encoded_image, length, image_type);
    return std::make_unique<TiffImage>(encoded_image, length, image_type);
  }
[...]
}

And create class TiffImage_LibTiff : public TiffImage. That way, PeekDims is the same for both implementation (which I think it should be), fallback for OpenCV is guaranteed and you only write DecodeImpl function.

I agree. Fixed

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [908043]: BUILD STARTED

Signed-off-by: Joaquin Anton <janton@nvidia.com>
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [908055]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [908089]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [908055]: BUILD FAILED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [908110]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [908089]: BUILD FAILED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [908110]: BUILD FAILED

Signed-off-by: Joaquin Anton <janton@nvidia.com>
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [909512]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [909527]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [909536]: BUILD STARTED


const auto ifd_offset = buffer.Read<uint32_t>(4);
const auto entry_count = buffer.Read<uint16_t>(ifd_offset);
bool width_read = false, height_read = false;
size_t width, height;
size_t width = 0, height = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just change the type here to int64_t and avoid the cast in return?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will take care of it in #1280

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [909527]: BUILD FAILED


#define DALI_ERROR(str) \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either align the backslashes

if (a)      \
   asdf     \

or move them to one space after end of line

if (a) \
    asdf \

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do in #1280

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [909536]: BUILD FAILED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [909602]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [909602]: BUILD PASSED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [909708]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [909708]: BUILD FAILED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [909602]: BUILD PASSED

@jantonguirao jantonguirao merged commit bcf0d45 into NVIDIA:master Sep 20, 2019
00liujj pushed a commit to 00liujj/DALI that referenced this pull request Oct 10, 2019
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Jianjun Liu <00liujj@163.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants