Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Video code enhancements #2507

Merged
merged 6 commits into from
Feb 11, 2023
Merged

Conversation

mohamedchebbii
Copy link
Contributor

  • Rework of aspectRatio method for all video formats
  • Rework of GUID's asf decoding

@ghost
Copy link

ghost commented Feb 10, 2023

👇 Click on the image for a new way to code review
  • Make big changes easier — review code in small groups of related files

  • Know where to start — see the whole change at a glance

  • Take a code tour — explore the change with an interactive tour

  • Make comments and review — all fully sync’ed with github

    Try it now!

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map Legend

@codecov
Copy link

codecov bot commented Feb 10, 2023

Codecov Report

Merging #2507 (d593260) into main (4bdee6f) will increase coverage by 0.21%.
The diff coverage is 75.60%.

@@            Coverage Diff             @@
##             main    #2507      +/-   ##
==========================================
+ Coverage   64.46%   64.67%   +0.21%     
==========================================
  Files         104      104              
  Lines       22389    22260     -129     
  Branches    10911    10848      -63     
==========================================
- Hits        14432    14397      -35     
+ Misses       5710     5622      -88     
+ Partials     2247     2241       -6     
Impacted Files Coverage Δ
include/exiv2/asfvideo.hpp 50.00% <ø> (ø)
include/exiv2/matroskavideo.hpp 56.25% <ø> (ø)
src/matroskavideo.cpp 4.52% <0.00%> (+0.42%) ⬆️
src/helper_functions.cpp 91.11% <66.66%> (-1.92%) ⬇️
src/asfvideo.cpp 56.12% <76.71%> (+2.86%) ⬆️
src/quicktimevideo.cpp 60.84% <100.00%> (+0.44%) ⬆️
src/riffvideo.cpp 65.15% <100.00%> (+5.58%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@neheb
Copy link
Collaborator

neheb commented Feb 10, 2023

 D:\a\exiv2\exiv2\src\helper_functions.cpp(68): error C2220: the following warning is treated as an error
D:\a\exiv2\exiv2\src\helper_functions.cpp(68): warning C4267: 'initializing': conversion from 'size_t' to 'int', possible loss of data
D:\a\exiv2\exiv2\src\helper_functions.cpp(69): warning C4267: 'initializing': conversion from 'size_t' to 'int', possible loss of data

*
*/

bool AsfVideo::GUIDTag::operator==(const AsfVideo::GUIDTag& other) const {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is equivalent to = default. No need for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I beleive you are talking about operator= but here I developped operator== to compare with other tags.

github/exiv2/src/asfvideo.cpp:203:17: error: invalid operands to binary expression ('const AsfVideo::GUIDTag' and 'AsfVideo::GUIDTag')
  return Header == AsfVideo::GUIDTag(buf);

bool operator==(const GUIDTag& other) const;

// Constructor to create a GUID object by passing individual values for each attribute
GUIDTag(unsigned int data1, unsigned short data2, unsigned short data3, std::array<byte, 8> data4);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is needed, {} can be used instead of ().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but here I already have " GUIDTag(const uint8_t* bytes);" witch will delete the default initializer list

exiv2/src/asfvideo.cpp:92:25: error: no matching constructor for initialization of 'const AsfVideo::GUIDTag'
const AsfVideo::GUIDTag Header = {0x75B22630, 0x668E, 0x11CF, {0xA6, 0xD9, 0x00, 0xAA, 0x00, 0x62, 0xCE, 0x6C}};

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that error comes from the std::array.

const AsfVideo::GUIDTag Header = {0x75B22630, 0x668E, 0x11CF, std::array<byte, 8>{0xA6, 0xD9, 0x00, 0xAA, 0x00, 0x62, 0xCE, 0x6C}};

should work. or don't use std::array. I can handle this in a later PR if you want.

@neheb neheb merged commit b8b4a04 into Exiv2:main Feb 11, 2023
@mohamedchebbii mohamedchebbii deleted the video_code_enhancements branch February 11, 2023 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants