Skip to content
Snippets Groups Projects

DXBC handling API

Merged Henri Verbeet requested to merge hverbeet/vkd3d:for_upstream_0 into master
9 unresolved threads

Public API for parsing and serialising DXBC blobs.

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • From patch 10/11:

    • /**
    • * The DXBC version. The only supported DXBC version in this version of
    • * vkd3d-shader is 1. */
    • uint32_t version;

    Do we even want to bother with this? I can only imagine that if the DXBC version is ever incremented (which seems unlikely at this point), it'll be for something that'll need us to reroll struct vkd3d_shader_dxbc_desc anyway.

    • /** The DXBC tag. This should always be "DXBC". */
    • uint32_t tag;

    Is there a point in this? This seems even less likely to change.

      • \param desc A vkd3d_shader_dxbc_desc structure describing the contents of
      • the DXBC blob. This structure may contain pointers into the input blob; its
      • contents are only valid while the input blob is valid. The contents of this
      • structure should be freed with vkd3d_shader_free_dxbc() when no longer
      • needed.

    Should we go farther than "may" and specify "will"? In order to be potentially more useful to the API consumer, and generally follow the principle of "be conservative in what you provide".

      • \param flags A set of flags modifying the behaviour of the function. No
      • flags are defined for this version of vkd3d-shader, and this parameter
      • should be set to 0.

    I don't suppose you have anything in mind for this parameter?

    Wrt patch 11/11, besides the tag and version, do we want to put the onus on the user to specify the checksum and total size? (And if not, do we want to pass a whole vkd3d_shader_dxbc_desc?)

    Either way, I think the documentation should be clear about it.

    • Author Maintainer
      • /**
      • * The DXBC version. The only supported DXBC version in this version of
      * vkd3d-shader is 1. */
      • uint32_t version;

      Do we even want to bother with this? I can only imagine that if the DXBC version is ever incremented (which seems unlikely at this point), it'll be for something that'll need us to reroll struct vkd3d_shader_dxbc_desc anyway.

      • /** The DXBC tag. This should always be "DXBC". */
      • uint32_t tag;

      Is there a point in this? This seems even less likely to change.

      It's not terribly useful, no. I didn't see the harm in including it though, mostly with the idea that it would allow the (largely hypothetical at this point) vkd3d-blob program to dump the actual content of the blob instead of making something up. And while a version 2 doesn't seem terribly likely at this point, I could certainly imagine a version 2 that e.g. just has some extra fields or e.g. 64-bit offsets and sizes that should be representable with the current structure, even if it wouldn't include everything contained in the blob. That said, I don't mind dropping these if you object to them.

      • \param desc A vkd3d_shader_dxbc_desc structure describing the contents of
      • the DXBC blob. This structure may contain pointers into the input blob; its
      • contents are only valid while the input blob is valid. The contents of this
      • structure should be freed with vkd3d_shader_free_dxbc() when no longer
      • needed.

      Should we go farther than "may" and specify "will"? In order to be potentially more useful to the API consumer, and generally follow the principle of "be conservative in what you provide".

      I had "will" originally and weakened it. I don't mind changing it back, although I'm not sure that's a guarantee we're necessarily willing to commit to?

      • \param flags A set of flags modifying the behaviour of the function. No
      • flags are defined for this version of vkd3d-shader, and this parameter
      • should be set to 0.

      I don't suppose you have anything in mind for this parameter?

      I do, actually. The idea was to provide the ability to disable validation of the checksum, and possibly sizes and offsets. Again mostly for the benefit of a hypothetical vkd3d-blob program.

      Wrt patch 11/11, besides the tag and version, do we want to put the onus on the user to specify the checksum and total size? (And if not, do we want to pass a whole vkd3d_shader_dxbc_desc?)

      Either way, I think the documentation should be clear about it.

      Perhaps I'm misunderstanding your comment, but I don't think we should, and that's why we're passing an array of sections to vkd3d_shader_serialize_dxbc() instead of a vkd3d_shader_dxbc_desc structure?

    • It's not terribly useful, no. I didn't see the harm in including it though, mostly with the idea that it would allow the (largely hypothetical at this point) vkd3d-blob program to dump the actual content of the blob instead of making something up.

      We currently check for TAG_DXBC and version 1 anyway, though. I guess that could change, but...

      And while a version 2 doesn't seem terribly likely at this point, I could certainly imagine a version 2 that e.g. just has some extra fields or e.g. 64-bit offsets and sizes that should be representable with the current structure, even if it wouldn't include everything contained in the blob. That said, I don't mind dropping these if you object to them.

      I suppose. I don't feel strongly about the version field, I just figured I'd put the question up at least.

      The DXBC tag field does seem a little silly though, if only because a DXBC shader that doesn't begin with DXBC magic sounds to me like "not a DXBC shader". (On the other hand, that kind of thing has happened before, so maybe that's a specious objection...)

      Should we go farther than "may" and specify "will"? In order to be potentially more useful to the API consumer, and generally follow the principle of "be conservative in what you provide".

      I had "will" originally and weakened it. I don't mind changing it back, although I'm not sure that's a guarantee we're necessarily willing to commit to?

      I don't think it'd cost us anything to commit to it, at least.

      I don't suppose you have anything in mind for this parameter?

      I do, actually. The idea was to provide the ability to disable validation of the checksum, and possibly sizes and offsets. Again mostly for the benefit of a hypothetical vkd3d-blob program.

      Ah, of course. I was blithely assuming we'd return the checksum without validating it, but I didn't actually check.

      Wrt patch 11/11, besides the tag and version, do we want to put the onus on the user to specify the checksum and total size? (And if not, do we want to pass a whole vkd3d_shader_dxbc_desc?)

      Either way, I think the documentation should be clear about it.

      Perhaps I'm misunderstanding your comment, but I don't think we should, and that's why we're passing an array of sections to vkd3d_shader_serialize_dxbc() instead of a vkd3d_shader_dxbc_desc structure?

      You're right, I'm just completely blind. Sorry about that.

    • I do, actually. The idea was to provide the ability to disable validation of the checksum, and possibly sizes and offsets. Again mostly for the benefit of a hypothetical vkd3d-blob program.

      In that case it would make sense to also provide a field for returning the expected checksum too, so the program can include it in the diagnostic message.

    • Please register or sign in to reply
  • Henri Verbeet added 2 commits

    added 2 commits

    • 03884d0c - vkd3d-shader/dxbc: Introduce API for parsing DXBC blobs.
    • 38ede239 - vkd3d-shader/dxbc: Introduce API for serialising DXBC blobs.

    Compare with previous version

    • Author Maintainer

      It's not terribly useful, no. I didn't see the harm in including it though, mostly with the idea that it would allow the (largely hypothetical at this point) vkd3d-blob program to dump the actual content of the blob instead of making something up.

      We currently check for TAG_DXBC and version 1 anyway, though. I guess that could change, but...

      And while a version 2 doesn't seem terribly likely at this point, I could certainly imagine a version 2 that e.g. just has some extra fields or e.g. 64-bit offsets and sizes that should be representable with the current structure, even if it wouldn't include everything contained in the blob. That said, I don't mind dropping these if you object to them.

      I suppose. I don't feel strongly about the version field, I just figured I'd put the question up at least.

      The DXBC tag field does seem a little silly though, if only because a DXBC shader that doesn't begin with DXBC magic sounds to me like "not a DXBC shader". (On the other hand, that kind of thing has happened before, so maybe that's a specious objection...)

      Right, I'm mostly thinking about a scenario like e.g. OSGN/OSG1/OSG5 or SHDR/SHEX.

      Should we go farther than "may" and specify "will"? In order to be potentially more useful to the API consumer, and generally follow the principle of "be conservative in what you provide".

      I had "will" originally and weakened it. I don't mind changing it back, although I'm not sure that's a guarantee we're necessarily willing to commit to?

      I don't think it'd cost us anything to commit to it, at least.

      I've pushed a v2, how's that?

    • FWIF, I agree it makes sense to include the tag and version fields.

    • Please register or sign in to reply
  • Giovanni Mascellani
    Giovanni Mascellani @giomasce started a thread on an outdated change in commit 03884d0c
