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

Update index.bs with ChapterInformation #308

Merged
merged 48 commits into from
Apr 10, 2024

Conversation

jiajiabingcheng
Copy link
Contributor

@jiajiabingcheng jiajiabingcheng commented Dec 13, 2023

Added a ChapterInformation attribute in the MediaMetadata to carry the Youtube video chapter data:1) title of the section 2)timestamp of this section 3)screenshot image data of this section


Preview | Diff

Copy link
Contributor

@steimelchrome steimelchrome left a comment

Choose a reason for hiding this comment

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

There's a section of examples near the bottom of the spec. I think we should also add an example of adding chapter information

index.bs Outdated

The <dfn dict-member for="ChapterInformation">startTime</dfn> <a>dictionary member</a>
is used to specify the {{ChapterInformation}} object's {{MediaImage/startTime}} in
seconds. It should always be positive.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can it also be zero? I'd guess most first chapters would have a startTime of zero

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed it to "should not be negative."

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@jiajiabingcheng
Copy link
Contributor Author

Updated the example section. Also updated the explainer.md in this pr.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
aarongable pushed a commit to chromium/chromium that referenced this pull request Dec 21, 2023
ChapterInformation will be added to the MediaMetadata
Github pr: w3c/mediasession#308

This cl implements it on the blink layer.

Bug: b/316045178
Change-Id: Ic06140e1aed1094d3db68fd167c9acf14c3cd2fa
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5124302
Commit-Queue: Alex Newcomer <[email protected]>
Reviewed-by: Xianzhu Wang <[email protected]>
Reviewed-by: Mohamed Heikal <[email protected]>
Reviewed-by: Joe Mason <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1240183}
explainer.md Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
explainer.md Outdated Show resolved Hide resolved
index.bs Outdated
};

dictionary MediaMetadataInit {
DOMString title = "";
DOMString artist = "";
DOMString album = "";
sequence&lt;MediaImage> artwork = [];
sequence&lt;ChapterInformation> chapterInfo = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

If we introduce a ChapterInformationInit, this could be a ChapterInformationInit instead (which would allow passing either ChapterInformationInit or ChapterInformation here).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Shall I also use ChapterInformationInit in the MediaMetadata interface declaration(L951)?

index.bs Outdated
better to do this with IDL primitives instead of JS - see
https://www.w3.org/Bugs/Public/show_bug.cgi?id=29004 -->
<li>
Call <a lt="freeze">Object.freeze</a> on <var>image</var>, to prevent
Copy link
Member

Choose a reason for hiding this comment

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

This seems not great... yeah, maybe this should be an interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have changed ChapterInformation to an interface in this pr. For MediaImage , it might be better to submit a separate pr under the corresponding issue.

index.bs Outdated
getting, it MUST return the result of the following steps:
<ol>
<li>
Let <var>frozenArtwork</var> be an empty list of type {{MediaImage}}.
Copy link
Member

Choose a reason for hiding this comment

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

Still kinda not great that we have ImageResource and MediaImage...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are they referring to the same thing? I see we are using MediaImage in MediaMetadata, so I also use it in the ChapterInformation.

Copy link
Contributor

@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.

explainer.md Outdated Show resolved Hide resolved
explainer.md Outdated Show resolved Hide resolved
};

