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

Mapper supports video streaming data reporting #5514

Merged
merged 1 commit into from
Apr 24, 2024
Merged

Mapper supports video streaming data reporting #5514

merged 1 commit into from
Apr 24, 2024

Conversation

wbc6080
Copy link
Contributor

@wbc6080 wbc6080 commented Apr 7, 2024

What type of PR is this?
/kind feature

What this PR does / why we need it:

This PR implements mapper’s feature of supporting reporting of streaming data. Mainly includes the following parts:

  1. Change the device model api definition to allow the data type of streaming data

  2. Add the function of saving frame and saving video to the mapper-framework data processing function.

  3. Change the logic of pushing device data in mapper-framework. When the type is streaming data, the device data is no longer pushed to the database and user applications, but processed by saving frames or saving videos.

Which issue(s) this PR fixes:

Fixes ##5401

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

@wbc6080 wbc6080 closed this Apr 7, 2024
@wbc6080 wbc6080 reopened this Apr 7, 2024
@wbc6080 wbc6080 closed this Apr 8, 2024
@wbc6080 wbc6080 reopened this Apr 8, 2024
@Shelley-BaoYue Shelley-BaoYue added this to the v1.17 milestone Apr 10, 2024
@kubeedge-bot kubeedge-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Apr 11, 2024
@wbc6080
Copy link
Contributor Author

wbc6080 commented Apr 11, 2024

/kind feature

@kubeedge-bot kubeedge-bot added the kind/feature Categorizes issue or PR as related to a new feature. label Apr 11, 2024
@wbc6080 wbc6080 closed this Apr 11, 2024
@wbc6080 wbc6080 reopened this Apr 11, 2024
@wbc6080 wbc6080 closed this Apr 11, 2024
@wbc6080 wbc6080 reopened this Apr 11, 2024
@wbc6080 wbc6080 closed this Apr 12, 2024
@wbc6080 wbc6080 reopened this Apr 12, 2024
@wbc6080 wbc6080 changed the title Mapper supports streaming data reporting Mapper supports video streaming data reporting Apr 17, 2024
@wbc6080
Copy link
Contributor Author

wbc6080 commented Apr 19, 2024

PTAL,thanks @luomengY @WillardHu @fisherxu

@luomengY
Copy link
Member

Suggest adding a switch control parameter for starting/stopping stream data collection in the next version.

WillardHu
WillardHu reviewed Apr 19, 2024
View reviewed changes
staging/src/github.com/kubeedge/mapper-framework/_template/mapper/data/stream/handler.go Outdated Show resolved Hide resolved
staging/src/github.com/kubeedge/mapper-framework/_template/mapper/data/stream/img.go Outdated Show resolved Hide resolved
staging/src/github.com/kubeedge/mapper-framework/_template/mapper/data/stream/img.go Outdated Show resolved Hide resolved
staging/src/github.com/kubeedge/mapper-framework/_template/mapper/data/stream/img.go
// Assign appropriate parts of buffer to image planes in pFrameRGB
// Note that pFrameRGB is an AVFrame, but AVFrame is a superset
// of AVPicture
avp := (*avcodec.Picture)(unsafe.Pointer(pFrameRGB))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we consider the case where type is invalid?

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 don't quite understand your question. Is there a problem with converting *avutil.Frame to *avcodec.Picture? The official documentation here shows that *avcodec.Picture is a superset of *avutil.Frame. You can refer to this

@wbc6080
Copy link
Contributor Author

wbc6080 commented Apr 19, 2024

Suggest adding a switch control parameter for starting/stopping stream data collection in the next version.

Yes, we can add it with the feature about mapper supporting data writing in v1.18

@kubeedge-bot kubeedge-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 21, 2024
@wbc6080 wbc6080 force-pushed the add-stream-field branch 2 times, most recently from 63456d3 to 2eaf646 Compare April 22, 2024 03:11
@kubeedge-bot kubeedge-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 22, 2024
@wbc6080
Copy link
Contributor Author

wbc6080 commented Apr 22, 2024

I have made modifications according to everyone's opinions, mainly:

  1. Removed conditional compilation.
  2. Modified the mapper generate script to generate different mapper projects depending on whether it is used to process stream data.
  3. Provide Dockerfile files for two situations
    PTAL,thanks @WillardHu @fisherxu

fisherxu
fisherxu reviewed Apr 23, 2024
View reviewed changes
...ng/src/github.com/kubeedge/mapper-framework/_template/mapper/data/stream/handler_nostream.go
@@ -0,0 +1,33 @@
/*
Copy link
Member

Choose a reason for hiding this comment

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

Is this file necessary? The file handler_nostream.go under stream dir seems a bit unreasonable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file is required, because stream.Handler needs to be called at mapper data plane capability like picture following. For mappers that do not require stream data processing, if this file is removed, an error will occur during compilation because stream.Handler cannot be found. If for name reasons, I can modify the mapper generation script and name this file to handler.go in the generated mapper project.

device

fisherxu
fisherxu reviewed Apr 23, 2024
View reviewed changes
staging/src/github.com/kubeedge/mapper-framework/_template/mapper/data/stream/handler.go Outdated Show resolved Hide resolved
staging/src/github.com/kubeedge/mapper-framework/_template/mapper/data/stream/handler.go Outdated Show resolved Hide resolved
staging/src/github.com/kubeedge/mapper-framework/_template/mapper/Dockerfile_stream Outdated
./configure && make && \
make install

RUN GOOS=linux go build -o main cmd/main.go
Copy link
Member

Choose a reason for hiding this comment

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

Is it necessary to separate into two dockerfile? also cc @WillardHu

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the stream mapper needs to download additional dependencies, it takes about five minutes to build the image, so I feel it would be better to separate it without affecting the construction of other mapper images.

staging/src/github.com/kubeedge/mapper-framework/hack/make-rules/generate.sh Outdated
Comment on lines 41 to 45
rm "${mapperPath}/data/stream/handler.go" "${mapperPath}/data/stream/img.go" "${mapperPath}/data/stream/video.go"
fi
Copy link
Member

Choose a reason for hiding this comment

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

If add new file for stream, this part will be hard to maintain.

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 modified it to only keep the handler_stream.go file and renamed this file to handler.go after build

@wbc6080
mapper supports streaming data reporting
7e31103 
Signed-off-by: wbc6080 <wangbincheng4@huawei.com>
@wbc6080 wbc6080 closed this Apr 23, 2024
@wbc6080 wbc6080 reopened this Apr 23, 2024
fisherxu
fisherxu reviewed Apr 23, 2024
View reviewed changes
Copy link
Member

@fisherxu fisherxu left a comment

Choose a reason for hiding this comment

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

/approve
Looks good for the first version.
/assign @WillardHu

@kubeedge-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fisherxu

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubeedge-bot kubeedge-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 23, 2024
@WillardHu
Copy link
Collaborator

/lgtm

@kubeedge-bot kubeedge-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 24, 2024
@kubeedge-bot kubeedge-bot merged commit 06a66af into kubeedge:master Apr 24, 2024
68 of 69 checks passed
@wbc6080 wbc6080 mentioned this pull request Apr 24, 2024
Closed
@wbc6080 wbc6080 deleted the add-stream-field branch May 10, 2024 03:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WillardHu WillardHu WillardHu left review comments

@fisherxu fisherxu fisherxu left review comments

Assignees

@WillardHu WillardHu

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
v1.17
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@wbc6080 @luomengY @kubeedge-bot @WillardHu @fisherxu @Shelley-BaoYue