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.

Sign up for GitHub

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

Jump to bottom

Add section for JSON metadata over MQTT #157

Merged
merged 25 commits into from
Jun 10, 2022
Merged

Add section for JSON metadata over MQTT #157

merged 25 commits into from
Jun 10, 2022

Conversation

svefredrik
Copy link
Contributor

No description provided.

@svefredrik svefredrik mentioned this pull request Oct 8, 2021
Closed
@svefredrik svefredrik changed the title Updated section for JSON metadata over MQTT Add section for JSON metadata over MQTT Oct 8, 2021
@svefredrik svefredrik added Analytics enhancement New feature or request labels Oct 8, 2021
@svefredrik svefredrik added this to the video milestone Oct 8, 2021
bsriramprasad and others added 9 commits December 3, 2021 13:47
@svefredrik svefredrik changed the base branch from 21.12 to 22.06 December 17, 2021 10:12
bsriramprasad and others added 10 commits December 17, 2021 13:10
HansBusch
HansBusch reviewed Jan 19, 2022
View reviewed changes
doc/Analytics.xml Outdated Show resolved Hide resolved
HansBusch
HansBusch reviewed Jan 19, 2022
View reviewed changes
Copy link
Member

@HansBusch HansBusch left a comment

Choose a reason for hiding this comment

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

This extension belongs to chapter 5.1. Unfortunately 5.1 is badly placed in the service part while it has nothing to do with the service.
Propose a restructuring by moving section 5.1 to a new 6 and appending the MQTT related normative part as 6.3.

@HansBusch
Copy link
Member

Suggest to use a different name for "Source" because the definition provided differs from the normal VideoSource or AudioSource.

HansBusch
HansBusch reviewed Jan 19, 2022
View reviewed changes
doc/Analytics.xml Outdated Show resolved Hide resolved
@bsriramprasad
Copy link
Contributor

Update 2022/3/17

  • Due to some connectivity issue, I am unable to join the call today
  • I am reworking on 'moving the chapters/sections' around as suggested in earlier Telco, but I see that 'cross-reference' links seems to be broken, I am trying to resolve them. I will update the PR once I resolve those issues, sometime next week.

@kieran242
Copy link
Contributor

Can't approve this as more work to be done and PR outstanding since 17th Mar. Can this PR be updated for review please.

bsriramprasad and others added 2 commits June 7, 2022 20:24
@bsriramprasad
Merge branch '22.06' into video/json-meta-over-mqtt
7743a0a
Addressed review feedback
9567b83 
1) Included JSON LD spec reference
2) Created a new section of JSON metadata (removed it as a subsection to service)
3) Accordingly all other sections are adjusted/auto-numbered.
@bsriramprasad
Copy link
Contributor

Hi All,

Sorry for getting back late, Here are the changes I pushed in the latest commit addressing earlier feedbacks, please review and let me know if this looks OK.

  1. Included JSON LD spec reference
  2. Created a new section 5 "ONVIF Metadata in JSON over MQTT" (removed it as a subsection within Section 6 "service", which was earlier Section 5 before the insertion)
  3. Accordingly all other sections are adjusted/auto-numbered.
  4. Clarified some of the text to improve the readability.

@bsriramprasad bsriramprasad requested a review from HansBusch June 8, 2022 09:19
HansBusch
HansBusch approved these changes Jun 9, 2022
View reviewed changes
Copy link
Member

@HansBusch HansBusch left a comment

Choose a reason for hiding this comment

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

Thanks for re-arranging. You didn't grasp the idea that we wanted to have the scene description as a separate section 5 with the metadata as 5.4.

Since I cannot edit your fork I suggest to merge as is and then rearrange section 5.

sujithhanwha
sujithhanwha requested changes Jun 9, 2022
View reviewed changes
doc/Core.xml
<para>Concrete Topic Expression to select specific topics to publish, see section <xref linkend="_Ref210199558" />.</para>
<para>If PublishFilter is empty, then device shall send all events but not metadata, for e.g. "MyDevice/onvif-ej//." <xref linkend="_Ref210199558" />.</para>
<para>Concrete Topic Expression to select specific topics to publish, see section <xref linkend="_Ref210199558" />. Example TopicExpression: "tns1:VideoAnalytics//.|tns1:RuleEngine//."</para>
<para>If PublishFilter is empty, then device shall send all events but not metadata, for e.g. "MyDevice/onvif-ej//." <xref linkend="_Ref210199558" />.</para>
Copy link
Contributor