dictionary MediaMetadataInit {
DOMString title = "";
DOMString artist = "";
DOMString album = "";
sequence&lt;MediaImage> artwork = [];
sequence&lt;ChapterInformationInit> chapterInfo = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not see ChapterInformationInit being defined here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is defined at L1184, can I use it here?

Copy link
Contributor

Choose a reason for hiding this comment

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

This whole section is somehow redundant with web idl.
Since we use ChapterInformationInit, it seems to make sense to add it there as well, since it seems this would be the only one missing here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add ChapterInformationInit for consistency here

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
explainer.md Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated

The <dfn dict-member for="ChapterInformation">startTime</dfn> <a>dictionary member</a>
is used to specify the {{ChapterInformation}} object's {{MediaImage/startTime}} in
seconds. It should not be negative.
Copy link
Member

Choose a reason for hiding this comment

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

Avoid "should" and describe what MUST happen if it is negative.

This should be done in a ChapterInformation(init) constructor (similar to MediaMetadata(init)), which should throw TypeError on invalid input, as well as run the convert artwork algorithm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated
Comment on lines 1192 to 1201
The <dfn dict-member for="ChapterInformation">title</dfn> <a>dictionary member</a> is
used to specify the {{ChapterInformation}} object's {{MediaImage/title}}.

The <dfn dict-member for="ChapterInformation">startTime</dfn> <a>dictionary member</a>
is used to specify the {{ChapterInformation}} object's {{MediaImage/startTime}} in
seconds. It should not be negative.

The <dfn dict-member for="ChapterInformation">artwork</dfn> <a>dictionary member</a>
is used to specify the {{ChapterInformation}} object's {{MediaImage/artwork}}. On
getting, it MUST return the result of the following steps:
Copy link
Member

Choose a reason for hiding this comment

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

Dictionary members cannot have getters or setters.

This section appears to conflate the dictionary members with the interface attributes, which is confusing. We should describe them separately like we do for MediaMetadata.

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 have changed it to an interface now as another comment suggested. I also followed the MediaMetadata 's description and rewrote this part in the latest commit :]

index.bs Outdated
Comment on lines 1204 to 1211
Let <var>frozenArtwork</var> be an empty list of type {{MediaImage}}.
</li>
<li>
For each <var>entry</var> in the {{ChapterInformation}}'s <a
for="ChapterInformation">artwork images</a>, perform the following steps:
<ol>
<li>
Let <var>image</var> be a new {{MediaImage}}.
Copy link
Member

@jan-ivar jan-ivar Jan 26, 2024

Choose a reason for hiding this comment

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

Creating new objects in a getter is an anti-pattern, especially in a frozen array. It violates § 6.1. Attributes should behave like data properties. See also crbug 94665.

I understand this is copied from https://w3c.github.io/mediasession/#dom-mediametadata-artwork but that is also wrong and redundant since the MediaMetadata(init) already runs "the convert artwork algorithm with init’s artwork as input and set metadata’s artwork images as the result if it succeeded."

IOW we ran convert (instantiated objects) on set, so no need to run it again (instantiating more objects) on every get. The attribute getter can simply return artwork images.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to:
"The artwork attribute reflects the ChapterInformation's artwork images. On getting, it MUST return the ChapterInformation's artwork. On setting, it MUST run the convert artwork algorithm with the new value as input, and set the ChapterInformation's artwork images as the result if it succeeded."
Let me know what you think.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated
Comment on lines 1244 to 1246
On setting, it MUST run the <a>convert artwork algorithm</a> with the new value as
<var>input</var>, and set the {{ChapterInformation}}'s <a for="ChapterInformation">artwork
images</a> as the result if it succeeded.
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be in the constructor algorithm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

jiajiabingcheng and others added 2 commits January 26, 2024 15:59
Co-authored-by: Jan-Ivar Bruaroey <[email protected]>
Co-authored-by: Jan-Ivar Bruaroey <[email protected]>
@jiajiabingcheng
Copy link
Contributor Author

Hey @jan-ivar, thanks for the review. Can you PTAL again at the latest patchset?

explainer.md Show resolved Hide resolved
index.bs Show resolved Hide resolved
jiajiabingcheng and others added 2 commits February 6, 2024 13:35
Co-authored-by: François Daoust <[email protected]>
Co-authored-by: François Daoust <[email protected]>
constructor, when invoked, MUST run the following steps:

<ol>
<li>
Copy link
Member

Choose a reason for hiding this comment

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

... and NaN (similar to the steps for setPositionState)

@jiajiabingcheng
Copy link
Contributor Author

Hey @chrisn, @jan-ivar, thanks for the previous review. Can you PTAL again at the latest patchset?

index.bs Show resolved Hide resolved
Co-authored-by: Chris Needham <[email protected]>
@jiajiabingcheng
Copy link
Contributor Author

Hey @chrisn, @jan-ivar, thanks for the previous review. Can you PTAL again at the latest patchset?

Copy link
Member

@chrisn chrisn left a comment

Choose a reason for hiding this comment

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

I added one minor comment, but apart from that, it all looks good to me. Thank you!

explainer.md Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
Copy link
Member

@jan-ivar jan-ivar left a comment

Choose a reason for hiding this comment

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

LGTM with some nits and the fix from @tidoust! Thanks!

index.bs Outdated Show resolved Hide resolved
index.bs Outdated
Comment on lines 1245 to 1246
Run the <a>convert artwork algorithm</a> with <var>init</var>'s
{{ChapterInformationInit/artwork}} as <var>input</var> and set
Copy link
Member

Choose a reason for hiding this comment

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

This looks good but requires fixing the convert artwork algorithm algorithm to expect artwork instead of input (right now it still says "For each entry in input’s artwork, perform the following steps:") (plus fixing the other call to that algorithm).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we rephrase this paragraph to:
"Run the convert artwork algorithm with init’s artwork as input and create a frozen array from the result, then set it to chapterInfo’s artwork images." ?

Do we need to change the other places? For this sentence "When the convert artwork algorithm with input parameter is invoked, the user agent MUST run the following steps:", isn't input the init's artwork list which need to be parsed with the algorithm? And then it can generate the output list?

Copy link
Member

@jan-ivar jan-ivar Feb 23, 2024

Choose a reason for hiding this comment

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

Clicking on the link in "For each entry in input’s artwork" reveals the expected type of input to be MediaMetadataInit. I.e. A's B is a deref. To take a sequence, e.g. artwork instead of {artwork}, it needs to say "For each entry in input".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to "For each entry in input (which is a MediaImage list), perform the following steps:"

@jiajiabingcheng
Copy link
Contributor Author

Thanks @chrisn and @jan-ivar for the approval!

Hi @youennf, can you PTAL at the latest patchset? Let me know if anything is missing :] Thanks!

