Avnu / libavtp

Open source implementation of Audio Video Transport Protocol (AVTP) specified in IEEE 1722-2016 spec.
BSD 3-Clause "New" or "Revised" License
62 stars 34 forks source link

Cvf h264 #15

Closed edersondisouza closed 5 years ago

edersondisouza commented 5 years ago

CVF support for libavtp

Compressed Video Format (CVF) is one of video formats defined by AVTP (IEEE 1722-2016 chapter 8). This pull request adds support to it, focusing on H.264 aspects, so fields related exclusively to MJPEG or JPEG2000 are not done in this PR.

As there is some overlapping fields on both AAF and CVF headers, the first patch in this series factor out common code in a way that no changes are done to AAF API, but enables some code reuse. Some code that is also duplicated on AAF and CVF examples prompted another patch that creates a "common" shared code for the examples, so things like socket configuration are on it.

For the sake of simplicity, this PR does not cover the "sub-packets" defined for aggregation types on AVTP.

This PR also adds unit tests for CVF, and examples for CVF talker and listener.

More information about each patch, on the patch commit message.

Overall code coverage details are: lines......: 97.6% (1321 of 1353 lines) functions..: 100.0% (134 of 134 functions)

Let me know of any questions,

andrew-elder commented 5 years ago

@edersondisouza - thanks for this pull request and all the work that went it to it! Do you think you can address the travis reported errors?

edersondisouza commented 5 years ago

@andrew-elder Sure, I'll take a look.

edersondisouza commented 5 years ago

v2: Fixed travis issues Make travis also build the new examples

andrew-elder commented 5 years ago

@aguedes - I think you should go ahead and merge this PR when you are comfortable with it.

edersondisouza commented 5 years ago

v3:

edersondisouza commented 5 years ago

v4:

aguedes commented 5 years ago

Hi Ederson, the PR looks go to me! I'm waiting until early next week to merge it so others have the chance to review it as well. Thanks!

aguedes commented 5 years ago

@edersondisouza, I just noticed some code from avtp_stream.h APIs aren't covered by unit tests (e.g. avtp_stream_pdu_get() with NULL 'pdu' or invalid 'field'). Use the coverage report generated by ninja -C build coverage-text to guide you on what is missing.

Could you please add tests to cover that code?

edersondisouza commented 5 years ago

Oh, true. I had some tests specially for this, but ended up not sending them as I thought the normal tests would cover everything. I'll update the PR with these tests.

edersondisouza commented 5 years ago

v5: Added tests for avtp_stream.h internal API. This should expand test code coverage a bit (some of it was already being tested by other tests, that's why is not a huge improvement).

andrew-elder commented 5 years ago

@aguedes - any further comments? Looks to me like you are close to merging?

aguedes commented 5 years ago

Hi @edersondisouza, I think you added an older version which doesn't take in consideration the previous reviews. For instance, the set() tests are using ntohl() in all fields. Besides that, no need to add code to cover what is already covered. That's what I meant by "Use the coverage report to guide you on what is missing" :)

edersondisouza commented 5 years ago

@aguedes Sure about not "repeating" the tests. I left them there because it is clear it already tests all the internal API - knowing that most of it is already tested indirectly is cool, but makes things harder to understand - don't you think?

aguedes commented 5 years ago

The coverage report is pretty clear on what is covered or not so I don't see how it makes things harder to understand. If you want to repeat them, fine by me, just please make sure they are aligned with the other unit tests. Thanks!

edersondisouza commented 5 years ago

The "hard to understand" is somebody going to look at "test-stream.h", then noting that most of the API is not tested there. I guess that people would then (maybe) realise that other stuff is already being tested. Just my 2¢

edersondisouza commented 5 years ago

v6: New tests don't have ntohl(0) and are correctly indented.

aguedes commented 5 years ago

LGTM, merging now. Thanks @edersondisouza!