| Message ID | fac11045-67bd-d414-5db1-09c8ae9be348@aracnet.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On Thu, Apr 13, 2017 at 12:39 PM, Aaron Levinson <alevinsn@aracnet.com> wrote: > On 4/13/2017 12:36 AM, Hendrik Leppkes wrote: >> On Thu, Apr 13, 2017 at 5:34 AM, Aaron Levinson <alevinsn@aracnet.com> wrote: >>> diff --git a/configure b/configure >>> index adb0060..5b76a33 100755 >>> --- a/configure >>> +++ b/configure >>> @@ -3646,6 +3646,8 @@ case "$toolchain" in >>> ld_default="$source_path/compat/windows/mslink" >>> nm_default="dumpbin -symbols" >>> ar_default="lib" >>> + decklink_indev_extralibs="" >>> + decklink_outdev_extralibs="" >>> case "$arch" in >>> arm*) >>> as_default="armasm" >> >> As in the other patch before, the toolchain handling section has no >> business setting these. >> >>> @@ -4902,6 +4904,8 @@ case $target_os in >>> fi >>> enabled x86_32 && check_ldflags -LARGEADDRESSAWARE >>> shlibdir_default="$bindir_default" >>> + decklink_outdev_extralibs="$decklink_outdev_extralibs -lole32 -loleaut32" >>> + decklink_indev_extralibs="$decklink_indev_extralibs -lole32 -loleaut32" >>> SLIBPREF="" >>> SLIBSUF=".dll" >>> SLIBNAME_WITH_VERSION='$(SLIBPREF)$(FULLNAME)-$(LIBVERSION)$(SLIBSUF)' >> >> I really don't like having these decklink specific things in the OS >> sections. I know its already there in one section, but I would greatly >> prefer if this would also be moved into a dedicated decklink section >> somewhere, instead of littering the msvc/mingw sections. >> In fact, its now set for both mingw and msvc, so setting it centrally >> in one place would be even better to avoid duplicating it. > > I believe that I've addressed both of these issues with the following new version of the patch. > > Thanks, > Aaron > > -------------------------------------------------------------------------------------- > > From 2e87ce15e9fb27b81b11b88a0660581549cfcfaf Mon Sep 17 00:00:00 2001 > From: Aaron Levinson <alevinsn@aracnet.com> > Date: Thu, 13 Apr 2017 03:28:40 -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 version of > patch. > > Comments: > > -- configure: Added if enabled decklink section and setting > decklink_indev_extralibs and decklink_outdev_extralibs here for > both mingw and Windows. In the case of Windows, the new value, > -lole32 and -loleaut32 overwrites the default value. In the case > of mingw, -lole32 and -loleaut32 is added to the default value. > 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 comments > in code for more details. > > -- libavdevice/decklink_enc.cpp: Rearranged the include of > libavformat/internal.h (for reasons as described above). > --- > configure | 15 +++++++++++++-- > libavdevice/decklink_common.cpp | 10 +++++++++- > libavdevice/decklink_dec.cpp | 17 +++++++++++++++-- > libavdevice/decklink_enc.cpp | 5 ++++- > 4 files changed, 41 insertions(+), 6 deletions(-) > > diff --git a/configure b/configure > index a383bf2..9a06437 100755 > --- a/configure > +++ b/configure > @@ -4843,8 +4843,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" > @@ -5946,6 +5944,19 @@ 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*) > + decklink_outdev_extralibs="$decklink_outdev_extralibs -lole32 -loleaut32" > + decklink_indev_extralibs="$decklink_indev_extralibs -lole32 -loleaut32" > + ;; > + win32|win64) > + decklink_outdev_extralibs="-lole32 -loleaut32" > + decklink_indev_extralibs="-lole32 -loleaut32" > + ;; > + esac > +fi > + Whats in the extralibs before this block that you need to clear out in the msvc case? Maybe that part should just be set conditionally so we can unify this, and don't set something we need to strip out again after. - Hendrik
On 4/13/2017 3:55 AM, Hendrik Leppkes wrote: > On Thu, Apr 13, 2017 at 12:39 PM, Aaron Levinson <alevinsn@aracnet.com> wrote: >> On 4/13/2017 12:36 AM, Hendrik Leppkes wrote: >> diff --git a/configure b/configure >> index a383bf2..9a06437 100755 >> --- a/configure >> +++ b/configure >> @@ -4843,8 +4843,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" >> @@ -5946,6 +5944,19 @@ 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*) >> + decklink_outdev_extralibs="$decklink_outdev_extralibs -lole32 -loleaut32" >> + decklink_indev_extralibs="$decklink_indev_extralibs -lole32 -loleaut32" >> + ;; >> + win32|win64) >> + decklink_outdev_extralibs="-lole32 -loleaut32" >> + decklink_indev_extralibs="-lole32 -loleaut32" >> + ;; >> + esac >> +fi >> + > > Whats in the extralibs before this block that you need to clear out in > the msvc case? > Maybe that part should just be set conditionally so we can unify this, > and don't set something we need to strip out again after. > > - Hendrik decklink_outdev_extralibs and decklink_indev_extralibs are declared as follows earlier in configure: decklink_indev_extralibs="-lstdc++" decklink_outdev_extralibs="-lstdc++" This is done in the standard section where <library>_<indev|outdev>_<extralibs|deps> can be found. libstdc++ is needed when building the Decklink source files with gcc/g++ because of, at a minimum, the use of STL in one of the .cpp files. For builds targeting Windows, -lole32 and -loleaut32 are also needed because of the use of COM. So, that explains why -lole32 and -loleaut32 are added for both mingw and Windows. Finally, when building with the Microsoft compiler (cl.exe), libstdc++ is not needed and actually can't be found. The C/C++ run-time is included by default in some form when building with cl.exe unless the -nodefaultlib linker option is used. So, that explains why it replaces the extralibs variables with just -lole32 and -loleaut32 for Windows builds. Note that win32 to specified as the target_os when an msvc build is done, and it may be the only case when target_os is set to win32 unless the user manually sets it as an option when invoking configure. -lole32 and -loleaut32 is likely relevant for both cygwin and the Intel compiler, but it should be a simple matter to add those later if anyone uses those for building. The elimination of -lstdc++ may also be relevant for the Intel compiler but probably only when using the Intel compiler on Windows. Anyway, I don't think an approach that moves -lstdc++ into the above if block is any better or worse than the patch in its current form, and the patch in its current form results in fewer changes to configure than a patch that moves -lstdc++ around. Aaron
On Thu, Apr 13, 2017 at 10:09 PM, Aaron Levinson <alevinsn@aracnet.com> wrote: > On 4/13/2017 3:55 AM, Hendrik Leppkes wrote: >> >> On Thu, Apr 13, 2017 at 12:39 PM, Aaron Levinson <alevinsn@aracnet.com> >> wrote: >>> >>> On 4/13/2017 12:36 AM, Hendrik Leppkes wrote: >>> diff --git a/configure b/configure >>> index a383bf2..9a06437 100755 >>> --- a/configure >>> +++ b/configure >>> @@ -4843,8 +4843,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" >>> @@ -5946,6 +5944,19 @@ 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*) >>> + decklink_outdev_extralibs="$decklink_outdev_extralibs >>> -lole32 -loleaut32" >>> + decklink_indev_extralibs="$decklink_indev_extralibs -lole32 >>> -loleaut32" >>> + ;; >>> + win32|win64) >>> + decklink_outdev_extralibs="-lole32 -loleaut32" >>> + decklink_indev_extralibs="-lole32 -loleaut32" >>> + ;; >>> + esac >>> +fi >>> + >> >> >> Whats in the extralibs before this block that you need to clear out in >> the msvc case? >> Maybe that part should just be set conditionally so we can unify this, >> and don't set something we need to strip out again after. >> >> - Hendrik > > > decklink_outdev_extralibs and decklink_indev_extralibs are declared as > follows earlier in configure: > > decklink_indev_extralibs="-lstdc++" > decklink_outdev_extralibs="-lstdc++" > > This is done in the standard section where > <library>_<indev|outdev>_<extralibs|deps> can be found. > > libstdc++ is needed when building the Decklink source files with gcc/g++ > because of, at a minimum, the use of STL in one of the .cpp files. > > For builds targeting Windows, -lole32 and -loleaut32 are also needed because > of the use of COM. So, that explains why -lole32 and -loleaut32 are added > for both mingw and Windows. > > Finally, when building with the Microsoft compiler (cl.exe), libstdc++ is > not needed and actually can't be found. The C/C++ run-time is included by > default in some form when building with cl.exe unless the -nodefaultlib > linker option is used. So, that explains why it replaces the extralibs > variables with just -lole32 and -loleaut32 for Windows builds. Note that > win32 to specified as the target_os when an msvc build is done, and it may > be the only case when target_os is set to win32 unless the user manually > sets it as an option when invoking configure. > > -lole32 and -loleaut32 is likely relevant for both cygwin and the Intel > compiler, but it should be a simple matter to add those later if anyone uses > those for building. The elimination of -lstdc++ may also be relevant for > the Intel compiler but probably only when using the Intel compiler on > Windows. > > Anyway, I don't think an approach that moves -lstdc++ into the above if > block is any better or worse than the patch in its current form, and the > patch in its current form results in fewer changes to configure than a patch > that moves -lstdc++ around. > You could add -lstdc++ to the msvc flag filter (ie. msvc_common_flags), since its never needed, and may be added by more things in the future, that would solve all of those neatly. - Hendrik
diff --git a/configure b/configure index a383bf2..9a06437 100755 --- a/configure +++ b/configure @@ -4843,8 +4843,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" @@ -5946,6 +5944,19 @@ 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*) + decklink_outdev_extralibs="$decklink_outdev_extralibs -lole32 -loleaut32" + decklink_indev_extralibs="$decklink_indev_extralibs -lole32 -loleaut32" + ;; + win32|win64) + decklink_outdev_extralibs="-lole32 -loleaut32" + 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..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. +extern "C" { +#include "libavformat/internal.h" +} + #include <DeckLinkAPI.h> #ifdef _WIN32 #include <DeckLinkAPI_i.c> @@ -28,7 +37,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..a663029 100644 --- a/libavdevice/decklink_dec.cpp +++ b/libavdevice/decklink_dec.cpp @@ -19,12 +19,15 @@ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA */ +extern "C" { +#include "libavformat/internal.h" +} + #include <DeckLinkAPI.h> 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 +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. + 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..3bd6357 100644 --- a/libavdevice/decklink_enc.cpp +++ b/libavdevice/decklink_enc.cpp @@ -22,11 +22,14 @@ #include <atomic> using std::atomic; +extern "C" { +#include "libavformat/internal.h" +} + #include <DeckLinkAPI.h> extern "C" { #include "libavformat/avformat.h" -#include "libavformat/internal.h" #include "libavutil/imgutils.h" }