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

[Aaron Levinson]

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 at 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