Choose a reason for hiding this comment

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

Publish filter empty case is clear, but its not clear how we can set the publish filter for event and metadata topic. Here in example, its following the eventservice topic filter concept, do we have to use the full base topic "mydevice/onvif-ej/VideoAnalytics//.|mydevice/onvif-mj/VideoAnalytics" ?

Copy link
Member

Choose a reason for hiding this comment

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

Your example is what actually also some implementer tried for normal events and complained that it didn't work. I don't think that it is a good idea to mix MQTT topic expression with ONVIF topic expression.

Should we keep this as open issue or do we want to fix it before release?

Copy link
Contributor

Choose a reason for hiding this comment

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

In my opinion its better to fix this, since this is introduced by metadata topic addition.
If we want to follow the onvif event topic filter concept with PublishFilter, because of backward compatibility issues. one solution would be to add an additional MetaPublishFilter separately, that way we can handle both the filters separately.

Copy link
Contributor

Choose a reason for hiding this comment

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

Me and Fredrik agree with the suggestion to have a separate filter for events and metadata and I will push the changes as proposed below (changes will be reflected in analytics.xml, core.xml, event.wsdl)

PublishFilter: Empty (All events)
PublishFilter: Included (events matching the filter)

MetadataPublishFilter: Empty (All possible metadata depending on active sources)
MetadataPublishFilter: Included (metadata related to filter)

Copy link
Member

Choose a reason for hiding this comment

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

Generally agree. However MetadataPublishFilter empty sending everything seems to be wrong, because than it is always on.

Question: what is the filter syntax?

Copy link
Contributor

Choose a reason for hiding this comment

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

MetadataPublishFilter empty sending everything seems to be wrong, because than it is always on.

But the same can be switched off with MetadataConfiguration as that's basic for all these filters to apply, Isn't it?

Copy link
Member

Choose a reason for hiding this comment

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

Think I got the all/nothing mechanism: When MetadataFilter is missing no metadata is sent and when it is present with empty string then everything is sent.

Only part missing is to describe the format which probably deviates from PublishFilter.

doc/Analytics.xml Outdated
<programlisting>MyDevice/onvif-mj/VideoAnalytics/1/MyClassifier</programlisting>

<para>Where "MyDevice" is the given TopicPrefix and "1" is the
analytics configuration token. Shown below is how the sample metadata
Copy link
Contributor

Choose a reason for hiding this comment

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

According to the table above topic should have profile token.

"1" is the analytics configuration token ---> "1" is the profile token

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, yes its a typo.

HansBusch added 2 commits June 9, 2022 09:10
@HansBusch
Move MQTT section as sub section of chapter Scene Description.
4718cbf 
Streamline section level of last three MQTT sections.
Update chapter overview section.
@HansBusch
Fix example token name explanation.
007b89a
kieran242
kieran242 approved these changes Jun 9, 2022
View reviewed changes
Copy link
Contributor

@kieran242 kieran242 left a comment

Choose a reason for hiding this comment

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

I reviewed all files and including Hans Updates all looks ok.

@HansBusch HansBusch merged commit 33057ed into onvif:22.06 Jun 10, 2022
HansBusch added a commit that referenced this pull request Jun 15, 2022
@HansBusch
Merge change from event.wsdl: Add section for JSON metadata over MQTT (…
425dfb9 
…#157)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HansBusch HansBusch HansBusch approved these changes

@bsriramprasad bsriramprasad bsriramprasad left review comments

@kieran242 kieran242 kieran242 approved these changes

@sujithhanwha sujithhanwha sujithhanwha approved these changes

Assignees
No one assigned
Labels
22.06 Analytics enhancement New feature or request
Projects
None yet
Milestone
video
Development

Successfully merging this pull request may close these issues.
5 participants
@svefredrik @HansBusch @bsriramprasad @kieran242 @sujithhanwha