#442 - Add KlvParser.parseStream(InputStream)

[ruckc]

Motivation and Context

#442 - reference issue. This is just a WIP attempt to provide InputStream parsing in KlvParser.

Description

Added the below method to KlvParser.

    public static void parseStream(
            InputStream is,
            Consumer<IMisbMessage> handler,
            Consumer<KlvParseException> exceptionHandler)

Given that parseBytes() throws a KlvParseException on bad data, and when dealing with a stream, you may want the ability to continue processing, this required the ability to emit IMisbMessage and/or KlvParseExceptions without breaking the stream.

How Has This Been Tested?

I tested with a mock application I'm developing where ffmpeg is running externally and its outputting the KLV data stream to stdout, which is being processed by KlvParser.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• All new and existing tests passed.

[ruckc]

Not sure how to update the documentation.

[bradh]

Not sure how to update the documentation.

The method javadoc is probably enough for this. If you had a really big change, it'd be worth putting in the top level README.md or the relevant package documentation.

[bradh]

Looks good on first check. Will take a more detailed look later, and would also appreciate input from @wlfgang

I think this can go into 1.x (aka main) as well if that would be useful for your needs.

[ruckc]

It would be useful in 1.x, but we can hash out any issues with 2.x first and then I can have a nice tidy PR for 1.x also.

[bradh]

A few suggestions, and a couple of requests for changes. Overall it looks pretty good, and I think this kind of streaming API could be worth making use of more broadly.

[ruckc]

@bradh - updated PR

[bradh]

LGTM.

@wlfgang Do those additional tests resolve your remaining concern(s)?

[bradh]

@ruckc You described this a WIP. Is anything else required?

[ruckc]

@bradh - thanks for the additional tests. I had no clue how to go about forcing a KlvParseException without trying to depend on something like st0601 for tests.

[ruckc]

@bradh - I don't think so. My main reason for WIP labeling was due to the unique nature of the API with the two Consumers, and not sure how well the implementation would be received.

[bradh]

@bradh - thanks for the additional tests. I had no clue how to go about forcing a KlvParseException without trying to depend on something like st0601 for tests.

np. I've spent a lot of time on the tests, and it still took a while to work it through.

Depending on other modules (which I also considered in the past) is pretty messy with JDK 17+ because the Jigsaw implementation doesn't seem to recognise the difference between "test only" and "real" dependencies when opening modules, so it requires polluting the requires just for the tests.

[ruckc]

@bradh - thanks for the additional tests. I had no clue how to go about forcing a KlvParseException without trying to depend on something like st0601 for tests.

np. I've spent a lot of time on the tests, and it still took a while to work it through.

Depending on other modules (which I also considered in the past) is pretty messy with JDK 17+ because the Jigsaw implementation doesn't seem to recognise the difference between "test only" and "real" dependencies when opening modules, so it requires polluting the requires just for the tests.

I've avoided even touching Jigsaw modules due to their inflexibility. Not entirely sure what problem they solve, but none of the problems Jigsaw solve are mine.

[codecov-commenter]

Codecov Report

Base: 91.80% // Head: 90.30% // Decreases project coverage by -1.49% ⚠️

Coverage data is based on head (81861d5) compared to base (e2dbccf).
Patch coverage: 88.70% of modified lines in pull request are covered.

Additional details and impacted files

@@             Coverage Diff              @@
##                2.x     #444      +/-   ##
============================================
- Coverage     91.80%   90.30%   -1.50%     
+ Complexity     5696     5658      -38     
============================================
  Files           615      615              
  Lines         14794    14855      +61     
  Branches       1406     1415       +9     
============================================
- Hits          13581    13415     -166     
- Misses         1144     1400     +256     
+ Partials         69       40      -29     

Flag	Coverage Δ
unittests-api	98.36% <88.70%> (-0.32%)	⬇️
unittests-api-ffmpeg	15.85% <ø> (-15.57%)	⬇️
unittests-awt	98.60% <ø> (ø)
unittests-core	98.44% <ø> (ø)
unittests-geoid	100.00% <ø> (ø)
unittests-mimd	97.24% <ø> (ø)
unittests-st0601	99.26% <ø> (ø)
unittests-st0602	77.64% <ø> (ø)
unittests-st0805	99.17% <ø> (ø)
unittests-st0806	99.64% <ø> (ø)
unittests-st0808	99.08% <ø> (ø)
unittests-st0809	99.11% <ø> (ø)
unittests-st0903-vmti	98.75% <ø> (ø)
unittests-st0903-vtrack	97.09% <ø> (ø)
unittests-st1108	99.66% <ø> (ø)
unittests-st1206	99.59% <ø> (ø)
unittests-st1301	98.91% <ø> (ø)
unittests-st1403	100.00% <ø> (ø)
unittests-st1601	100.00% <ø> (ø)
unittests-st1602	100.00% <ø> (ø)
unittests-st1603	98.03% <ø> (ø)
unittests-st1909	99.65% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
api/src/main/java/org/jmisb/api/klv/KlvParser.java	82.69% <78.57%> (-4.81%)	⬇️
...n/java/org/jmisb/api/common/KlvParseException.java	85.71% <83.33%> (-14.29%)	⬇️
...pi/src/main/java/org/jmisb/api/klv/BerDecoder.java	96.72% <100.00%> (+2.78%)	⬆️
.../src/main/java/org/jmisb/api/video/CodecUtils.java	0.00% <0.00%> (-95.84%)	⬇️
...in/java/org/jmisb/api/video/VideoStreamOutput.java	0.00% <0.00%> (-63.16%)	⬇️
...ain/java/org/jmisb/api/video/OutputStatistics.java	0.00% <0.00%> (-54.55%)	⬇️
...g/src/main/java/org/jmisb/api/video/FfmpegLog.java	11.76% <0.00%> (-52.95%)	⬇️
...src/main/java/org/jmisb/api/video/VideoOutput.java	0.00% <0.00%> (-47.13%)	⬇️
...src/main/java/org/jmisb/api/video/FfmpegUtils.java	10.63% <0.00%> (-4.26%)	⬇️

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

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[wlfgang]

I am good with this if you both are. Thanks!

[bradh]

@ruckc Thanks for the work on this - appreciated.

Do you want to propose the backport to main branch?

[ruckc]

@bradh - yes... tomorrow. the tests have changed significantly from 1.x to 2.x.

[bradh]

For 1.x, you can probably leverage something in ST 0601. Different but hopefully a bit simpler.

[bradh]

@ruckc: do you still have capacity for the backport, or would you like me to take a look at that?

[ruckc]

At this point I don't. We ended up going a different direction for KLV parsing using our own implementation. Trying to turn the jmisb output into json required more work than a simpler implementation.

[bradh]

At this point I don't. We ended up going a different direction for KLV parsing using our own implementation. Trying to turn the jmisb output into json required more work than a simpler implementation.

OK. Would be interested in discussion of a shared JSON format at some stage. Have created a discussion topic at #456