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 <alevi...@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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