explainer.md Outdated
[Exposed=Window]
interface ChapterInformation {
attribute DOMString title;
attribute double startTime;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these two be readonly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


dictionary ChapterInformationInit {
DOMString title = "";
double startTime = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if there are two chapter information with the same startTime?
Should we define which one is selected using the sequence order?
Also, it seems we somehow expect that the web page provides a sequence ordered by startTime but I would guess it is fine to not do so.

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 believe it is unlikely that there will be more than one chapter with the same start time. However, if this does occur, we can simply use the chapter list as normal and sort it by start time after we have obtained it. Because the image sources are associated with each chapterInformation, there should be no problems regardless of the order in the list. Then in the UI, some chapters with the same start time will be displayed next to each other.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed this is an edge case, let's make sure that even these corner cases get implemented in an interoperable manner. So if we have to use list order to disambiguate, let's define it here.

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'm thinking the website should be able to set whatever information is with the media. Even if there are two or more chapters with the same start time, there should be no order from them because they all refer to the same point in the video. Also, would it be better to leave this up to the client side? When the client receives a list of chapters, they can order them by start time first. Then, it is up to them whether or not to display them in any order. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey youennf, we're moving forward with submitting this pull request. Please feel free to open an issue later if you still have any additional questions about this change. Thanks :]

};

dictionary MediaMetadataInit {
DOMString title = "";
DOMString artist = "";
DOMString album = "";
sequence&lt;MediaImage> artwork = [];
sequence&lt;ChapterInformationInit> chapterInfo = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

This whole section is somehow redundant with web idl.
Since we use ChapterInformationInit, it seems to make sense to add it there as well, since it seems this would be the only one missing here.

index.bs Outdated
Run the <a>convert artwork algorithm</a> with <var>init</var>'s
{{ChapterInformationInit/artwork}} as <var>input</var> and set
<var>chapterInfo</var>'s <a for="ChapterInformation">artwork images</a> to the
result of [=Create a frozen array|creating a frozen array=] from the result.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is good as is I think but it could be made slightly clearer by splitting in two steps:

  • Let artWork be the result of running the convert artwork algorithm ...
  • Set chapterInfo.... to the result of creating a frozen array from artWork.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@jiajiabingcheng
Copy link
Contributor Author

@youennf Thanks for the review!
I will add the chapter_information_init.idl under blink soon!
PTAL at the latest patchset and see if you have more comments :]

@jiajiabingcheng
Copy link
Contributor Author

Hi @youennf, can you PTAL at the latest patchset and see if you have more comments? The corresponding change is already landed under blink/.

@steimelchrome steimelchrome merged commit c7a47fb into w3c:main Apr 10, 2024
1 check passed
github-actions bot added a commit that referenced this pull request Apr 10, 2024
SHA: c7a47fb
Reason: push, by steimelchrome

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.

7 participants