From patchwork Sun Apr 16 23:11:04 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Aaron Levinson X-Patchwork-Id: 3433 Delivered-To: ffmpegpatchwork@gmail.com Received: by 10.103.3.129 with SMTP id 123csp1053076vsd; Sun, 16 Apr 2017 16:11:19 -0700 (PDT) X-Received: by 10.223.179.24 with SMTP id j24mr16543928wrd.172.1492384279413; Sun, 16 Apr 2017 16:11:19 -0700 (PDT) Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org. [79.124.17.100]) by mx.google.com with ESMTP id 18si10862140wrw.52.2017.04.16.16.11.18; Sun, 16 Apr 2017 16:11:19 -0700 (PDT) Received-SPF: pass (google.com: domain of ffmpeg-devel-bounces@ffmpeg.org designates 79.124.17.100 as permitted sender) client-ip=79.124.17.100; Authentication-Results: mx.google.com; spf=pass (google.com: domain of ffmpeg-devel-bounces@ffmpeg.org designates 79.124.17.100 as permitted sender) smtp.mailfrom=ffmpeg-devel-bounces@ffmpeg.org Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 5BFD568830F; Mon, 17 Apr 2017 02:11:08 +0300 (EEST) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from white.spiritone.com (white.spiritone.com [216.99.193.38]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 38C8F6882E2 for ; Mon, 17 Apr 2017 02:11:01 +0300 (EEST) Received: from [192.168.3.100] (184-100-162-109.ptld.qwest.net [184.100.162.109]) by white.spiritone.com (Postfix) with ESMTPSA id 2DAEE73403AF for ; Sun, 16 Apr 2017 16:11:07 -0700 (PDT) References: <71cba29c-68f9-b40b-d8f5-9d19292876d7@aracnet.com> <4a4660c5-2f40-242c-3dfb-18dc374a7d72@aracnet.com> <80fdcb09-c7e6-b6f5-6e57-f719ff098845@aracnet.com> <4540627e-3d35-521a-86a7-4ce59fd8a27b@aracnet.com> To: FFmpeg development discussions and patches From: Aaron Levinson Message-ID: Date: Sun, 16 Apr 2017 16:11:04 -0700 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <4540627e-3d35-521a-86a7-4ce59fd8a27b@aracnet.com> Subject: Re: [FFmpeg-devel] [PATCH] Made minor changes to get the decklink avdevice code to build using Visual C++ X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" On 4/15/2017 6:13 AM, Aaron Levinson wrote: > On 4/15/2017 4:19 AM, Marton Balint wrote: >> >> On Thu, 13 Apr 2017, Aaron Levinson wrote: >> >>> On 4/13/2017 1:23 PM, Hendrik Leppkes wrote: >> [...] >> >>> -------------------------------------------------------------------------------------------- >>> >>> >>> From 00fdc9d15414a92a155eb7d1394bac3736dc9405 Mon Sep 17 00:00:00 2001 >>> From: Aaron Levinson >>> Date: Thu, 13 Apr 2017 14:22:19 -0700 >>> Subject: [PATCH] Made minor changes to get the decklink avdevice code >>> to build using Visual C++. >>> >> >> Maybe it just me, but as I mentioned earlier, I don't like too verbose >> comments in the code, see below: >> >>> diff --git a/libavdevice/decklink_common.cpp >>> b/libavdevice/decklink_common.cpp >>> index f01fba9..523217c 100644 >>> --- a/libavdevice/decklink_common.cpp >>> +++ b/libavdevice/decklink_common.cpp >>> @@ -19,6 +19,15 @@ >>> * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA >>> 02110-1301 USA >>> */ >>> >>> +// Moved inclusion of internal.h here in order to get it to build >>> successfully >>> +// when using Visual C++ to build--otherwise, compilation errors result >>> +// due to winsock.h (which is included indirectly by DeckLinkAPI.h and >>> +// DeckLinkAPI_i.c) conflicting with winsock2.h, which is included by >>> +// internal.h. If winsock2.h is included first, then the conflict is >>> resolved. >> >> This can be as short as this: >> >> /* Include internal.h first to avoid conflict of winsock.h (used by >> * DeckLink) and winsock2.h (used by libavformat) in MSVC++ builds */ >> >> (for multiline comments I think /* */ is preferred) >> >> Although since you do this in multiple headers, maybe it is enough if >> you specify the reason in the commit message, and delete the comment >> from here entirely. > > I think it is a good idea to have a comment in the code, as then it is > front in center if someone in the future wonders why internal.h precedes > the other includes and decides to move it because it will look cleaner, > thereby breaking the MSVC build. Although, in that case, maybe it would > be preferable to have the same comment in each of the three places, as > opposed to just one. > > A shorter comment is fine, and your example comment explains things well > enough, although I tend to think that more information is better than > less for comments in code. From my perspective, "used by DeckLink" is a > bit vague, since technically, DeckLinkAPI.h and DeckLinkAPI_i.c would be > generated by the user if the actual Blackmagic DeckLink SDK were used > (these files are not actually supplied with the Blackmagic DeckLink > SDK), which is how I got DeckLinkAPI.h and DeckLinkAPI_c.h when I built > ffmpeg. Well, DeckLinkAPI.h is included in the Linux header files in > the Blackmagic DeckLink SDK, but the _i.c file is not, and on Windows, > neither file is provided. > > Regarding multi-line comments being wrapped in /* */ vs using // on each > line, it doesn't so much matter in this case, but I personally find // > more versatile because then it makes it easier to comment out blocks of > code. But, if that's the standard for the ffmpeg code base, then I'll > make that change. > >>> @@ -262,8 +265,18 @@ static int64_t >>> get_pkt_pts(IDeckLinkVideoInputFrame *videoFrame, >>> res = >>> videoFrame->GetHardwareReferenceTimestamp(time_base.den, &bmd_pts, >>> &bmd_duration); >>> break; >>> case PTS_SRC_WALLCLOCK: >>> - pts = av_rescale_q(wallclock, AV_TIME_BASE_Q, time_base); >>> + { >>> + // doing the following rather than using AV_TIME_BASE_Q >>> because >>> + // AV_TIME_BASE_Q doesn't work when building with Visual >>> C++ >>> + // for C++ files (works for C files). When building C++ >>> files, >>> + // it results in compiler error C4576. At least, this is >>> the case >>> + // with Visual C++ 2015. >> >> And this is: >> >> // MSVC does not support compound literals like AV_TIME_BASE_Q in C++ >> code >> >>> + AVRational timebase; >>> + timebase.num = 1; >>> + timebase.den = AV_TIME_BASE; >>> + pts = av_rescale_q(wallclock, timebase, time_base); >>> break; >>> + } >> >> This whole block needs to be indented 1 column more I think. > > I'll double-check later today to make sure that it is indented properly, > adjust the comment, and submit a new patch then. > >> Regards, >> Marton > > Thanks, > Aaron A new version of the patch (generated against the latest code in git) can be found below. Basically, I just improved the comments. There was nothing wrong with the spacing--I think it just looks off because the "break;" line is left alone. Thanks, Aaron Levinson ---------------------------------------------------------------------- From f62e33366bd38e3eb15c37d6363d1862b34f5b9e Mon Sep 17 00:00:00 2001 From: Aaron Levinson Date: Sun, 16 Apr 2017 16:03:56 -0700 Subject: [PATCH] Made minor changes to get the decklink avdevice code to build using Visual C++. Purpose: Made minor changes to get the decklink avdevice code to build using Visual C++. Notes: Used Visual Studio 2015 (with update 3) for this. Also made changes to configure per Hendrik Leppkes's review of first and second versions of patch. Comments: -- configure: Added if enabled decklink section and setting decklink_indev_extralibs and decklink_outdev_extralibs here for both mingw and Windows. Also eliminated the setting of these variables in the mingw section earlier in the file. -- libavdevice/decklink_common.cpp: Switched the order of the include of libavformat/internal.h to workaround build issues with Visual C++. See comment in file for more details. -- libavdevice/decklink_dec.cpp: a) Rearranged the include of libavformat/internal.h (for reasons as described above). b) Made slight alteration to an argument for call to av_rescale_q() to workaround a compiler error with Visual C++. This appears to only be an issue when building C++ files with Visual C++. See comment in code for more details. -- libavdevice/decklink_enc.cpp: Rearranged the include of libavformat/internal.h (for reasons as described above). --- configure | 11 +++++++++-- libavdevice/decklink_common.cpp | 7 ++++++- libavdevice/decklink_dec.cpp | 17 +++++++++++++++-- libavdevice/decklink_enc.cpp | 7 ++++++- 4 files changed, 36 insertions(+), 6 deletions(-) diff --git a/configure b/configure index ec4e016..0d63c3d 100755 --- a/configure +++ b/configure @@ -4872,8 +4872,6 @@ case $target_os in else target_os=mingw32 fi - decklink_outdev_extralibs="$decklink_outdev_extralibs -lole32 -loleaut32" - decklink_indev_extralibs="$decklink_indev_extralibs -lole32 -loleaut32" LIBTARGET=i386 if enabled x86_64; then LIBTARGET="i386:x86-64" @@ -5975,6 +5973,15 @@ if ! disabled sdl2; then fi enabled sdl2 && enable sdl && add_cflags $sdl2_cflags && add_extralibs $sdl2_libs +if enabled decklink; then + case $target_os in + mingw32*|mingw64*|win32|win64) + decklink_outdev_extralibs="$decklink_outdev_extralibs -lole32 -loleaut32" + decklink_indev_extralibs="$decklink_indev_extralibs -lole32 -loleaut32" + ;; + esac +fi + disabled securetransport || { check_func SecIdentityCreate "-Wl,-framework,CoreFoundation -Wl,-framework,Security" && check_lib "Security/SecureTransport.h Security/Security.h" "SSLCreateContext SecItemImport" "-Wl,-framework,CoreFoundation -Wl,-framework,Security" && enable securetransport; } diff --git a/libavdevice/decklink_common.cpp b/libavdevice/decklink_common.cpp index f01fba9..cbb591c 100644 --- a/libavdevice/decklink_common.cpp +++ b/libavdevice/decklink_common.cpp @@ -19,6 +19,12 @@ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA */ +/* Include internal.h first to avoid conflict between winsock.h (used by + * DeckLink headers) and winsock2.h (used by libavformat) in MSVC++ builds */ +extern "C" { +#include "libavformat/internal.h" +} + #include #ifdef _WIN32 #include @@ -28,7 +34,6 @@ extern "C" { #include "libavformat/avformat.h" -#include "libavformat/internal.h" #include "libavutil/imgutils.h" #include "libavutil/intreadwrite.h" #include "libavutil/bswap.h" diff --git a/libavdevice/decklink_dec.cpp b/libavdevice/decklink_dec.cpp index 67eaf97..69f790d 100644 --- a/libavdevice/decklink_dec.cpp +++ b/libavdevice/decklink_dec.cpp @@ -19,12 +19,17 @@ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA */ +/* Include internal.h first to avoid conflict between winsock.h (used by + * DeckLink headers) and winsock2.h (used by libavformat) in MSVC++ builds */ +extern "C" { +#include "libavformat/internal.h" +} + #include extern "C" { #include "config.h" #include "libavformat/avformat.h" -#include "libavformat/internal.h" #include "libavutil/avutil.h" #include "libavutil/common.h" #include "libavutil/imgutils.h" @@ -262,8 +267,16 @@ static int64_t get_pkt_pts(IDeckLinkVideoInputFrame *videoFrame, res = videoFrame->GetHardwareReferenceTimestamp(time_base.den, &bmd_pts, &bmd_duration); break; case PTS_SRC_WALLCLOCK: - pts = av_rescale_q(wallclock, AV_TIME_BASE_Q, time_base); + { + /* MSVC does not support compound literals like AV_TIME_BASE_Q + * in C++ code (compiler error C4576) */ + // pts = av_rescale_q(wallclock, AV_TIME_BASE_Q, time_base); + AVRational timebase; + timebase.num = 1; + timebase.den = AV_TIME_BASE; + pts = av_rescale_q(wallclock, timebase, time_base); break; + } } if (res == S_OK) pts = bmd_pts / time_base.num; diff --git a/libavdevice/decklink_enc.cpp b/libavdevice/decklink_enc.cpp index 5105967..be01bcd 100644 --- a/libavdevice/decklink_enc.cpp +++ b/libavdevice/decklink_enc.cpp @@ -22,11 +22,16 @@ #include using std::atomic; +/* Include internal.h first to avoid conflict between winsock.h (used by + * DeckLink headers) and winsock2.h (used by libavformat) in MSVC++ builds */ +extern "C" { +#include "libavformat/internal.h" +} + #include extern "C" { #include "libavformat/avformat.h" -#include "libavformat/internal.h" #include "libavutil/imgutils.h" }