-
Notifications
You must be signed in to change notification settings - Fork 77
Test: Test framework refactor #1287
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.
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Wilczynski, Andrzej <[email protected]>
…xTxApp, FFmpeg and Gstreamer and create test_format based on new approach Signed-off-by: Wilczynski, Andrzej <[email protected]>
Signed-off-by: Wilczynski, Andrzej <[email protected]>
Signed-off-by: Wilczynski, Andrzej <[email protected]>
DawidWesierski4
left a comment
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 belive we should have 4 classes
one per applicaiton
every function of application class will need
if App1
else if App2
This is ...
Suboptimal
Classes will be easier to udnerstand imo
| "video": "mtl_video", | ||
| "audio": "mtl_audio" |
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 don't this this is right?
Gstreamer types are
mtl_st20p_tx and mtl_st20p_rx
There is no mtl_video or mtl_audio there at tall
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 i understand your configuration correctly
video and audio are legacy apis that do not have gstreamer support they should not be included
| "ancillary_port": 40000, | ||
| "fastmetadata_port": 40000 |
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 think they shoould be diffrent ?
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.
It could be, but this is for ST 2110-40 and ST 2110-41. It's unlikely they would be used simultaneously. However, this is only the default option and can be changed during command creation.
| "video": "rawvideo", | ||
| "audio": "pcm_s24le" |
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 do not understand the video and audio session type
if you want this to be "video" and "audio" to be legacy pipelines like in RxTxApp (i think thats the purpose?
Then they should be
"video" mtl_st20
"auido" mtl_st30
Without p
| "width": "rx-width", # for RX pipeline | ||
| "width_tx": "width", # for caps in TX pipeline | ||
| "height": "rx-height", # for RX pipeline | ||
| "height_tx": "height", # for caps in TX pipeline | ||
| "framerate": "rx-fps", # for RX pipeline | ||
| "framerate_tx": "framerate", # for caps in TX pipeline | ||
| "pixel_format": "rx-pixel-format", | ||
| "pixel_format_tx": "format", # for caps in TX pipeline |
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.
You have 2 diffrent kinds of arguments here
the ones passed by caps and MTL plugin specific
Is this fine ?
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.
it's redundant and will be removed
tests/validation/mtl_engine/app.py
Outdated
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
| class Application: |
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 belive the current application should be a class that will have
Application class with only general/common functionality
Have specific implementations inherit from it:
RxTxApp class
GstreamerApp
FFmpegApp
i belive it will be much more understandable when the super class (Application) will have only general functionality implemented
now it appears to mix general application logic with RxTx-specific functionality in the same class. This is confusing - sometimes the Application class handles generic operations, other times it contains RxTx-specific methods.
Would you consider refactoring to use inheritance here, with Application as the base class and RxTxApp as a derived class?
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.
You make a valid point about separate classes. The current approach emphasizes ease of use through a consistent interface across all frameworks. While separate classes would certainly improve readability, there's a trade-off: future changes might introduce inconsistencies between framework implementations, potentially leading to different behaviors for each framework. Let me prepare a separated version for future discussion.
… classess approach Signed-off-by: Wilczynski, Andrzej <[email protected]>
0c172a8 to
3c29b7b
Compare
DawidWesierski4
left a comment
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 think this looks fine at this point
Unify API for test creation and execution across multiple frameworks - Create common interface for RxTxApp, FFmpeg, and GStreamer frameworks - Standardize input parameters and user interface across all frameworks - Consolidate test creation and execution logic - Improve maintainability and reduce code duplication