From 549f79e9e8450be1fce6c445966b363fb6aa6ab3 Mon Sep 17 00:00:00 2001 From: Anish Goyal Date: Thu, 6 Jun 2024 15:17:07 -0400 Subject: [PATCH 1/5] Add calculations for YUV / planar images Also, create format definitions for YUV images. --- include/ppx/graphics_util.h | 8 ++ include/ppx/grfx/grfx_format.h | 57 ++++++++++ src/ppx/graphics_util.cpp | 192 +++++++++++++++++++++++++++++++++ src/ppx/grfx/grfx_format.cpp | 72 ++++++++++++- 4 files changed, 327 insertions(+), 2 deletions(-) diff --git a/include/ppx/graphics_util.h b/include/ppx/graphics_util.h index 792974594..881ff3018 100644 --- a/include/ppx/graphics_util.h +++ b/include/ppx/graphics_util.h @@ -149,6 +149,14 @@ class TextureOptions const std::filesystem::path& path, grfx::Texture** ppTexture, const TextureOptions& options); + + friend Result LoadFramesFromRawVideo( + grfx::Queue* pQueue, + const std::filesystem::path& path, + uint32_t width, + uint32_t height, + grfx::Texture** ppTexture, + const TextureOptions& options); }; //! @fn CreateTextureFromBitmap diff --git a/include/ppx/grfx/grfx_format.h b/include/ppx/grfx/grfx_format.h index 7d44a2c8e..89f117648 100644 --- a/include/ppx/grfx/grfx_format.h +++ b/include/ppx/grfx/grfx_format.h @@ -16,6 +16,8 @@ #define ppx_grfx_format_h #include +#include +#include namespace ppx { namespace grfx { @@ -150,6 +152,8 @@ enum Format FORMAT_BC7_UNORM, FORMAT_BC7_SRGB, + FORMAT_G8_B8R8_2PLANE_420_UNORM, + FORMAT_COUNT, }; @@ -163,6 +167,14 @@ enum FormatAspectBit FORMAT_ASPECT_DEPTH_STENCIL = FORMAT_ASPECT_DEPTH | FORMAT_ASPECT_STENCIL, }; +enum FormatChromaSubsampling +{ + FORMAT_CHROMA_SUBSAMPLING_UNDEFINED = 0x0, + FORMAT_CHROMA_SUBSAMPLING_444 = 0x1, + FORMAT_CHROMA_SUBSAMPLING_422 = 0x2, + FORMAT_CHROMA_SUBSAMPLING_420 = 0x3, +}; + enum FormatComponentBit { FORMAT_COMPONENT_UNDEFINED = 0x0, @@ -255,11 +267,56 @@ struct FormatDesc // In case of packed or compressed formats, this field is invalid // and the offsets will be set to -1. FormatComponentOffset componentOffset; + + // In chroma-based formats, there can be subsampling of chroma color components + // of an image, to reduce image size. + FormatChromaSubsampling chromaSubsampling; + + // If true, this is a planar format that does not store all image components + // in a single block. E.G. YCbCr formats, where Cb and Cr may be defined in + // a separate plane than Y values, and have a different resolution. + bool isPlanar; +}; + +enum FormatPlaneChromaType +{ + FORMAT_PLANE_CHROMA_TYPE_UNDEFINED, + FORMAT_PLANE_CHROMA_TYPE_LUMA, + FORMAT_PLANE_CHROMA_TYPE_CHROMA, +}; + +struct FormatPlaneDesc +{ + struct Member + { + // Note: it's expected that only one bit would be set here. That being + // said, this is mostly to add clarity to plane component definitions. + FormatComponentBit component; + // This defines whether this is a luma value, chroma value, or neither + // (will be set to undefined for non-YCbCr types). + FormatPlaneChromaType type; + // Number of bits used to describe this component. + int bitCount; + }; + + struct Plane + { + std::vector members; + }; + + FormatPlaneDesc(std::initializer_list>&& planes); + + std::vector planes; }; //! @brief Gets a description of the given /b format. const FormatDesc* GetFormatDescription(grfx::Format format); +// Gets a description of planes in the format, if the format is planar. +// If the format is not planar, returns nullopt. +const std::optional GetFormatPlaneDescription( + grfx::Format format); + const char* ToString(grfx::Format format); } // namespace grfx diff --git a/src/ppx/graphics_util.cpp b/src/ppx/graphics_util.cpp index 5cfab3561..33866fdc6 100644 --- a/src/ppx/graphics_util.cpp +++ b/src/ppx/graphics_util.cpp @@ -22,6 +22,7 @@ #include "ppx/grfx/grfx_buffer.h" #include "ppx/grfx/grfx_command.h" #include "ppx/grfx/grfx_device.h" +#include "ppx/grfx/grfx_format.h" #include "ppx/grfx/grfx_image.h" #include "ppx/grfx/grfx_queue.h" #include "ppx/grfx/grfx_util.h" @@ -31,6 +32,145 @@ namespace ppx { namespace grfx_util { +namespace { + +const uint32_t kYuvWidth = 3152; // 3000; +const uint32_t kYuvHeight = 3840; // 3000; + +// Start planar image helper functions + +uint32_t GetPlaneHeightForCopy( + const grfx::FormatPlaneDesc::Plane& plane, + grfx::FormatChromaSubsampling subsampling, + uint32_t imageHeight) +{ + 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; + if (member.type == grfx::FORMAT_PLANE_CHROMA_TYPE_CHROMA) { + hasChromaValue = true; + } + else { + hasLumaValue = true; + } + } + + if (hasColSubsampling && hasChromaValue) { + // Note: you never have subsampling on the height axis of the image in + // a plane if luma values are present, since luma values usually aren't + // subsampled. You might have subsampling on the width axis, but that + // would essentially mean you get two luma values, and one of each + // chroma value, in a block of four. + if (hasLumaValue) { + PPX_LOG_WARN( + "Frame size will be inaccurate, there is vertical subsampling " + "with both chroma and luma values present on a single plane, " + "which is not supported!"); + } + + // If we're subsampling at 4:2:0, the image will have half its height. + return imageHeight / 2; + } + return imageHeight; +} + +uint32_t GetPlaneWidthForCopy( + const grfx::FormatPlaneDesc::Plane& plane, + grfx::FormatChromaSubsampling subsampling, + uint32_t imageWidth) +{ + 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; + if (member.type == grfx::FORMAT_PLANE_CHROMA_TYPE_CHROMA) { + hasChromaValue = true; + break; + } + } + + if (hasRowSubsampling && hasChromaValue) { + // Note: even if the layer has a luma value, generally, in the case of + // buffer copies, the width is treated as a half width, if we're + // subsampling at 4:2:0 or 4:2:2, and are looking at a plane with chroma + // values. + return imageWidth / 2; + } + return imageWidth; +} + +uint32_t GetPlaneSizeInBytes( + const grfx::FormatPlaneDesc::Plane& plane, + grfx::FormatChromaSubsampling subsampling, + uint32_t width, + uint32_t height) +{ + bool hasColSubsampling = subsampling == grfx::FORMAT_CHROMA_SUBSAMPLING_420; + bool hasRowSubsampling = hasColSubsampling || subsampling == grfx::FORMAT_CHROMA_SUBSAMPLING_422; + 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; + if (member.type == grfx::FORMAT_PLANE_CHROMA_TYPE_CHROMA) { + hasChromaValue = true; + } + else if (member.type == grfx::FORMAT_PLANE_CHROMA_TYPE_LUMA) { + hasLumaValue = true; + } + + // We only subsample chroma values. + if (member.type == grfx::FORMAT_PLANE_CHROMA_TYPE_CHROMA && hasRowSubsampling) { + rowBitFactor += member.bitCount / 2; + } + else { + rowBitFactor += member.bitCount; + } + } + + if (hasColSubsampling && hasChromaValue) { + // Note: you never have subsampling on the height axis of the image in + // a plane if luma values are present, since luma values usually aren't + // subsampled. You might have subsampling on the width axis, but that + // would essentially mean you get two luma values, and one of each + // chroma value, in a block of four. + if (hasLumaValue) { + PPX_LOG_WARN( + "Frame size will be inaccurate, there is vertical subsampling " + "with both chroma and luma values present on a single plane, " + "which is not supported!"); + } + + return (width * rowBitFactor * (height / 2)) / 8; + } + + // No subsampling for height, OR this plane is of luma values (which are + // not subsampled). + return (width * rowBitFactor * height) / 8; +} + +uint32_t GetPlanarImageSizeInBytes( + const grfx::FormatDesc& formatDesc, + const grfx::FormatPlaneDesc& planeDesc, + uint32_t width, + uint32_t height) +{ + 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); + } + return imageSize; +} + +// End planar image helper functions + +} // namespace + grfx::Format ToGrfxFormat(Bitmap::Format value) { // clang-format off @@ -1510,5 +1650,57 @@ Result CreateMeshFromFile( return ppx::SUCCESS; } +// ------------------------------------------------------------------------------------------------- + +Result LoadFramesFromRawVideo( + const std::filesystem::path& path, + grfx::Format format, + uint32_t width, + uint32_t height, + std::vector>* pFrames) +{ + PPX_ASSERT_NULL_ARG(pFrames); + + const grfx::FormatDesc* formatDesc = grfx::GetFormatDescription(format); + if (formatDesc == nullptr) { + PPX_LOG_ERROR("Failed to fetch information for texture format " << format); + return ppx::ERROR_FAILED; + } + + std::optional formatPlanes = grfx::GetFormatPlaneDescription(format); + + uint32_t frameSizeInBytes = 0; + if (formatPlanes.has_value()) { + frameSizeInBytes = GetPlanarImageSizeInBytes(*formatDesc, *formatPlanes, width, height); + } + else { + frameSizeInBytes = formatDesc->bytesPerTexel * width * height; + } + + ppx::fs::File file; + if (!file.Open(path)) { + 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."); + } + + std::vector buffer(frameSizeInBytes); + 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; + } + pFrames->push_back(std::move(buffer)); + } + + return ppx::SUCCESS; +} + } // namespace grfx_util } // namespace ppx diff --git a/src/ppx/grfx/grfx_format.cpp b/src/ppx/grfx/grfx_format.cpp index 26f04c5d7..7743cca11 100644 --- a/src/ppx/grfx/grfx_format.cpp +++ b/src/ppx/grfx/grfx_format.cpp @@ -28,7 +28,9 @@ namespace grfx { BytesPerComponent, \ FORMAT_LAYOUT_##Layout, \ FORMAT_COMPONENT_##Component, \ - OFFSETS_##ComponentOffsets \ + OFFSETS_##ComponentOffsets, \ + FORMAT_CHROMA_SUBSAMPLING_UNDEFINED, \ + /*isPlanar=*/false \ } #define COMPRESSED_FORMAT(Name, Type, BytesPerBlock, BlockWidth, Component) \ @@ -41,7 +43,24 @@ namespace grfx { -1, \ FORMAT_LAYOUT_COMPRESSED, \ FORMAT_COMPONENT_##Component, \ - OFFSETS_UNDEFINED \ + OFFSETS_UNDEFINED, \ + FORMAT_CHROMA_SUBSAMPLING_UNDEFINED, \ + /*isPlanar=*/false \ + } + +#define UNCOMP_PLANAR_FORMAT(Name, Type, Aspect, BytesPerTexel, Layout, Component, ChromaSubsampling) \ + { \ + "" #Name, \ + FORMAT_DATA_TYPE_##Type, \ + FORMAT_ASPECT_##Aspect, \ + BytesPerTexel, \ + /*blockWidth=*/1, \ + -1, \ + FORMAT_LAYOUT_##Layout, \ + FORMAT_COMPONENT_##Component, \ + OFFSETS_UNDEFINED, \ + FORMAT_CHROMA_SUBSAMPLING_##ChromaSubsampling, \ + /*isPlanar=*/true \ } // clang-format off @@ -52,6 +71,21 @@ namespace grfx { #define OFFSETS_RGBA(R, G, B, A) { R, G, B, A } // clang-format on +#define PLANE_MEMBER(Component, Type, BitSize) \ + FormatPlaneDesc::Member \ + { \ + FORMAT_COMPONENT_##Component, \ + FORMAT_PLANE_CHROMA_TYPE_##Type, \ + BitSize \ + } + +FormatPlaneDesc::FormatPlaneDesc(std::initializer_list>&& planes) +{ + for (auto it = planes.begin(); it != planes.end(); ++it) { + this->planes.push_back(FormatPlaneDesc::Plane{*it}); + } +} + // A static registry of format descriptions. // The order must match the order of the grfx::Format enum, so that // retrieving the description for a given format can be done in @@ -173,6 +207,13 @@ constexpr FormatDesc formatDescs[] = { COMPRESSED_FORMAT(BC7_UNORM , UNORM, 16, 4, RED_GREEN_BLUE_ALPHA), COMPRESSED_FORMAT(BC7_SRGB , SRGB, 16, 4, RED_GREEN_BLUE_ALPHA), + // We don't support retrieving component size (which are typically non-uniform) or byte offsets for uncompressed planar formats. + // +---------------------------------------------------------------------------------------+ + // | ,-> bytes per texel | + // | format name | type | aspect | layout | components | chroma subsampling | + UNCOMP_PLANAR_FORMAT(G8_B8R8_2PLANE_420_UNORM, UNORM, COLOR, 2, PACKED, RED_GREEN_BLUE, 420), + +#undef UNCOMP_PLANAR_FORMAT #undef COMPRESSED_FORMAT #undef UNCOMPRESSED_FORMAT #undef OFFSETS_UNDEFINED @@ -192,6 +233,33 @@ const FormatDesc* GetFormatDescription(grfx::Format format) return &formatDescs[formatIndex]; } +// Definitions for image planes - component types and bit sizes. +const std::optional GetFormatPlaneDescription( + grfx::Format format) +{ + switch (format) { + case FORMAT_G8_B8R8_2PLANE_420_UNORM: + return FormatPlaneDesc{ + {PLANE_MEMBER(GREEN, LUMA, 8)}, + {PLANE_MEMBER(BLUE, CHROMA, 8), PLANE_MEMBER(RED, CHROMA, 8)}}; + default: + // We'll log a warning and return nullopt outside of the switch + // statement. This is for the compiler. + break; + } + + const FormatDesc* description = GetFormatDescription(format); + if (description == nullptr || !description->isPlanar) { + PPX_LOG_WARN("Attempted to get planes for format " << format << ", which is non-planar"); + } + else { + PPX_LOG_ERROR("Could not find planes for planar format " << format << "; returning null."); + } + + return std::nullopt; +} +#undef PLANE_MEMBER + const char* ToString(grfx::Format format) { uint32_t formatIndex = static_cast(format); From 4a9ce6072eea628ce534897893a0c32bfbb20f7a Mon Sep 17 00:00:00 2001 From: Anish Goyal Date: Fri, 7 Jun 2024 15:23:53 -0400 Subject: [PATCH 2/5] Address PR comments --- include/ppx/graphics_util.h | 22 +++++++++---- include/ppx/grfx/grfx_format.h | 17 +++++++++-- src/ppx/graphics_util.cpp | 56 +++++++++++++++++++++++++++------- src/ppx/grfx/grfx_format.cpp | 12 ++++---- 4 files changed, 81 insertions(+), 26 deletions(-) diff --git a/include/ppx/graphics_util.h b/include/ppx/graphics_util.h index 881ff3018..6c75f7ee1 100644 --- a/include/ppx/graphics_util.h +++ b/include/ppx/graphics_util.h @@ -150,13 +150,23 @@ class TextureOptions grfx::Texture** ppTexture, const TextureOptions& options); + // Splits the frames from a raw video, based on the format of the frames, + // and metadata such as height and width. This assumes raw video, with no + // metadata in the file itself, and no audio tracks (such as a camera + // feed). Returns if the operation succeeded. + // path: The path to the file containing the video. + // format: Describes the video format; this is important for determining + // the size of a frame. + // width: The width of each frame, in pixels, with no subsampling applied. + // height: The height of each frame, in pixels, with no subsampling applied. + // pFrames: A vector where resulting frames will be stored. This should not + // be null. friend Result LoadFramesFromRawVideo( - grfx::Queue* pQueue, - const std::filesystem::path& path, - uint32_t width, - uint32_t height, - grfx::Texture** ppTexture, - const TextureOptions& options); + const std::filesystem::path& path, + grfx::Format format, + uint32_t width, + uint32_t height, + std::vector>* pFrames); }; //! @fn CreateTextureFromBitmap diff --git a/include/ppx/grfx/grfx_format.h b/include/ppx/grfx/grfx_format.h index 89f117648..f7f6bf2c5 100644 --- a/include/ppx/grfx/grfx_format.h +++ b/include/ppx/grfx/grfx_format.h @@ -285,13 +285,24 @@ enum FormatPlaneChromaType FORMAT_PLANE_CHROMA_TYPE_CHROMA, }; +// Note: this is distinct from FormatComponentBit because in the case of a +// member of an image plane, we only want to be able to specify one component +// bit. +enum FormatPlaneComponentType +{ + FORMAT_PLANE_COMPONENT_TYPE_UNDEFINED, + FORMAT_PLANE_COMPONENT_TYPE_RED, + FORMAT_PLANE_COMPONENT_TYPE_GREEN, + FORMAT_PLANE_COMPONENT_TYPE_BLUE, +}; + struct FormatPlaneDesc { struct Member { - // Note: it's expected that only one bit would be set here. That being - // said, this is mostly to add clarity to plane component definitions. - FormatComponentBit component; + // For debugging purposes: the color component that this plane member + // describes. + FormatPlaneComponentType component; // This defines whether this is a luma value, chroma value, or neither // (will be set to undefined for non-YCbCr types). FormatPlaneChromaType type; diff --git a/src/ppx/graphics_util.cpp b/src/ppx/graphics_util.cpp index 33866fdc6..889e0d4ec 100644 --- a/src/ppx/graphics_util.cpp +++ b/src/ppx/graphics_util.cpp @@ -34,17 +34,22 @@ namespace grfx_util { namespace { -const uint32_t kYuvWidth = 3152; // 3000; -const uint32_t kYuvHeight = 3840; // 3000; - // Start planar image helper functions -uint32_t GetPlaneHeightForCopy( +// Gets the height of a single plane, in terms of number of pixels represented. +// This doesn't directly correlate to the number of bits / bytes for the plane's +// height. The value returned can be used in a copy-image-to-buffer command. +// plane: The plane to get the height for (containing information about the +// color components represented in the plane). +// subsampling: The type of subsampling applied to chroma values for the image +// (e.g. 444, 422, 420). +// imageHeight: The height of the image, in pixels, with no subsampling applied. +uint32_t GetPlaneHeightInPixels( const grfx::FormatPlaneDesc::Plane& plane, grfx::FormatChromaSubsampling subsampling, uint32_t imageHeight) { - bool hasColSubsampling = subsampling == grfx::FORMAT_CHROMA_SUBSAMPLING_420; + 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) { @@ -52,9 +57,12 @@ uint32_t GetPlaneHeightForCopy( if (member.type == grfx::FORMAT_PLANE_CHROMA_TYPE_CHROMA) { hasChromaValue = true; } - else { + else if (member.type == grfx::FORMAT_PLANE_CHROMA_TYPE_LUMA) { hasLumaValue = true; } + else { + PPX_LOG_WARN("Member " << member.component << "has unknown chroma type."); + } } if (hasColSubsampling && hasChromaValue) { @@ -76,13 +84,21 @@ uint32_t GetPlaneHeightForCopy( return imageHeight; } -uint32_t GetPlaneWidthForCopy( +// Gets the width of a single plane, in terms of number of pixels represented. +// This doesn't directly correlate to the number of bits / bytes for the plane's +// height. The value returned can be used in a copy-image-to-buffer command. +// plane: The plane to get the width for (containing information about the +// color components represented in the plane). +// subsampling: The type of subsampling applied to chroma values for the image +// (e.g. 444, 422, 420). +// imageWidth: The width of the image, in pixels, with no subsampling applied. +uint32_t GetPlaneWidthInPixels( const grfx::FormatPlaneDesc::Plane& plane, grfx::FormatChromaSubsampling subsampling, uint32_t imageWidth) { - bool hasRowSubsampling = subsampling == grfx::FORMAT_CHROMA_SUBSAMPLING_420 || - subsampling == grfx::FORMAT_CHROMA_SUBSAMPLING_422; + 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; @@ -102,14 +118,21 @@ uint32_t GetPlaneWidthForCopy( return imageWidth; } +// Gets the size of an image plane in bytes. +// plane: The plane to get information for. (Contains information about the +// color components represented by this plane, and their bit counts). +// subsampling: The type of chroma subsampling applied to this image (e.g. +// 444, 422, 420). +// width: The width of the image, in pixels, with no subsampling applied. +// height: The height of the image, in pixels, with no subsampling applied. uint32_t GetPlaneSizeInBytes( const grfx::FormatPlaneDesc::Plane& plane, grfx::FormatChromaSubsampling subsampling, uint32_t width, uint32_t height) { - bool hasColSubsampling = subsampling == grfx::FORMAT_CHROMA_SUBSAMPLING_420; - bool hasRowSubsampling = hasColSubsampling || subsampling == grfx::FORMAT_CHROMA_SUBSAMPLING_422; + bool hasColSubsampling = (subsampling == grfx::FORMAT_CHROMA_SUBSAMPLING_420); + bool hasRowSubsampling = hasColSubsampling || (subsampling == grfx::FORMAT_CHROMA_SUBSAMPLING_422); bool hasChromaValue = false; bool hasLumaValue = false; uint32_t rowBitFactor = 0; @@ -121,6 +144,9 @@ uint32_t GetPlaneSizeInBytes( else if (member.type == grfx::FORMAT_PLANE_CHROMA_TYPE_LUMA) { hasLumaValue = true; } + else { + PPX_LOG_WARN("Member " << member.component << "has unknown chroma type."); + } // We only subsample chroma values. if (member.type == grfx::FORMAT_PLANE_CHROMA_TYPE_CHROMA && hasRowSubsampling) { @@ -152,6 +178,13 @@ uint32_t GetPlaneSizeInBytes( return (width * rowBitFactor * height) / 8; } +// Gets the total size of a planar image in bytes, by calculating the size of +// each plane individually. +// formatDesc: Information about the image format, such as the components +// represented, etc. +// planeDesc: Information about the components in the current image plane. +// width: The width of the image, in pixels, with no subsampling applied. +// height: The height of the image, in pixels, with no subsampling applied. uint32_t GetPlanarImageSizeInBytes( const grfx::FormatDesc& formatDesc, const grfx::FormatPlaneDesc& planeDesc, @@ -1697,6 +1730,7 @@ Result LoadFramesFromRawVideo( return ppx::ERROR_FAILED; } pFrames->push_back(std::move(buffer)); + totalRead += readSize; } return ppx::SUCCESS; diff --git a/src/ppx/grfx/grfx_format.cpp b/src/ppx/grfx/grfx_format.cpp index 7743cca11..e844f35d9 100644 --- a/src/ppx/grfx/grfx_format.cpp +++ b/src/ppx/grfx/grfx_format.cpp @@ -71,12 +71,12 @@ namespace grfx { #define OFFSETS_RGBA(R, G, B, A) { R, G, B, A } // clang-format on -#define PLANE_MEMBER(Component, Type, BitSize) \ - FormatPlaneDesc::Member \ - { \ - FORMAT_COMPONENT_##Component, \ - FORMAT_PLANE_CHROMA_TYPE_##Type, \ - BitSize \ +#define PLANE_MEMBER(Component, Type, BitSize) \ + FormatPlaneDesc::Member \ + { \ + FORMAT_PLANE_COMPONENT_TYPE_##Component, \ + FORMAT_PLANE_CHROMA_TYPE_##Type, \ + BitSize \ } FormatPlaneDesc::FormatPlaneDesc(std::initializer_list>&& planes) From f9ac2fbabae143d9e4aa0abe3ad65f90bca9b2ef Mon Sep 17 00:00:00 2001 From: Anish Goyal Date: Mon, 10 Jun 2024 14:02:14 -0400 Subject: [PATCH 3/5] Address PR comments --- src/ppx/graphics_util.cpp | 56 +++++++++++++++++++----------------- src/ppx/grfx/grfx_format.cpp | 10 +++---- 2 files changed, 35 insertions(+), 31 deletions(-) diff --git a/src/ppx/graphics_util.cpp b/src/ppx/graphics_util.cpp index 889e0d4ec..d9bfa42ad 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,14 @@ 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 +1712,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, \ From 9f6e6a69abe7bea7fd1af6aedfcbed2f6ff1b318 Mon Sep 17 00:00:00 2001 From: Anish Goyal Date: Mon, 10 Jun 2024 14:29:54 -0400 Subject: [PATCH 4/5] Fix headers in graphics_util.cpp Put self-header at the top, and C++ std headers after, before project- wide headers. --- src/ppx/graphics_util.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/ppx/graphics_util.cpp b/src/ppx/graphics_util.cpp index d9bfa42ad..40ca98dc5 100644 --- a/src/ppx/graphics_util.cpp +++ b/src/ppx/graphics_util.cpp @@ -12,9 +12,12 @@ // See the License for the specific language governing permissions and // limitations under the License. +#include "ppx/graphics_util.h" + +#include + #include "ppx/generate_mip_shader_VK.h" #include "ppx/generate_mip_shader_DX.h" -#include "ppx/graphics_util.h" #include "ppx/bitmap.h" #include "ppx/fs.h" #include "ppx/mipmap.h" From ffd6e73a438a157e9346bfae12617a4ed2112f2d Mon Sep 17 00:00:00 2001 From: Anish Goyal Date: Tue, 11 Jun 2024 11:02:38 -0400 Subject: [PATCH 5/5] Add more documentation Add a quick explanation of what chroma subsampling is / how it affects how pixels are stored. --- src/ppx/graphics_util.cpp | 29 ++++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-) diff --git a/src/ppx/graphics_util.cpp b/src/ppx/graphics_util.cpp index 40ca98dc5..a1e7dcc0f 100644 --- a/src/ppx/graphics_util.cpp +++ b/src/ppx/graphics_util.cpp @@ -42,6 +42,12 @@ namespace { // Gets the height of a single plane, in terms of number of pixels represented. // This doesn't directly correlate to the number of bits / bytes for the plane's // height. The value returned can be used in a copy-image-to-buffer command. +// +// This takes chroma subsampling into account; 4:4:4 means no subsampling, 4:2:2 +// means chroma values will be defined for every other column, and 4:2:0 means +// that chroma values will be defined for every other column and every other +// row. Chroma subsampling specifically applies to YCbCr images. +// // plane: The plane to get the height for (containing information about the // color components represented in the plane). // subsampling: The type of subsampling applied to chroma values for the image @@ -89,6 +95,12 @@ uint32_t GetPlaneHeightInPixels( // Gets the width of a single plane, in terms of number of pixels represented. // This doesn't directly correlate to the number of bits / bytes for the plane's // height. The value returned can be used in a copy-image-to-buffer command. +// +// This takes chroma subsampling into account; 4:4:4 means no subsampling, 4:2:2 +// means chroma values will be defined for every other column, and 4:2:0 means +// that chroma values will be defined for every other column and every other +// row. Chroma subsampling specifically applies to YCbCr images. +// // plane: The plane to get the width for (containing information about the // color components represented in the plane). // subsampling: The type of subsampling applied to chroma values for the image @@ -120,6 +132,12 @@ uint32_t GetPlaneWidthInPixels( } // Gets the size of an image plane in bytes. +// +// This takes chroma subsampling into account; 4:4:4 means no subsampling, 4:2:2 +// means chroma values will be defined for every other column, and 4:2:0 means +// that chroma values will be defined for every other column and every other +// row. Chroma subsampling specifically applies to YCbCr images. +// // plane: The plane to get information for. (Contains information about the // color components represented by this plane, and their bit counts). // subsampling: The type of chroma subsampling applied to this image (e.g. @@ -180,6 +198,12 @@ uint32_t GetPlaneSizeInBytes( // Gets the total size of a planar image in bytes, by calculating the size of // each plane individually. +// +// This takes chroma subsampling into account; 4:4:4 means no subsampling, 4:2:2 +// means chroma values will be defined for every other column, and 4:2:0 means +// that chroma values will be defined for every other column and every other +// row. Chroma subsampling specifically applies to YCbCr images. +// // formatDesc: Information about the image format, such as the components // represented, etc. // planeDesc: Information about the components in the current image plane. @@ -191,11 +215,10 @@ uint32_t GetPlanarImageSizeInBytes( uint32_t width, uint32_t height) { - grfx::FormatChromaSubsampling subsampling = formatDesc.chromaSubsampling; - uint32_t imageSize = 0; for (const grfx::FormatPlaneDesc::Plane& plane : planeDesc.planes) { - imageSize += GetPlaneSizeInBytes(plane, subsampling, width, height); + imageSize += GetPlaneSizeInBytes( + plane, formatDesc.chromaSubsampling, width, height); } return imageSize; }