From e14a6a3be915fa170fec7bd3fe572b40dfa68227 Mon Sep 17 00:00:00 2001 From: Anish Goyal Date: Mon, 10 Jun 2024 14:02:14 -0400 Subject: [PATCH] Address PR comments --- src/ppx/graphics_util.cpp | 57 ++++++++++++++++++++---------------- src/ppx/grfx/grfx_format.cpp | 10 +++---- 2 files changed, 36 insertions(+), 31 deletions(-) diff --git a/src/ppx/graphics_util.cpp b/src/ppx/graphics_util.cpp index 889e0d4ec..8e7fae706 100644 --- a/src/ppx/graphics_util.cpp +++ b/src/ppx/graphics_util.cpp @@ -52,8 +52,7 @@ uint32_t GetPlaneHeightInPixels( bool hasColSubsampling = (subsampling == grfx::FORMAT_CHROMA_SUBSAMPLING_420); bool hasChromaValue = false; bool hasLumaValue = false; - for (auto it = plane.members.begin(); it != plane.members.end(); ++it) { - const grfx::FormatPlaneDesc::Member& member = *it; + for (const grfx::FormatPlaneDesc::Member& member : plane.members) { if (member.type == grfx::FORMAT_PLANE_CHROMA_TYPE_CHROMA) { hasChromaValue = true; } @@ -100,8 +99,7 @@ uint32_t GetPlaneWidthInPixels( bool hasRowSubsampling = (subsampling == grfx::FORMAT_CHROMA_SUBSAMPLING_420) || (subsampling == grfx::FORMAT_CHROMA_SUBSAMPLING_422); bool hasChromaValue = false; - for (auto it = plane.members.begin(); it != plane.members.end(); ++it) { - const grfx::FormatPlaneDesc::Member& member = *it; + for (const grfx::FormatPlaneDesc::Member& member : plane.members) { if (member.type == grfx::FORMAT_PLANE_CHROMA_TYPE_CHROMA) { hasChromaValue = true; break; @@ -136,8 +134,7 @@ uint32_t GetPlaneSizeInBytes( bool hasChromaValue = false; bool hasLumaValue = false; uint32_t rowBitFactor = 0; - for (auto it = plane.members.begin(); it != plane.members.end(); ++it) { - const grfx::FormatPlaneDesc::Member& member = *it; + for (const grfx::FormatPlaneDesc::Member& member : plane.members) { if (member.type == grfx::FORMAT_PLANE_CHROMA_TYPE_CHROMA) { hasChromaValue = true; } @@ -194,8 +191,8 @@ uint32_t GetPlanarImageSizeInBytes( grfx::FormatChromaSubsampling subsampling = formatDesc.chromaSubsampling; uint32_t imageSize = 0; - for (auto it = planeDesc.planes.begin(); it != planeDesc.planes.end(); ++it) { - imageSize += GetPlaneSizeInBytes(*it, subsampling, width, height); + for (const grfx::FormatPlaneDesc::Plane& plane : planeDesc.planes) { + imageSize += GetPlaneSizeInBytes(plane, subsampling, width, height); } return imageSize; } @@ -1700,14 +1697,15 @@ Result LoadFramesFromRawVideo( return ppx::ERROR_FAILED; } - std::optional formatPlanes = grfx::GetFormatPlaneDescription(format); - - uint32_t frameSizeInBytes = 0; - if (formatPlanes.has_value()) { - frameSizeInBytes = GetPlanarImageSizeInBytes(*formatDesc, *formatPlanes, width, height); + uint32_t frameSize = 0; // As measured in bytes, not pixels. + if (formatDesc->isPlanar) + { + std::optional formatPlanes = grfx::GetFormatPlaneDescription(format); + PPX_ASSERT_MSG(formatPlanes.has_value(), "No planes found for format " << format); + frameSize = GetPlanarImageSizeInBytes(*formatDesc, *formatPlanes, width, height); } else { - frameSizeInBytes = formatDesc->bytesPerTexel * width * height; + frameSize = formatDesc->bytesPerTexel * width * height; } ppx::fs::File file; @@ -1715,22 +1713,29 @@ Result LoadFramesFromRawVideo( PPX_ASSERT_MSG(false, "Cannot open the file!"); return ppx::ERROR_FAILED; } - const size_t size = file.GetLength(); - - if (size % frameSizeInBytes != 0) { - PPX_LOG_WARN("There may be partial frames in the raw video file at " << path << "; loading as much as possible."); - } + const size_t fileSize = file.GetLength(); - std::vector buffer(frameSizeInBytes); + std::vector buffer(frameSize); size_t totalRead = 0; - while (totalRead + frameSizeInBytes <= size) { - const size_t readSize = file.Read(buffer.data(), size); - if (readSize != size) { - PPX_ASSERT_MSG(false, "Cannot read the content of the file!"); - return ppx::ERROR_FAILED; + while (totalRead < fileSize) { + const size_t bytesRead = file.Read(buffer.data(), frameSize); + if (bytesRead < frameSize) { + // If we didn't read as many bytes as we expected to, and we haven't + // reached the end of the file, this is an error. + if (totalRead + bytesRead < fileSize) { + PPX_ASSERT_MSG( + false, + "Unable to load video frame; expected " << frameSize << " but read " << bytesRead << "bytes (previously read " << totalRead << ")."); + return ppx::ERROR_FAILED; + } + // Otherwise, fill the rest of the buffer with 0s. + else { + PPX_LOG_WARN("Read " << bytesRead << " bytes for the last frame of the video at " << path << "; filling the rest of the frame with 0s."); + std::fill(buffer.begin() + bytesRead, buffer.end(), 0); + } } pFrames->push_back(std::move(buffer)); - totalRead += readSize; + totalRead += bytesRead; } return ppx::SUCCESS; diff --git a/src/ppx/grfx/grfx_format.cpp b/src/ppx/grfx/grfx_format.cpp index e844f35d9..37e04d848 100644 --- a/src/ppx/grfx/grfx_format.cpp +++ b/src/ppx/grfx/grfx_format.cpp @@ -20,7 +20,7 @@ namespace grfx { #define UNCOMPRESSED_FORMAT(Name, Type, Aspect, BytesPerTexel, BytesPerComponent, Layout, Component, ComponentOffsets) \ { \ - "" #Name, \ + #Name, \ FORMAT_DATA_TYPE_##Type, \ FORMAT_ASPECT_##Aspect, \ BytesPerTexel, \ @@ -35,12 +35,12 @@ namespace grfx { #define COMPRESSED_FORMAT(Name, Type, BytesPerBlock, BlockWidth, Component) \ { \ - "" #Name, \ + #Name, \ FORMAT_DATA_TYPE_##Type, \ FORMAT_ASPECT_COLOR, \ BytesPerBlock, \ BlockWidth, \ - -1, \ + /*bytesPerComponent=*/-1, \ FORMAT_LAYOUT_COMPRESSED, \ FORMAT_COMPONENT_##Component, \ OFFSETS_UNDEFINED, \ @@ -50,12 +50,12 @@ namespace grfx { #define UNCOMP_PLANAR_FORMAT(Name, Type, Aspect, BytesPerTexel, Layout, Component, ChromaSubsampling) \ { \ - "" #Name, \ + #Name, \ FORMAT_DATA_TYPE_##Type, \ FORMAT_ASPECT_##Aspect, \ BytesPerTexel, \ /*blockWidth=*/1, \ - -1, \ + /*bytesPerComponent=*/-1, \ FORMAT_LAYOUT_##Layout, \ FORMAT_COMPONENT_##Component, \ OFFSETS_UNDEFINED, \