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.

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

describe (some) fields #155

Merged
merged 10 commits into from
Oct 20, 2022
Merged

describe (some) fields #155

merged 10 commits into from
Oct 20, 2022

Conversation

fippo
Copy link
Collaborator

@fippo fippo commented Sep 29, 2022

html taken from webrtc-stats, lets see how this renders...


Preview | Diff

@fippo fippo force-pushed the describe-all-the-things branch 3 times, most recently from 4e3c03f to 76fcd19 Compare September 29, 2022 16:45
@fippo
Copy link
Collaborator Author

fippo commented Sep 29, 2022

@dontcallmedom any idea why https://pr-preview.s3.amazonaws.com/fippo/webrtc-media-streams/pull/155.html#RTCEncodedAudioFrameMetadata-members looks more BOLD and larger? I might be missing some CSS magic..

Copy link
Member

@dontcallmedom dontcallmedom left a comment

Choose a reason for hiding this comment

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

markdown fixes

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
fippo and others added 2 commits September 30, 2022 10:56
Co-authored-by: Dominique Hazael-Massieux <dom@w3.org>
Co-authored-by: Dominique Hazael-Massieux <dom@w3.org>
@fippo
Copy link
Collaborator Author

fippo commented Sep 30, 2022

actually the not-so-nice display seems to be a preview issue, also happens for stats. I think this should be ok.

@fippo fippo force-pushed the describe-all-the-things branch 4 times, most recently from 16cbe3d to 0eff049 Compare October 3, 2022 06:56
Copy link
Collaborator

@youennf youennf left a comment

Choose a reason for hiding this comment

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

LGTM overall, some nits below.
In terms of style, I wonder whether we want to have all these dl/dt/dd, or if there is a more markdown-based approach.

@@ -264,6 +264,7 @@ The <dfn method for="SFrameTransform">setEncryptionKey(|key|, |keyID|)</dfn> met

# RTCRtpScriptTransform # {#scriptTransform}

## <dfn>RTCEncodedVideoFrameType</dfn> dictionary ## {#RTCEncodedVideoFrameType}
Copy link
Collaborator

Choose a reason for hiding this comment

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

We probably want to replace RTCEncodedVideoFrameType by EncodedVideoFrameType, expect if there is a use for "empty".
That would remove the need for the below added section.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

empty seems to be used when the underlying frame is gone (i.e. after enqueing?).
I think that should be handled differently

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, interesting. This is a difference between Safari and Chrome I guess.
It might be worth its own GitHub issue. If we align with Chrome, the enqueue algorithm should detail this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

### Members ### {#RTCEncodedAudioFrameMetadata-members}
<dl data-link-for="RTCEncodedAudioFrameMetadata"
data-dfn-for="RTCEncodedAudioFrameMetadata"
class="dictionary-members">
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we anticipate to add more metadata common to video and audio, we might want to introduce a RTCEncodedFrameMetata dictionary that audio and video metadata dictionaries would extend.
Let's think about this once this PR is done.

index.bs Outdated Show resolved Hide resolved
@fippo fippo marked this pull request as ready for review October 3, 2022 09:44
@fippo
Copy link
Collaborator Author

fippo commented Oct 3, 2022

The argument in favor of the ugly HTML is that it is consistent with stats (hopefully also visually but we'll only see after merging I think). Now that it is there i expect this to be just copy-pasted on additions.

@fippo
Copy link
Collaborator Author

fippo commented Oct 20, 2022

can we merge this so PRs like #140 and #155 can be updated with proper descriptions?

@aboba aboba merged commit c85fc84 into w3c:main Oct 20, 2022
github-actions bot added a commit that referenced this pull request Oct 20, 2022
SHA: c85fc84
Reason: push, by aboba

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@fippo fippo deleted the describe-all-the-things branch October 21, 2022 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants