Address FFmpeg/devel review comments

* Rename variables and functions * Fix rational initialization * Fix memory leaks when heap allocation fails * Replaced `unsigned long` with `uint<xx>_t` * Make `asset_locator_map` a stack object * `imf_close()` is now called automatically * Miscellaneous code path optimizations * Code style improved
sandflow · Dec 10, 2021 · 68f3152 · 68f3152
1 parent a7761a4
commit 68f3152
Show file tree

Hide file tree

Showing 7 changed files with 795 additions and 606 deletions.
diff --git a/.clang-format b/.clang-format
@@ -4,7 +4,7 @@ AlignAfterOpenBracket: false
 AlignConsecutiveAssignments: false
 AlignConsecutiveDeclarations: false
 AlignOperands: false
-AlignTrailingComments: false
+AlignTrailingComments: true
 AllowAllParametersOfDeclarationOnNextLine: true
 AllowShortBlocksOnASingleLine: false
 AllowShortCaseLabelsOnASingleLine: false
@@ -22,6 +22,7 @@ BraceWrapping:
   AfterFunction: true
 BreakBeforeTernaryOperators: false
 BreakConstructorInitializersBeforeComma: true
+BreakStringLiterals: true
 ColumnLimit: 0
 CommentPragmas: '^ IWYU pragma:'
 ContinuationIndentWidth: 4

diff --git a/imf-prepare-patches.sh b/imf-prepare-patches.sh
@@ -2,7 +2,7 @@
 
 set -e
 
-PATCH_VERSION="5"
+PATCH_VERSION="9"
 
 PATCH_NAME="avformat/imf"
 
@@ -11,14 +11,12 @@ PATCH_BRANCH="rd/patches"
 
 PATCHES_DIR="build/patches"
 
-PATCHES_IMF_HEADERS="libavformat/imf.h"
-PATCHES_IMF_DEC="libavformat/imfdec.c"
-PATCHES_IMF_CPL="libavformat/imf_cpl.c"
-PATCHES_IMF_TESTS="libavformat/tests/imf.c"
-PATCHES_MXF="libavformat/mxf.h libavformat/mxfdec.c"
+PATCHES_SRC="libavformat/imf.h libavformat/imf_cpl.c libavformat/imfdec.c"
 PATCHES_MISC="MAINTAINERS configure doc/demuxers.texi libavformat/Makefile libavformat/allformats.c"
+PATCHES_MAKEFILE="libavformat/Makefile"
+PATCHES_TESTS="libavformat/tests/imf.c"
 
-PATCHES_ALL="$PATCHES_IMF_HEADERS $PATCHES_IMF_DEC $PATCHES_IMF_CPL $PATCHES_IMF_TESTS $PATCHES_MXF $PATCHES_MISC"
+PATCHES_ALL="$PATCHES_SRC $PATCHES_MAKEFILE $PATCHES_MISC $PATCHES_TESTS"
 
 git fetch --all
 
@@ -30,9 +28,8 @@ git rebase $BASE_BRANCH
 
 git reset $BASE_BRANCH
 
-AUGMENTED=" * POSSIBILITY OF SUCH DAMAGE.\n\
- *\n\
- * This file is part of FFmpeg.\n\
+# update copyright header
+GPLCC=" * This file is part of FFmpeg.\n\
  *\n\
  * FFmpeg is free software; you can redistribute it and\/or\n\
  * modify it under the terms of the GNU Lesser General Public\n\
@@ -46,14 +43,24 @@ AUGMENTED=" * POSSIBILITY OF SUCH DAMAGE.\n\
  *\n\
  * You should have received a copy of the GNU Lesser General Public\n\
  * License along with FFmpeg; if not, write to the Free Software\n\
- * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA"
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA\n\
+ *\\/\n\
+\n\
+\\/*"
+
+sed -i "s/^ \* This file is part of FFmpeg\./$GPLCC/" $PATCHES_SRC $PATCHES_TESTS
 
-sed -i "/^ \* This file is part of FFmpeg\./,+1 d" $PATCHES_IMF_HEADERS $PATCHES_IMF_DEC $PATCHES_IMF_CPL $PATCHES_IMF_TESTS 
-sed -i "s/^ \* POSSIBILITY OF SUCH DAMAGE\./$AUGMENTED/" $PATCHES_IMF_HEADERS $PATCHES_IMF_DEC $PATCHES_IMF_CPL $PATCHES_IMF_TESTS
+# remove MXF documentation
+sed -i "/^@section mxf/,+12d" doc/demuxers.texi 
 
-git add -- $PATCHES_ALL
+# remove clang formatting commands
+sed -i "/^\\/\\/ clang-format/d" $PATCHES_SRC $PATCHES_TESTS
 
-git commit -m "${PATCH_NAME}: Headers" -- $PATCHES_IMF_HEADERS
+# remove tests from Makefile
+sed -i "/^TESTPROGS-\$(CONFIG_IMF_DEMUXER)/d" $PATCHES_MAKEFILE
+
+git add -- $PATCHES_SRC $PATCHES_MISC $PATCHES_MAKEFILE
+git commit -m "${PATCH_NAME}: Demuxer" -- $PATCHES_SRC $PATCHES_MISC $PATCHES_MAKEFILE
 git notes add -m "The IMF demuxer accepts as input an IMF CPL. The assets referenced by the CPL can be
 contained in multiple deliveries, each defined by an ASSETMAP file:
 
@@ -73,32 +80,16 @@ The location of the Track Files referenced by the Composition Playlist is stored
 in one or more XML documents called Asset Maps. More details at https://www.imfug.com/explainer.
 The IMF standard was first introduced in 2013 and is managed by the SMPTE.
 
-Header and build files.
-
 CHANGE NOTES:
 
-- fixed patchwork warnings
-- updated patch notes
-- added LGPL license
-- removed imf_internal.h
-- Improve error handling, including removing exit()
-- Fix code style
-- Allow custom I/O for all files (following DASH and HLS template)
-- replace realloc with av_realloc_f to fix leaks"
-
-# git commit -m "[IMF demuxer] MCA improvements to MXF decoder" -- $PATCHES_MXF
-# git notes add -m "Add support for SMPTE ST 377-4 (Multichannel Audio Labeling -- MCA) \
-# to the MXF decoder. MCA allows arbitrary audio channel configurations \
-# in MXF files."
-
-git commit -m "${PATCH_NAME}: CPL processor" -- $PATCHES_IMF_CPL
-git notes add -m "Implements IMF Composition Playlist (CPL) parsing."
-
-git commit -m "${PATCH_NAME}: Demuxer implementation" -- $PATCHES_IMF_DEC
-git notes add -m "Implements the IMF demuxer."
+- fix leaks when head allocation fails
+"
 
-git commit -m "${PATCH_NAME}: Tests and build files" -- $PATCHES_IMF_TESTS $PATCHES_MISC 
-git notes add -m "Tests and build files for the IMF demuxer."
+# add tests back to the Makefile
+sed -i "/^TESTPROGS-\$(CONFIG_SRTP)/a TESTPROGS-\$(CONFIG_IMF_DEMUXER)          += imf" $PATCHES_MAKEFILE
+git add -- $PATCHES_TESTS $PATCHES_MAKEFILE
+git commit -m "${PATCH_NAME}: Tests" -- $PATCHES_TESTS $PATCHES_MAKEFILE
+git notes add -m "Tests for the IMF demuxer."
 
 mkdir -p $PATCHES_DIR
 

diff --git a/imf-unit-tests.sh b/imf-unit-tests.sh
@@ -1,7 +1,11 @@
 #!/bin/sh
 
 valgrind --leak-check=full ./ffmpeg -y -i http://ffmpeg-imf-samples-public.s3.us-west-1.amazonaws.com/callout_51_l_r_c_lfe_ls_rs.mxf -f wav /dev/null
-valgrind --leak-check=full ./ffmpeg -y -i http://ffmpeg-imf-samples-public.s3-website-us-west-1.amazonaws.com/countdown/CPL_f5095caa-f204-4e1c-8a84-7af48c7ae16b.xml -f mp4 /dev/null
+
+valgrind --leak-check=full ./ffmpeg -y \
+  -assetmaps http://ffmpeg-imf-samples-public.s3.us-west-1.amazonaws.com/countdown-qc/ASSETMAP.xml,http://ffmpeg-imf-samples-public.s3.us-west-1.amazonaws.com/countdown/ASSETMAP.xml \
+  -i http://ffmpeg-imf-samples-public.s3-website-us-west-1.amazonaws.com/countdown/CPL_f5095caa-f204-4e1c-8a84-7af48c7ae16b.xml \
+  -f mp4 /dev/null
 
 make libavformat/tests/imf
 valgrind --leak-check=full libavformat/tests/imf
diff --git a/libavformat/imf.h b/libavformat/imf.h
@@ -26,8 +26,9 @@
  */
 
 /**
- * Public header file for the processing of Interoperable Master Format (IMF) packages.
- * 
+ * Public header file for the processing of Interoperable Master Format (IMF)
+ * packages.
+ *
  * @author Pierre-Anthony Lemieux
  * @author Valentin Noel
  * @file
@@ -42,142 +43,148 @@
 #include "libavutil/rational.h"
 #include <libxml/tree.h>
 
-#define IMF_UUID_FORMAT "urn:uuid:%02hhx%02hhx%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx"
+#define FF_UUID_FORMAT                                \
+    "urn:uuid:%02hhx%02hhx%02hhx%02hhx-%02hhx%02hhx-" \
+    "%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx"
 
 /**
  * UUID as defined in IETF RFC 422
  */
-typedef uint8_t UUID[16];
+typedef uint8_t FFUUID[16];
 
 /**
  * IMF Composition Playlist Base Resource
  */
-typedef struct IMFBaseResource {
-    AVRational edit_rate; /**< BaseResourceType/EditRate */
-    unsigned long entry_point; /**< BaseResourceType/EntryPoint */
-    unsigned long duration; /**< BaseResourceType/Duration */
-    unsigned long repeat_count; /**< BaseResourceType/RepeatCount */
-} IMFBaseResource;
+typedef struct FFIMFBaseResource {
+    AVRational edit_rate;  /**< BaseResourceType/EditRate */
+    uint32_t entry_point;  /**< BaseResourceType/EntryPoint */
+    uint32_t duration;     /**< BaseResourceType/Duration */
+    uint32_t repeat_count; /**< BaseResourceType/RepeatCount */
+} FFIMFBaseResource;
 
 /**
  * IMF Composition Playlist Track File Resource
  */
-typedef struct IMFTrackFileResource {
-    IMFBaseResource base;
-    UUID track_file_uuid; /**< TrackFileResourceType/TrackFileId */
-} IMFTrackFileResource;
+typedef struct FFIMFTrackFileResource {
+    FFIMFBaseResource base;
+    FFUUID track_file_uuid; /**< TrackFileResourceType/TrackFileId */
+} FFIMFTrackFileResource;
 
 /**
  * IMF Marker
  */
-typedef struct IMFMarker {
+typedef struct FFIMFMarker {
     xmlChar *label_utf8; /**< Marker/Label */
     xmlChar *scope_utf8; /**< Marker/Label/\@scope */
-    unsigned long offset; /**< Marker/Offset */
-} IMFMarker;
+    uint32_t offset;     /**< Marker/Offset */
+} FFIMFMarker;
 
 /**
  * IMF Composition Playlist Marker Resource
  */
-typedef struct IMFMarkerResource {
-    IMFBaseResource base;
-    unsigned long marker_count; /**< Number of Marker elements */
-    IMFMarker *markers; /**< Marker elements */
-} IMFMarkerResource;
+typedef struct FFIMFMarkerResource {
+    FFIMFBaseResource base;
+    uint32_t marker_count; /**< Number of Marker elements */
+    FFIMFMarker *markers;  /**< Marker elements */
+} FFIMFMarkerResource;
 
 /**
  * IMF Composition Playlist Virtual Track
  */
