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

Pranay Code Review #30

Open
wants to merge 35 commits into
base: master
Choose a base branch
from
Open

Pranay Code Review #30

wants to merge 35 commits into from

Conversation

mundrapranay
Copy link
Collaborator

No description provided.

Copy link
Member

@BrandonHaynes BrandonHaynes left a comment

Choose a reason for hiding this comment

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

Nice job, just some minor code cleanup left to go.

@@ -9,6 +9,10 @@ find_package(Python2 COMPONENTS Development)
file(GLOB LIGHTDB_CORE_INCLUDE_DIRS "${PROJECT_SOURCE_DIR}/../core/*/include/")
include_directories(${LIGHTDB_CORE_INCLUDE_DIRS} ${Boost_INCLUDE_DIRS} ${Python2_INCLUDE_DIRS})

# Include ipp
include_directories(/opt/intel/ipp/include)
Copy link
Member

Choose a reason for hiding this comment

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

Remove hardcoded paths

python/CMakeLists.txt Outdated Show resolved Hide resolved
python/include/PythonGreyscale.h Outdated Show resolved Hide resolved
python/include/PythonGreyscale.h Outdated Show resolved Hide resolved
python/include/PythonGreyscale.h Outdated Show resolved Hide resolved
boost::python::call<void>(callable_, swap);

// RGB -> NV12
CHECK_EQ(ippiRGBToYCbCr420_8u_C3P2R(rgb_.data(), channels * frame->width(), (Ipp8u*)y_out, frame->width(), (Ipp8u*)uv_out, frame->width(), size), ippStsNoErr);
Copy link
Member

Choose a reason for hiding this comment

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

reinterpret_cast

auto uv_out = y_out + frameSize_;

// NV12 -> RGB
CHECK_EQ(ippiYCbCr420ToRGB_8u_P2C3R((Ipp8u*)y_in, frame->width(), (Ipp8u*)uv_in, frame->width(), rgb_.data(), channels * frame->width(), size), ippStsNoErr);
Copy link
Member

Choose a reason for hiding this comment

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

reinterpret_cast

@@ -35,6 +35,10 @@ get_property(LIGHTDB_INCLUDES DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR} PROPERTY INC
string(REPLACE ";" "|" LIGHTDB_INCLUDES_STRING "${LIGHTDB_INCLUDES}")

foreach(LIGHTDB_PLUGIN_DIR ${LIGHTDB_PLUGIN_DIRS})
message("Lightdb plugin dir: ${LIGHTDB_PLUGIN_DIR}")
Copy link
Member

Choose a reason for hiding this comment

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

?

@@ -156,6 +171,15 @@ TEST_F(SelectionTestFixture, testTemporalThetaSelect) {
EXPECT_EQ(remove(Resources.out.hevc), 0);
}

TEST_F(SelectionTestFixture, testPythonQuery) {
Copy link
Member

Choose a reason for hiding this comment

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

?

@@ -27,7 +27,7 @@ TEST_F(UDFTestFixture, testGreyscale) {
auto query = Scan(Resources.red10.name)
.Map(lightdb::Greyscale)
.Encode()
.Save(Resources.out.hevc);
.Save("/home/maureen/grey_test.hevc");
Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Member

@BrandonHaynes BrandonHaynes left a comment

Choose a reason for hiding this comment

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

Super minor nits and this is ready to go.

python/CMakeLists.txt Outdated Show resolved Hide resolved
python/include/PythonUnary.h Outdated Show resolved Hide resolved
python/include/PythonUnary.h Outdated Show resolved Hide resolved
python/src/PythonUnary.cc Outdated Show resolved Hide resolved
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.

3 participants