-
Notifications
You must be signed in to change notification settings - Fork 106
Conversation
@saininav thanks for your patch. It's very helpful.
thanks for help. |
codecparsers/mpeg2_parser.cpp
Outdated
@@ -173,7 +173,7 @@ namespace MPEG2 { | |||
|
|||
SeqExtension::SeqExtension() { memset(this, 0, sizeof(*this)); } | |||
|
|||
SeqHeader::SeqHeader() { memset(this, 0, sizeof(*this)); } | |||
SeqHeader::SeqHeader() { memset((void*)this, 0, sizeof(*this)); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we need this, why we do not need to add the same thing for SeqExtension?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your review. GCC9 feels uneasy with non-trival classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but SeqExtension has constructor like SeqHeader, right?
thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe definition of Trivial class includes not only constructors but also operators, destructors, data members, functions, baseclass. All these should be trivial in order to define a class trivial.
useful link:
https://stackoverflow.com/questions/30096911/which-rules-determine-whether-an-object-is-trivially-copyable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a quick view, I can say SeqExtension has no user defined members, but this is not the case with SeqHeader.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
struct SeqHeader {
SeqHeader();
uint32_t horizontal_size_value;
uint32_t vertical_size_value;
uint32_t aspect_ratio_info;
uint32_t frame_rate_code;
uint32_t bit_rate_value;
bool marker_bit;
uint32_t vbv_buffer_size_value;
bool constrained_params_flag;
QuantMatrices quantizationMatrices;
};
You are right. SeqHeader has the quantizationMatrices, the memset will override the value set in QuantMatrices().
Could you help change it to
SeqHeader::SeqHeader() { memset((void*)this, 0, offsetof(SeqHeader, quantizationMatrices)); }
thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@saininav , thanks for the update. thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@saininav thanks for your great help.
Will merge it after we did some regression test.
@saininav , seems CI build failed, https://travis-ci.org/intel/libyami/builds/544025956#L1433 thanks |
GCC9 causing build failure: | ../../git/codecparsers/h264Parser.cpp: In constructor 'YamiParser::H264::PPS::PPS()': | ../../git/codecparsers/h264Parser.cpp:140:41: error: 'void* memset(void*, int, size_t)' clearing an object of type 'struct YamiParser::H264::PPS' with no trivial copy-assignment; use assignment or value-initialization instead [-Werror=class-memaccess] | 140 | memset(this, 0, offsetof(PPS, m_sps)); | | ^ | In file included from ../../git/codecparsers/h264Parser.cpp:21: | ../../git/codecparsers/h264Parser.h:292:8: note: 'struct YamiParser::H264::PPS' declared here | 292 | struct PPS { | | ^~~ | ../../git/codecparsers/h264Parser.cpp: In constructor 'YamiParser::H264::SliceHeader::SliceHeader()': | ../../git/codecparsers/h264Parser.cpp:686:49: error: 'void* memset(void*, int, size_t)' clearing an object of type 'class YamiParser::H264::SliceHeader' with no trivial copy-assignment; use assignment or value-initialization instead [-Werror=class-memaccess] | 686 | memset(this, 0, offsetof(SliceHeader, m_pps)); | | ^ | In file included from ../../git/codecparsers/h264Parser.cpp:21: | ../../git/codecparsers/h264Parser.h:371:7: note: 'class YamiParser::H264::SliceHeader' declared here | 371 | class SliceHeader { | | ^~~~~~~~~~~ | ../../git/codecparsers/h265Parser.cpp: In constructor 'YamiParser::H265::VPS::VPS()': | ../../git/codecparsers/h265Parser.cpp:165:53: error: 'void* memset(void*, int, size_t)' clearing an object of type 'struct YamiParser::H265::VPS' with no trivial copy-assignment; use assignment or value-initialization instead [-Werror=class-memaccess] | 165 | memset(this, 0, offsetof(VPS, hrd_layer_set_idx)); | | ^ | In file included from ../../git/codecparsers/h265Parser.cpp:21: | ../../git/codecparsers/h265Parser.h:256:12: note: 'struct YamiParser::H265::VPS' declared here | 256 | struct VPS { | | ^~~ | ../../git/codecparsers/h265Parser.cpp: In constructor 'YamiParser::H265::SPS::SPS()': | ../../git/codecparsers/h265Parser.cpp:174:39: error: 'void* memset(void*, int, size_t)' clearing an object of type 'struct YamiParser::H265::SPS' with no trivial copy-assignment; use assignment or value-initialization instead [-Werror=class-memaccess] | 174 | memset(this, 0, offsetof(SPS, vps)); | | ^ | In file included from ../../git/codecparsers/h265Parser.cpp:21: | ../../git/codecparsers/h265Parser.h:290:12: note: 'struct YamiParser::H265::SPS' declared here | 290 | struct SPS { | | ^~~ | ../../git/codecparsers/h265Parser.cpp: In constructor 'YamiParser::H265::PPS::PPS()': | ../../git/codecparsers/h265Parser.cpp:179:39: error: 'void* memset(void*, int, size_t)' clearing an object of type 'struct YamiParser::H265::PPS' with no trivial copy-assignment; use assignment or value-initialization instead [-Werror=class-memaccess] | 179 | memset(this, 0, offsetof(PPS, sps)); | | ^ | In file included from ../../git/codecparsers/h265Parser.cpp:21: | ../../git/codecparsers/h265Parser.h:362:12: note: 'struct YamiParser::H265::PPS' declared here | 362 | struct PPS { | | ^~~ | ../../git/codecparsers/h265Parser.cpp: In constructor 'YamiParser::H265::SliceHeader::SliceHeader()': | ../../git/codecparsers/h265Parser.cpp:184:47: error: 'void* memset(void*, int, size_t)' clearing an object of type 'struct YamiParser::H265::SliceHeader' with no trivial copy-assignment; use assignment or value-initialization instead [-Werror=class-memaccess] | 184 | memset(this, 0, offsetof(SliceHeader, pps)); | | ^ | In file included from ../../git/codecparsers/h265Parser.cpp:21: | ../../git/codecparsers/h265Parser.h:499:12: note: 'struct YamiParser::H265::SliceHeader' declared here | 499 | struct SliceHeader { | | ^~~~~~~~~~~ | ../../git/codecparsers/mpeg2_parser.cpp: In constructor 'YamiParser::MPEG2::SeqHeader::SeqHeader()': | ../../git/codecparsers/mpeg2_parser.cpp:163:59: error: 'void* memset(void*, int, size_t)' clearing an object of non-trivial type 'struct YamiParser::MPEG2::SeqHeader'; use assignment or value-initialization instead [-Werror=class-memaccess] | 163 | SeqHeader::SeqHeader() { memset(this, 0, sizeof(*this)); } | | ^ | In file included from ../../git/codecparsers/mpeg2_parser.cpp:34: | ../../git/codecparsers/mpeg2_parser.h:153:12: note: 'struct YamiParser::MPEG2::SeqHeader' declared here | 153 | struct SeqHeader { | | ^~~~~~~~~ | cc1plus: all warnings being treated as errors By typecasting structure pointer to void pointer, GCC9 does normal memset operation where offsetof() give correct number of bytes to set. Signed-off-by: Naveen Saini <[email protected]>
@wangzj0601 , could you help run regression test for this too. |
@wangzj0601 , any feedback on this? |
thanks @saininav for this. |
GCC9 causing build failure:
| ../../git/codecparsers/h264Parser.cpp: In constructor 'YamiParser::H264::PPS::PPS()':
| ../../git/codecparsers/h264Parser.cpp:140:41: error: 'void* memset(void*, int, size_t)' clearing an object of type 'struct YamiParser::H264::PPS' with no trivial copy-assignment; use assignment or value-initialization instead [-Werror=class-memaccess]
| 140 | memset(this, 0, offsetof(PPS, m_sps));
| | ^
| In file included from ../../git/codecparsers/h264Parser.cpp:21:
| ../../git/codecparsers/h264Parser.h:292:8: note: 'struct YamiParser::H264::PPS' declared here
| 292 | struct PPS {
| | ^~~
| ../../git/codecparsers/h264Parser.cpp: In constructor 'YamiParser::H264::SliceHeader::SliceHeader()':
| ../../git/codecparsers/h264Parser.cpp:686:49: error: 'void* memset(void*, int, size_t)' clearing an object of type 'class YamiParser::H264::SliceHeader' with no trivial copy-assignment; use assignment or value-initialization instead [-Werror=class-memaccess]
| 686 | memset(this, 0, offsetof(SliceHeader, m_pps));
| | ^
| In file included from ../../git/codecparsers/h264Parser.cpp:21:
| ../../git/codecparsers/h264Parser.h:371:7: note: 'class YamiParser::H264::SliceHeader' declared here
| 371 | class SliceHeader {
| | ^~~~~~~~~~~
| ../../git/codecparsers/h265Parser.cpp: In constructor 'YamiParser::H265::VPS::VPS()':
| ../../git/codecparsers/h265Parser.cpp:165:53: error: 'void* memset(void*, int, size_t)' clearing an object of type 'struct YamiParser::H265::VPS' with no trivial copy-assignment; use assignment or value-initialization instead [-Werror=class-memaccess]
| 165 | memset(this, 0, offsetof(VPS, hrd_layer_set_idx));
| | ^
| In file included from ../../git/codecparsers/h265Parser.cpp:21:
| ../../git/codecparsers/h265Parser.h:256:12: note: 'struct YamiParser::H265::VPS' declared here
| 256 | struct VPS {
| | ^~~
| ../../git/codecparsers/h265Parser.cpp: In constructor 'YamiParser::H265::SPS::SPS()':
| ../../git/codecparsers/h265Parser.cpp:174:39: error: 'void* memset(void*, int, size_t)' clearing an object of type 'struct YamiParser::H265::SPS' with no trivial copy-assignment; use assignment or value-initialization instead [-Werror=class-memaccess]
| 174 | memset(this, 0, offsetof(SPS, vps));
| | ^
| In file included from ../../git/codecparsers/h265Parser.cpp:21:
| ../../git/codecparsers/h265Parser.h:290:12: note: 'struct YamiParser::H265::SPS' declared here
| 290 | struct SPS {
| | ^~~
| ../../git/codecparsers/h265Parser.cpp: In constructor 'YamiParser::H265::PPS::PPS()':
| ../../git/codecparsers/h265Parser.cpp:179:39: error: 'void* memset(void*, int, size_t)' clearing an object of type 'struct YamiParser::H265::PPS' with no trivial copy-assignment; use assignment or value-initialization instead [-Werror=class-memaccess]
| 179 | memset(this, 0, offsetof(PPS, sps));
| | ^
| In file included from ../../git/codecparsers/h265Parser.cpp:21:
| ../../git/codecparsers/h265Parser.h:362:12: note: 'struct YamiParser::H265::PPS' declared here
| 362 | struct PPS {
| | ^~~
| ../../git/codecparsers/h265Parser.cpp: In constructor 'YamiParser::H265::SliceHeader::SliceHeader()':
| ../../git/codecparsers/h265Parser.cpp:184:47: error: 'void* memset(void*, int, size_t)' clearing an object of type 'struct YamiParser::H265::SliceHeader' with no trivial copy-assignment; use assignment or value-initialization instead [-Werror=class-memaccess]
| 184 | memset(this, 0, offsetof(SliceHeader, pps));
| | ^
| In file included from ../../git/codecparsers/h265Parser.cpp:21:
| ../../git/codecparsers/h265Parser.h:499:12: note: 'struct YamiParser::H265::SliceHeader' declared here
| 499 | struct SliceHeader {
| | ^~~~~~~~~~~
| cc1plus: all warnings being treated as errors
By typecasting structure pointer to void pointer, GCC9 does normal memset operation where offsetof() give correct
number of bytes to set.
Signed-off-by: Naveen Saini [email protected]