-typedef struct IMFBaseVirtualTrack {
-    UUID id_uuid; /**< TrackId associated with the Virtual Track */
-} IMFBaseVirtualTrack;
+typedef struct FFIMFBaseVirtualTrack {
+    FFUUID id_uuid; /**< TrackId associated with the Virtual Track */
+} FFIMFBaseVirtualTrack;
 
 /**
  * IMF Composition Playlist Virtual Track that consists of Track File Resources
  */
-typedef struct IMFTrackFileVirtualTrack {
-    IMFBaseVirtualTrack base;
-    unsigned long resource_count; /**< Number of Resource elements present in the Virtual Track */
-    IMFTrackFileResource *resources; /**< Resource elements of the Virtual Track */
-} IMFTrackFileVirtualTrack;
+typedef struct FFIMFTrackFileVirtualTrack {
+    FFIMFBaseVirtualTrack base;
+    uint32_t resource_count;           /**< Number of Resource elements present in the Virtual Track */
+    FFIMFTrackFileResource *resources; /**< Resource elements of the Virtual Track */
+    uint32_t resources_alloc_sz;       /**< Size of the resources buffer */
+} FFIMFTrackFileVirtualTrack;
 
 /**
  * IMF Composition Playlist Virtual Track that consists of Marker Resources
  */
-typedef struct IMFMarkerVirtualTrack {
-    IMFBaseVirtualTrack base;
-    unsigned long resource_count; /**< Number of Resource elements present in the Virtual Track */
-    IMFMarkerResource *resources; /**< Resource elements of the Virtual Track */
-} IMFMarkerVirtualTrack;
+typedef struct FFIMFMarkerVirtualTrack {
+    FFIMFBaseVirtualTrack base;
+    uint32_t resource_count;        /**< Number of Resource elements present in the Virtual Track */
+    FFIMFMarkerResource *resources; /**< Resource elements of the Virtual Track */
+} FFIMFMarkerVirtualTrack;
 
 /**
  * IMF Composition Playlist
  */
-typedef struct IMFCPL {
-    UUID id_uuid; /**< CompositionPlaylist/Id element */
-    xmlChar *content_title_utf8; /**< CompositionPlaylist/ContentTitle element */
-    AVRational edit_rate; /**< CompositionPlaylist/EditRate element */
-    IMFMarkerVirtualTrack *main_markers_track; /**< Main Marker Virtual Track */
-    IMFTrackFileVirtualTrack *main_image_2d_track; /**< Main Image Virtual Track */
-    unsigned long main_audio_track_count; /**< Number of Main Audio Virtual Tracks */
-    IMFTrackFileVirtualTrack *main_audio_tracks; /**< Main Audio Virtual Tracks */
-} IMFCPL;
+typedef struct FFIMFCPL {
+    FFUUID id_uuid;                                  /**< CompositionPlaylist/Id element */
+    xmlChar *content_title_utf8;                     /**< CompositionPlaylist/ContentTitle element */
+    AVRational edit_rate;                            /**< CompositionPlaylist/EditRate element */
+    FFIMFMarkerVirtualTrack *main_markers_track;     /**< Main Marker Virtual Track */
+    FFIMFTrackFileVirtualTrack *main_image_2d_track; /**< Main Image Virtual Track */
+    uint32_t main_audio_track_count;                 /**< Number of Main Audio Virtual Tracks */
+    FFIMFTrackFileVirtualTrack *main_audio_tracks;   /**< Main Audio Virtual Tracks */
+} FFIMFCPL;
 
 /**
- * Parse an IMF CompositionPlaylist element into the IMFCPL data structure.
+ * Parse an IMF CompositionPlaylist element into the FFIMFCPL data structure.
  * @param[in] doc An XML document from which the CPL is read.
- * @param[out] cpl Pointer to a memory area (allocated by the client), where the function writes a pointer to the newly constructed
- * IMFCPL structure (or NULL if the CPL could not be parsed). The client is responsible for freeing the IMFCPL structure using
- * imf_cpl_free().
+ * @param[out] cpl Pointer to a memory area (allocated by the client), where the
+ *  function writes a pointer to the newly constructed FFIMFCPL structure (or
+ *  NULL if the CPL could not be parsed). The client is responsible for freeing
+ *  the FFIMFCPL structure using ff_imf_cpl_free().
  * @return A non-zero value in case of an error.
  */
