diff mbox

[FFmpeg-devel] Made minor changes to get the decklink avdevice code to build using Visual C++

Message ID 80fdcb09-c7e6-b6f5-6e57-f719ff098845@aracnet.com
State Superseded
Headers show

Commit Message

Aaron Levinson April 13, 2017, 9:32 p.m. UTC 
On 4/13/2017 1:23 PM, Hendrik Leppkes wrote:
> 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

That's a good idea and is cleaner and more general than the approach I had used.  Here's a new version of the patch, although it is my understanding that you may push the -lstdc++ change in msvc_common_flags() as part of another patch, but regardless, a new patch for this is needed.

--------------------------------------------------------------------------------------------

From 00fdc9d15414a92a155eb7d1394bac3736dc9405 Mon Sep 17 00:00:00 2001
From: Aaron Levinson <alevinsn@aracnet.com>
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++.

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:
a) Added replacement of -lstdc++ with nothing in msvc_common_flags()
   since the C++ standard library is included by default for MSVC
   builds as part of the C/C++ run-time.  Also, MSVC builds will fail
   when -lstdc++ is used.
b) 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 comments
   in code for more details.

-- libavdevice/decklink_enc.cpp: Rearranged the include of
   libavformat/internal.h (for reasons as described above).
---
 configure                       | 12 ++++++++++--
 libavdevice/decklink_common.cpp | 10 +++++++++-
 libavdevice/decklink_dec.cpp    | 17 +++++++++++++++--
 libavdevice/decklink_enc.cpp    |  5 ++++-
 4 files changed, 38 insertions(+), 6 deletions(-)

Comments

Marton Balint April 15, 2017, 11:19 a.m. UTC | #1 
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 <alevinsn@aracnet.com>
> 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.

> @@ -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.

Regards,
Marton
Aaron Levinson April 15, 2017, 1:13 p.m. UTC | #2 
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 <alevinsn@aracnet.com>
>> 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
diff mbox

Patch

diff --git a/configure b/configure
index a383bf2..684650a 100755
--- a/configure
+++ b/configure
@@ -3851,6 +3851,7 @@  msvc_common_flags(){
             -lz)                  echo zlib.lib ;;
             -lavicap32)           echo vfw32.lib user32.lib ;;
             -lx264)               echo libx264.lib ;;
+            -lstdc++)             ;;
             -l*)                  echo ${flag#-l}.lib ;;
             -LARGEADDRESSAWARE)   echo $flag ;;
             -L*)                  echo -libpath:${flag#-L} ;;
@@ -4843,8 +4844,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 +5945,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..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"
 }