Skip to content

🚀 [Story video] Use the inlined video response instead of issuing an XHR request, for the 1st video of the 1st web story page#37499

Merged
coreymasanto merged 25 commits intoampproject:mainfrom
coreymasanto:inlinedVideo
Feb 14, 2022
Merged

🚀 [Story video] Use the inlined video response instead of issuing an XHR request, for the 1st video of the 1st web story page#37499
coreymasanto merged 25 commits intoampproject:mainfrom
coreymasanto:inlinedVideo

Conversation

@coreymasanto
Copy link
Copy Markdown
Contributor

@coreymasanto coreymasanto commented Jan 27, 2022 •
edited
Loading

Google video cache will be inlining the 1st video in the 1st story page, removing the need for an XHR call. This PR prevents the XHR request for this video whenever the inlined response is present.

Fixes #37397

@coreymasanto coreymasanto marked this pull request as ready for review January 27, 2022 16:16
@coreymasanto
Copy link
Copy Markdown
Contributor Author

coreymasanto commented Jan 27, 2022

I have tested this PR on a local server by doing the following steps:

  1. Add the following script within the <head> of videos-google-cache.html.
<script id="amp-google-video-cache-response" type="application/json">
    {
      "sources":  [
        {
          "url": "http://inlined_vid.mov",
          "codec": "h264",
          "type": "video/mp4",
          "bitrate_kbps": 400
        }
      ]
    }
  </script>
  1. Add a second video child to the first <amp-story-page>
  2. Go to videos-google-cache.html and verify that only the first video on the first page uses the inlined video source, and that an XHR request is issued for all other videos in the story.

@coreymasanto
Copy link
Copy Markdown
Contributor Author

coreymasanto commented Jan 27, 2022 •
edited
Loading

I think my current logic won't work when there are multiple grid layers on the first page that each have its own video child, because I would expect such videos to all match the amp-story-page:first-of-type amp-video:first-of-type selector. Do you know which video will be inlined by Google video cache in such a scenario? I'm guessing that it will always be the video element whose ancestor appears first, such as vid1 in this example:

<amp-story-page>
    <amp-story-grid-layer>
        <div>
            <div>
                <amp-video id="vid1">...</amp-video>
            </div>
        </div>
    </amp-story-grid-layer>

    <amp-story-grid-layer>
        <amp-video id="vid2">...</amp-video>
    </amp-story-grid-layer>

    <amp-video id="vid3">...</amp-video>
</amp-story-page>

gmajoulet
gmajoulet reviewed Jan 27, 2022
View reviewed changes
gmajoulet
gmajoulet reviewed Jan 28, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@gmajoulet gmajoulet left a comment

Choose a reason for hiding this comment

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

Can you write unit tests for this feature?
https://github.com/ampproject/amphtml/blob/main/extensions/amp-video/0.1/test/test-video-cache.js

I'd love to see at least:

  • correctly adds sources from inline config
  • doesn't send the XHR request if inline config
  • sends XHR request if non first video or non first page
  • sends XHR request if inline config (not provided|fails to parse|doesn't have the right data)
  • etc

@coreymasanto coreymasanto self-assigned this Feb 1, 2022
gmajoulet
gmajoulet reviewed Feb 10, 2022
View reviewed changes
@coreymasanto coreymasanto enabled auto-merge (squash) February 11, 2022 18:23
@coreymasanto coreymasanto merged commit c8ea23b into ampproject:main Feb 14, 2022
@coreymasanto coreymasanto deleted the inlinedVideo branch February 14, 2022 18:23
@processprocess processprocess mentioned this pull request Feb 14, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gmajoulet gmajoulet gmajoulet approved these changes

Assignees

@coreymasanto coreymasanto

Labels

PR use: In Beta / Experimental PR use: In LTS PR use: In Stable

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Story video] Use inlined video cache responses

3 participants

@coreymasanto @gmajoulet @ampprojectbot