-int parse_imf_cpl_from_xml_dom(xmlDocPtr doc, IMFCPL **cpl);
+int ff_parse_imf_cpl_from_xml_dom(xmlDocPtr doc, FFIMFCPL **cpl);
 
 /**
- * Parse an IMF Composition Playlist document into the IMFCPL data structure.
+ * Parse an IMF Composition Playlist document into the FFIMFCPL data structure.
  * @param[in] in The context from which the CPL is read.
- * @param[out] cpl Pointer to a memory area (allocated by the client), where the function writes a pointer to the newly constructed
- * IMFCPL structure (or NULL if the CPL could not be parsed). The client is responsible for freeing the IMFCPL structure using
- * imf_cpl_free().
+ * @param[out] cpl Pointer to a memory area (allocated by the client), where the
+ * function writes a pointer to the newly constructed FFIMFCPL structure (or
+ * NULL if the CPL could not be parsed). The client is responsible for freeing
+ * the FFIMFCPL structure using ff_imf_cpl_free().
  * @return A non-zero value in case of an error.
  */
-int parse_imf_cpl(AVIOContext *in, IMFCPL **cpl);
+int ff_parse_imf_cpl(AVIOContext *in, FFIMFCPL **cpl);
 
 /**
- * Allocates and initializes an IMFCPL data structure.
- * @return A pointer to the newly constructed IMFCPL structure (or NULL if the structure could not be constructed). The client is
- * responsible for freeing the IMFCPL structure using imf_cpl_free().
+ * Allocates and initializes an FFIMFCPL data structure.
+ * @return A pointer to the newly constructed FFIMFCPL structure (or NULL if the
+ * structure could not be constructed). The client is responsible for freeing
+ * the FFIMFCPL structure using ff_imf_cpl_free().
  */
-IMFCPL *imf_cpl_alloc(void);
+FFIMFCPL *ff_imf_cpl_alloc(void);
 
 /**
- * Deletes an IMFCPL data structure previously instantiated with imf_cpl_alloc().
- * @param[in] cpl The IMFCPL structure to delete.
+ * Deletes an FFIMFCPL data structure previously instantiated with ff_imf_cpl_alloc().
+ * @param[in] cpl The FFIMFCPL structure to delete.
  */
-void imf_cpl_free(IMFCPL *cpl);
+void ff_imf_cpl_free(FFIMFCPL *cpl);
 
 /**
- * Reads an unsigned long from an XML element
+ * Reads an unsigned 32-bit integer from an XML element
  * @return 0 on success, < 0 AVERROR code on error.
  */
-int imf_xml_read_ulong(xmlNodePtr element, unsigned long *number);
+int ff_xml_read_uint32(xmlNodePtr element, uint32_t *number);
 
 /**
  * Reads an AVRational from an XML element
  * @return 0 on success, < 0 AVERROR code on error.
  */
-int imf_xml_read_rational(xmlNodePtr element, AVRational *rational);
+int ff_xml_read_rational(xmlNodePtr element, AVRational *rational);
 
 /**
  * Reads a UUID from an XML element
  * @return 0 on success, < 0 AVERROR code on error.
  */
-int imf_xml_read_UUID(xmlNodePtr element, uint8_t uuid[16]);
+int ff_xml_read_uuid(xmlNodePtr element, uint8_t uuid[16]);
 
 /**
  * Returns the first child element with the specified local name
  * @return A pointer to the child element, or NULL if no such child element exists.
  */
-xmlNodePtr imf_xml_get_child_element_by_name(xmlNodePtr parent, const char *name_utf8);
+xmlNodePtr ff_xml_get_child_element_by_name(xmlNodePtr parent, const char *name_utf8);
 
 #endif