1480 struct vkd3d_shader_dxbc_section_desc
1481 {
1482 /** The section tag. */
1483 uint32_t tag;
1484 /** The contents of the section. */
1485 struct vkd3d_shader_code data;
1486 };
1487
1488 /**
1489 * A description of a DXBC blob, as returned by vkd3d_shader_parse_dxbc().
1490 *
1491 * \since 1.7
1492 */
1493 struct vkd3d_shader_dxbc_desc
1494 {
1495 /** The DXBC tag. This should always be "DXBC". */
  • Giovanni Mascellani
  • 1865 1901 */
    1866 1902 VKD3D_SHADER_API void vkd3d_shader_set_log_callback(PFN_vkd3d_log callback);
    1867 1903
    1904 /**
    1905 * Free the contents of a vkd3d_shader_dxbc_desc structure allocated by
    1906 * another vkd3d-shader function, such as vkd3d_shader_parse_dxbc().
    1907 *
    1908 * This function may free the \ref vkd3d_shader_dxbc_desc.sections member, but
    1909 * does not free the structure itself.
    1910 *
    1911 * \param dxbc The vkd3d_shader_dxbc_desc structure to free.
    1912 *
    1913 * \since 1.7
    1914 */
    1915 VKD3D_SHADER_API void vkd3d_shader_free_dxbc(struct vkd3d_shader_dxbc_desc *dxbc);
    • My understanding is that free usually means that the pointer itself is freed. Maybe this should be renamed to destroy?

      Though I notice that this problem is also with vkd3d_shader_free_shader_code(), so maybe that's a ship that has already sailed.

    • Please register or sign in to reply
  • Giovanni Mascellani
  • 1921 *
    1922 * \param flags A set of flags modifying the behaviour of the function. No
    1923 * flags are defined for this version of vkd3d-shader, and this parameter
    1924 * should be set to 0.
    1925 *
    1926 * \param desc A vkd3d_shader_dxbc_desc structure describing the contents of
    1927 * the DXBC blob. Its vkd3d_shader_dxbc_section_desc structures will contain
    1928 * pointers into the input blob; its contents are only valid while the input
    1929 * blob is valid. The contents of this structure should be freed with
    1930 * vkd3d_shader_free_dxbc() when no longer needed.
    1931 *
    1932 * \param messages Optional output location for error or informational messages
    1933 * produced by the parser.
    1934 * \n
    1935 * This parameter behaves identically to the \a messages parameter of
    1936 * vkd3d_shader_compile().
    • Comment on lines +1935 to +1936

      I don't like this too much. It behaves identically in what respect? Being NULL-terminated and UTF-8? Being regulated by log_level? I would just repeat the sentences that apply. It's a bit more verbose, but clearer, and you don't have to scroll to vkd3d_shader_compile() to understand the remark.

    • I don't like this too much. It behaves identically in what respect? Being NULL-terminated and UTF-8? Being regulated by log_level? I would just repeat the sentences that apply. It's a bit more verbose, but clearer, and you don't have to scroll to vkd3d_shader_compile() to understand the remark.

      Yeah, that one's my fault for doing it with other functions...

    • Please register or sign in to reply
  • Giovanni Mascellani
  • 1949 * \param section_count The number of DXBC sections to serialise.
    1950 *
    1951 * \param sections An array of vkd3d_shader_dxbc_section_desc structures
    1952 * to serialise.
    1953 *
    1954 * \param dxbc A pointer to a vkd3d_shader_code structure in which the
    1955 * serialised blob will be stored.
    1956 * \n
    1957 * The output blob is allocated by vkd3d-shader and should be freed with
    1958 * vkd3d_shader_free_shader_code() when no longer needed.
    1959 *
    1960 * \param messages Optional output location for error or informational messages
    1961 * produced by the serialiser.
    1962 * \n
    1963 * This parameter behaves identically to the \a messages parameter of
    1964 * vkd3d_shader_compile().
  • Giovanni Mascellani
  • 1956 * \n
    1957 * The output blob is allocated by vkd3d-shader and should be freed with
    1958 * vkd3d_shader_free_shader_code() when no longer needed.
    1959 *
    1960 * \param messages Optional output location for error or informational messages
    1961 * produced by the serialiser.
    1962 * \n
    1963 * This parameter behaves identically to the \a messages parameter of
    1964 * vkd3d_shader_compile().
    1965 *
    1966 * \return A member of \ref vkd3d_result.
    1967 *
    1968 * \since 1.7
    1969 */
    1970 VKD3D_SHADER_API int vkd3d_shader_serialize_dxbc(size_t section_count,
    1971 const struct vkd3d_shader_dxbc_section_desc *sections, struct vkd3d_shader_code *dxbc, char **messages);
    • Comment on lines +1970 to +1971

      I am tempted to say we might want to allow for specifying the tag and version here too. Or rather take a struct vkd3d_shader_dxbc_desc as input and ignore size and checksum (or even allow for flags to use them even if they're invalid).

      Not that I have any real usage in mind, of course, so that should probably be ignored. I happen to like symmetry, and I don't see reason for having the parsing interface allowing for the hypothesis that a format change might in theory happen, and the serializing interface not doing the same.

    • Please register or sign in to reply
  • As a bottom line, I don't think a few tests would be harmful...

    • Author Maintainer

      Should we probably make VKD3D_MAKE_TAG() public too? And possibly TAG_DXBC too, after renaming?

      An argument could probably be made for VKD3D_MAKE_TAG or something similar; I'm not as convinced about TAG_DXBC and all the others. Note that Windows already has MAKEFOURCC though. Personally I'd also happily defer this to a future release unless someone has strong feelings about it.

      My understanding is that free usually means that the pointer itself is freed. Maybe this should be renamed to destroy?

      Though I notice that this problem is also with vkd3d_shader_free_shader_code(), so maybe that's a ship that has already sailed.

      It's a fair point, but we already have vkd3d_shader_free_messages(), vkd3d_shader_free_root_signature(), vkd3d_shader_free_scan_descriptor_info(), vkd3d_shader_free_shader_code(), and vkd3d_shader_free_shader_signature() with the same behaviour. I think that at this point doing something different for vkd3d_shader_free_dxbc() would just make things more confusing.

      I don't like this too much. It behaves identically in what respect? Being NULL-terminated and UTF-8? Being regulated by log_level? I would just repeat the sentences that apply. It's a bit more verbose, but clearer, and you don't have to scroll to vkd3d_shader_compile() to understand the remark.

      It's mostly just consistent with the existing documentation for functions taking a "messages" argument. Improvements welcome, of course, but this MR doesn't seem like the place for it.

      I am tempted to say we might want to allow for specifying the tag and version here too. Or rather take a struct vkd3d_shader_dxbc_desc as input and ignore size and checksum (or even allow for flags to use them even if they're invalid).

      Not that I have any real usage in mind, of course, so that should probably be ignored. I happen to like symmetry, and I don't see reason for having the parsing interface allowing for the hypothesis that a format change might in theory happen, and the serializing interface not doing the same.

      I could be convinced, but I had this taking a vkd3d_shader_dxbc_desc structure originally, and it was mostly just an inconvenience when implementing D3DGetBlobPart() and D3DStripShader() on top of vkd3d_shader_serialize_dxbc(). It also implies adding validation for the tag and version. How about I change it if you can convince Zeb?

      As a bottom line, I don't think a few tests would be harmful...

      Fair enough. Between future vkd3d-utils and d3dcompiler usage and the existing usage of most of this code in vkd3d and vkd3d-shader I figured we had enough coverage, but I'll add some vkd3d_shader_api.c tests; either in a potential v3 or in a follow-up MR.

    • An argument could probably be made for VKD3D_MAKE_TAG or something similar; I'm not as convinced about TAG_DXBC and all the others. Note that Windows already has MAKEFOURCC though. Personally I'd also happily defer this to a future release unless someone has strong feelings about it.

      My point of view is that in general we expect (or hope?) vkd3d to be helpful also for people not using it on top of Wine/Windows. So what is available in Windows doesn't count a lot. I think that having VKD3D_MAKE_TAG() at least would make sense.

      It's a fair point, but we already have vkd3d_shader_free_messages(), vkd3d_shader_free_root_signature(), vkd3d_shader_free_scan_descriptor_info(), vkd3d_shader_free_shader_code(), and vkd3d_shader_free_shader_signature() with the same behaviour. I think that at this point doing something different for vkd3d_shader_free_dxbc() would just make things more confusing.

      Yeah, right.

      I could be convinced, but I had this taking a vkd3d_shader_dxbc_desc structure originally, and it was mostly just an inconvenience when implementing D3DGetBlobPart() and D3DStripShader() on top of vkd3d_shader_serialize_dxbc(). It also implies adding validation for the tag and version. How about I change it if you can convince Zeb?

      It seems I couldn't. And after all what she says about possibly rerolling the API in the future makes sense.

    • Please register or sign in to reply
  • I am tempted to say we might want to allow for specifying the tag and version here too. Or rather take a struct vkd3d_shader_dxbc_desc as input and ignore size and checksum (or even allow for flags to use them even if they're invalid).

    Not that I have any real usage in mind, of course, so that should probably be ignored. I happen to like symmetry, and I don't see reason for having the parsing interface allowing for the hypothesis that a format change might in theory happen, and the serializing interface not doing the same.

    I could be convinced, but I had this taking a vkd3d_shader_dxbc_desc structure originally, and it was mostly just an inconvenience when implementing D3DGetBlobPart() and D3DStripShader() on top of vkd3d_shader_serialize_dxbc(). It also implies adding validation for the tag and version. How about I change it if you can convince Zeb?

    Heh. My position wrt issues like this, as a consumer of many APIs of different styles, is that the easiest APIs to work with are the ones with relatively simple functions, and also that rerolling an API, even rerolling it several times, doesn't actually make things any harder to work with. (Nor to implement, I think). Especially when all you're rerolling is the function signature itself. So if it were up to me, I'd just remove the tag and version from both sides.

    But that said, I'm not going to bikeshed this very hard, since it's a pretty minor concern. I just wanted to make sure that some alternatives were thought of (or, more saliently, discussed in a public place; I do quite trust Henri to think of design questions like that.)

    • Actually, looking at this again, the phrasing on the documentation for the tag and version is a little odd. The use of "should" and "only supported" suggests that they are meant to be filled by the user, when in fact they're currently only output parameters. I'm not sure what's better phrasing for this, though.

    • One possibility is to move them in the docstring for vkd3d_shader_parse_dxbc().

    • Please register or sign in to reply
  • Henri Verbeet added 2 commits

    added 2 commits

    • 04570cc9 - vkd3d-shader/dxbc: Introduce API for parsing DXBC blobs.
    • 8ae2992b - vkd3d-shader/dxbc: Introduce API for serialising DXBC blobs.

    Compare with previous version

  • Author Maintainer

    Actually, looking at this again, the phrasing on the documentation for the tag and version is a little odd. The use of "should" and "only supported" suggests that they are meant to be filled by the user, when in fact they're currently only output parameters. I'm not sure what's better phrasing for this, though.

    I've slightly adjusted this in v3. Note that I've also adjusted some of the types and included a basic test. Provided we can all live with the API as it currently is, I'd suggest to merge it this way. I'd really like to have this in 1.7, but I'm not going to hold off on starting the 1.7 release process much longer. We'll still have quite some time to make documentation changes, of course.

  • Henri Verbeet approved this merge request

    approved this merge request

  • Elizabeth Figura approved this merge request

    approved this merge request

  • Alexandre Julliard approved this merge request

    approved this merge request

  • added 31 commits

    • 8ae2992b...dea21268 - 20 commits from branch wine:master
    • b59de4de - vkd3d-shader/dxbc: Store DXBC section data as a vkd3d_shader_code structure.
    • bf3c0128 - vkd3d-shader/dxbc: Rename the dxbc_writer_section structure to vkd3d_shader_dxbc_section_desc.
    • 6c64f8a2 - vkd3d-shader/dxbc: Pass a vkd3d_shader_dxbc_section_desc structure to shader_parse_signature().
    • 5020781f - vkd3d-shader/dxbc: Pass a vkd3d_shader_dxbc_section_desc structure to...
    • e2825f76 - vkd3d-shader/dxbc: Pass a vkd3d_shader_dxbc_section_desc structure to the...
    • 3a3acb5b - vkd3d-shader/dxbc: Pass a vkd3d_shader_code structure to shader_parse_input_signature().
    • 2f762465 - vkd3d-shader/dxbc: Pass a vkd3d_shader_code structure to shader_extract_from_dxbc().
    • a06320c1 - vkd3d-shader/dxbc: Pass a vkd3d_shader_code structure to parse_dxbc().
    • d0d2130f - vkd3d-shader/dxbc: Rename parse_dxbc() to for_each_dxbc_section().
    • c87492ed - vkd3d-shader/dxbc: Introduce API for parsing DXBC blobs.
    • b1e13d6e - vkd3d-shader/dxbc: Introduce API for serialising DXBC blobs.

    Compare with previous version

  • Please register or sign in to reply
    Loading