split super source and border commands #3
Conversation
There was a problem hiding this comment.
Thanks for your work @claudiocabral !
This looks really good, except for some small suggestions i've left.
It seems that this PR would close issue #2
| @@ -98,6 +98,7 @@ pf_fields["pf_field_unknown0"] = ProtoField.new ("Unknown", "atem.field.unknow | |||
| VALS["VALS__TOP_HASSDOUTPUT"] = {[0] = "No", [1] = "Yes"} | |||
| pf_fields["pf_cmd__top_hassdoutput"] = ProtoField.new ("Has SD Output", "atem.cmd._top.hassdoutput", ftypes.UINT8, VALS["VALS__TOP_HASSDOUTPUT"], base.DEC) | |||
|
|
|||
| pf_fields["pf_field_padding"] = ProtoField.new ("Padding", "atem.field.padding", ftypes.NONE, nil, base.NONE) | |||
There was a problem hiding this comment.
Why do we need an extra padding field? Have you a deeper insight in Blackmagic's ATEM protocol or do you just assume that there is special padding data? In case of the latter i would think, that one of the existing unknown fields would be the better and clearer choice.
There was a problem hiding this comment.
Hi Julian, I worked on this a while ago; but given that those bytes appear to be random, don't seem to have an effect on the ATEM and always round commands to a multiple of 4, we have enough information to infer that they are padding bytes.
If you don't think that's enough, it can of course be changed to unknown
| cmd_tree:add(pf_fields["pf_field_clip"], tvbuf:range(pos+16, 2)) | ||
| cmd_tree:add(pf_fields["pf_field_gain0"], tvbuf:range(pos+18, 2)) | ||
| cmd_tree:add(pf_fields["pf_field_invertkey0"], tvbuf:range(pos+20, 1)) | ||
| cmd_tree:add(pf_fields["pf_field_padding"], tvbuf:range(pos+21, 3)) |
There was a problem hiding this comment.
| cmd_tree:add(pf_fields["pf_field_padding"], tvbuf:range(pos+21, 3)) | |
| cmd_tree:add(pf_fields["pf_field_unknown1"], tvbuf:range(pos+21, 3)) |
See above, why not using existing unknown field instead of new padding field?
| cmd_tree:add(pf_fields["pf_field_borderluma"], tvbuf:range(pos+26, 2)) | ||
| cmd_tree:add(pf_fields["pf_field_lightsourcedirection"], tvbuf:range(pos+28, 2)) | ||
| cmd_tree:add(pf_fields["pf_field_lightsourcealtitude"], tvbuf:range(pos+30, 1)) | ||
| cmd_tree:add(pf_fields["pf_field_padding"], tvbuf:range(pos+31, 1)) |
There was a problem hiding this comment.
| cmd_tree:add(pf_fields["pf_field_padding"], tvbuf:range(pos+31, 1)) | |
| cmd_tree:add(pf_fields["pf_field_unknown1"], tvbuf:range(pos+31, 1)) |
Co-authored-by: jayjay <mail@julianjacobi.net>
Co-authored-by: jayjay <mail@julianjacobi.net>
Co-authored-by: jayjay <mail@julianjacobi.net>
Co-authored-by: jayjay <mail@julianjacobi.net>
Co-authored-by: jayjay <mail@julianjacobi.net>
|
@JulianJacobi all of the remaining comments concern the new "padding" bytes. |
This pull request splits the supersource packet into the 2 new packests used by version 8.0.x