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

CMCD nor field should be "more" relative to the current request #3741

Closed
5 tasks done
mgreve-akamai opened this issue Aug 25, 2021 · 7 comments
Closed
5 tasks done

CMCD nor field should be "more" relative to the current request #3741

mgreve-akamai opened this issue Aug 25, 2021 · 7 comments
Assignees
Labels
Milestone

Comments

@mgreve-akamai
Copy link

Environment
Steps to reproduce

Play the stream on the CMCD sample page: https://reference.dashif.org/dash.js/nightly/samples/advanced/cmcd.html

Observed behavior

The CMCD nor field is always sent relative to the root of the URL, which is somewhat inefficient. Here's an example from the textarea on the CMCD sample page:

type: video
file: bbb_30fps_3840x2160_12000k_15.m4v
bl : 56000
br : 14931
cid : 21cf726cfe3d937b5f974f72bb5bd06a
d : 4000
dl : 56000
mtp : 171600
nor : /akamai/bbb_30fps/bbb_30fps_3840x2160_12000k/bbb_30fps_3840x2160_12000k_16.m4v
ot : v
rtp : 5400
sf : d
sid : b248658d-1d1a-4039-91d0-8c08ba597da5
st : v
tb : 14932
v : 1

type: audio
file: bbb_a64k_14.m4a
bl : 52100
br : 67
cid : 21cf726cfe3d937b5f974f72bb5bd06a
d : 4011
dl : 52100
mtp : 1500
nor : /akamai/bbb_30fps/bbb_a64k/bbb_a64k_15.m4a
ot : a
rtp : 100
sf : d
sid : b248658d-1d1a-4039-91d0-8c08ba597da5
st : v
tb : 67
v : 1

Console output

Irrelevant

Expected behavior

In the two cases above, the most efficient value (in terms of request size) for the nor field is:
nor: bbb_30fps_3840x2160_12000k/bbb_30fps_3840x2160_12000k_16.m4v
nor: bbb_a64k_15.m4a

For reference, here's one full URL: https://dash.akamaized.net/akamai/bbb_30fps/bbb_a64k/bbb_a64k_1.m4a, where you can see that the /akamai/bbb_30fps/ components are always present, and hence don't need to be repeated in the nor field. Technically, what dash.js is doing is not incorrect - rather it's not efficient given that URLs in practice can be very long.

@dsilhavy dsilhavy self-assigned this Aug 25, 2021
@dsilhavy dsilhavy added this to the 4.1.0 milestone Aug 25, 2021
@dsilhavy
Copy link
Collaborator

I will keep this in the backlog. In my opinion this is low priority, I fear that there are a lot of edge cases like for instance different levels of pathnames for the representations. Right now I prefer the solution we have in place which might create a bit of overhead but will deliver valid data

@dsilhavy dsilhavy removed this from the 4.1.0 milestone Aug 27, 2021
@wilaw
Copy link
Member

wilaw commented Aug 30, 2021

Actually there are two problems here which should be fixed immediately as they are not spec compliant.

  1. The spec says that the field value MUST be relative to the current requested object, not relative to the host as they currently are. So in the case shown in this issue, for a GET /akamai/bbb_30fps/bbb_a64k/bbb_a64k_14.m4a, the 'nor' field must hold a value of 'bbb_a64k_15.m4a'.
  2. The spec also says that the nor value MUST be URLEncoded, which dash.js is currently not doing.

Both of these issues will cause a CDN to miss-construct the prefetch request.

Please add this issue back to 4.1.0 milestone.

@dsilhavy dsilhavy added this to the 4.1.0 milestone Aug 30, 2021
@dsilhavy
Copy link
Collaborator

dsilhavy commented Sep 1, 2021

@mgreve-akamai @wilaw Can I please ask you to have a look at the following unit tests and confirm that this is the expected path. a is the current url while b is the absolute url of the next segment. For me everything seems clear except the last two test.

        it('Should return complete url if no original url is given', () => {
            const b = 'https://localhost:3000/d/e/f.mp4';

            expect(Utils.getRelativeUrl(undefined,b)).to.be.equal('https://localhost:3000/d/e/f.mp4');
        })

        it('Should return relative url if strings are different after server name', () => {
            const a = 'https://localhost:3000/a/b/c.mp4';
            const b = 'https://localhost:3000/d/e/f.mp4';

            expect(Utils.getRelativeUrl(a,b)).to.be.equal('d/e/f.mp4');
        })

        it('Should return relative url if strings are similar up to one element before filename', () => {
            const a = 'https://localhost:3000/a/b/c.mp4';
            const b = 'https://localhost:3000/a/c/f.mp4';

            expect(Utils.getRelativeUrl(a,b)).to.be.equal('c/f.mp4');
        })

        it('Should return relative url if strings are similar up to filename', () => {
            const a = 'https://localhost:3000/a/b/c.mp4';
            const b = 'https://localhost:3000/a/b/f.mp4';

            expect(Utils.getRelativeUrl(a,b)).to.be.equal('f.mp4');
        })

        it('Should return complete url if origin is different', () => {
            const a = 'https://localhost:3000/a/b/c.mp4';
            const b = 'https://loca:3000/a/b/f.mp4';

            expect(Utils.getRelativeUrl(a,b)).to.be.equal('https://loca:3000/a/b/f.mp4');
        })

        it('Should return relative url if part of the pathnames are not equal', () => {
            const a = 'https://localhost:3000/a/b/c.mp4';
            const b = 'https://localhost:3000/ab/b/f.mp4';

            expect(Utils.getRelativeUrl(a,b)).to.be.equal('ab/b/f.mp4');
        })

        it('Should return relative url if target pathname is longer than source pathname ', () => {
            const a = 'https://localhost:3000/a/b/f.mp4';
            const b = 'https://localhost:3000/a/b/f/e/c.mp4';

            expect(Utils.getRelativeUrl(a,b)).to.be.equal('/a/b/f/e/c.mp4');
        })

        it('Should return relative url if source pathname is longer than target pathname ', () => {
            const a = 'https://localhost:3000/a/b/f/e/c.mp4';
            const b = 'https://localhost:3000/a/b/f.mp4';

            expect(Utils.getRelativeUrl(a,b)).to.be.equal('../../f.mp4');
        })

@mgreve-akamai
Copy link
Author

I looked over the examples, and I think a number of them are incorrect. I'll list them below:

I think this test is incorrect.

it('Should return relative url if strings are different after server name', () => {
            const a = 'https://localhost:3000/a/b/c.mp4';
            const b = 'https://localhost:3000/d/e/f.mp4';

            expect(Utils.getRelativeUrl(a,b)).to.be.equal('d/e/f.mp4');
})

It should be '/d/e/f.mp4' (with 'd/e/f.mp4' as in the test, the new url would be https://localhost:3000/a/b/d/e/f.mp4).

This one is also incorrect.

 it('Should return relative url if strings are similar up to one element before filename', () => {
            const a = 'https://localhost:3000/a/b/c.mp4';
            const b = 'https://localhost:3000/a/c/f.mp4';

            expect(Utils.getRelativeUrl(a,b)).to.be.equal('c/f.mp4');
})

It should be '../c/f.mp4' or '/a/c/f.mp4'

This one is also incorrect:

   it('Should return relative url if part of the pathnames are not equal', () => {
            const a = 'https://localhost:3000/a/b/c.mp4';
            const b = 'https://localhost:3000/ab/b/f.mp4';

            expect(Utils.getRelativeUrl(a,b)).to.be.equal('ab/b/f.mp4');
    })

It should be '../../ab/b/f.mp4' or '/ab/b/f.mp4' (the latter is shorter.

The second-to-last is longer than needed:

 it('Should return relative url if target pathname is longer than source pathname ', () => {
            const a = 'https://localhost:3000/a/b/f.mp4';
            const b = 'https://localhost:3000/a/b/f/e/c.mp4';

            expect(Utils.getRelativeUrl(a,b)).to.be.equal('/a/b/f/e/c.mp4');
        })

The shorter answer is 'f/e/c.mp4'.

This last one is correct:

 it('Should return relative url if source pathname is longer than target pathname ', () => {
            const a = 'https://localhost:3000/a/b/f/e/c.mp4';
            const b = 'https://localhost:3000/a/b/f.mp4';

            expect(Utils.getRelativeUrl(a,b)).to.be.equal('../../f.mp4');
        })

An unrelated point to the test cases: What if the scheme (https vs. https) changes? I'm wondering if you want tests for that and what's the intended behavior in this case (?).

Hopefully, I got all of them correct :)

@mgreve-akamai
Copy link
Author

mgreve-akamai commented Sep 1, 2021

I've written an implementation of this function, which is think is fairly straightforward and I think it's correct :). It has no error handling for the case of different hosts etc. It has the correct (and optimal) answers for the test cases. There may be an error or two in my comment above, which I spotted while building this (I have not edited my comment above). Feel free to adapt and use parts of this code if you think it's useful.

import { assert } from "console";

/**
 * Returns the shortest representation of the URL in the variable next as a relative URL in terms of the URL current. If no such
 * representation is possible, undefined will be returned.
 * 
 * Preconditions:
 * 1) Both current and next must be absolute URLs, and must be valid.
 * 2) Both must have the same scheme.
 * 3) Both must have the same host (taking into account default port numbers and such).
 **/
function getRelativeUrl(current: string, next: string): string {
	// Split by / and remove the scheme and host.
	let currentSplit = current.split("/").slice(3);
	const nextSplit = next.split("/").slice(3);
	let equalCount;
	for (equalCount = 0; equalCount < currentSplit.length -1 && equalCount < nextSplit.length - 1 && currentSplit[equalCount] == nextSplit[equalCount]; ++equalCount) { }
	// We know that the first equalCount components of currentSplit and nextSplit are equal.

	let candidate1 = "";

	// Traverse backwards in the current string until we hit the common prefix.
	for (let i = 0; i < currentSplit.length - equalCount - 1; ++i) candidate1 += "../";

	// Then go forwards in the next string until we have all parts except the filename (which could be empty).
	for (let i = equalCount; i < nextSplit.length - 1; ++i) candidate1 += nextSplit[i] + "/";

	// Now add the filename.
	candidate1 += nextSplit[nextSplit.length - 1];


	// Build the other candidate, e.g. the 'host relative' path that starts with "/", and return the shortest of the two candidates.
	const candidate2 = "/" + nextSplit.join("/");
	if (candidate2.length <= candidate1.length) return candidate2;
	return candidate1;
}

assert(getRelativeUrl('https://localhost:3000/a/b/c.mp4', 'https://localhost:3000/d/e/f.mp4') == "/d/e/f.mp4");
assert(getRelativeUrl('https://localhost:3000/a/b/c.mp4', 'https://localhost:3000/a/c/f.mp4') == "../c/f.mp4");
assert(getRelativeUrl('https://localhost:3000/a/b/c.mp4', 'https://localhost:3000/ab/b/f.mp4') == "/ab/b/f.mp4");
assert(getRelativeUrl('https://localhost:3000/a/b/f.mp4', 'https://localhost:3000/a/b/f/e/c.mp4') == "f/e/c.mp4");
assert(getRelativeUrl('https://localhost:3000/a/b/f/e/c.mp4', 'https://localhost:3000/a/b/f.mp4') == "/a/b/f.mp4"); // shorter than "../../f.mp4".
assert(getRelativeUrl('https://localhost:3000/a/b/c/d/e/f/g/h/1.mp4', 'https://localhost:3000/a/b/c/d/e/change/i/j/k/l/m/n/2.mp4') == "../../../change/i/j/k/l/m/n/2.mp4");
assert(getRelativeUrl('https://localhost:3000/a/', 'https://localhost:3000/a/b.mp4') == "b.mp4");
assert(getRelativeUrl('https://localhost:3000/a/b/c/', 'https://localhost:3000/a/b/x/c.mp4') == "../x/c.mp4");

@dsilhavy dsilhavy mentioned this issue Sep 2, 2021
@dsilhavy
Copy link
Collaborator

dsilhavy commented Sep 2, 2021

Thanks @mgreve-akamai that was very helpful. I used parts of your sample code for the implementation but moved to the path module:

   static getRelativeUrl(originalUrl, targetUrl) {
        try {
            const original = new URL(originalUrl);
            const target = new URL(targetUrl);

            // Unify the protocol to compare the origins
            original.protocol = target.protocol;
            if (original.origin !== target.origin) {
                return targetUrl;
            }

            // Use the relative path implementation of the path library. We need to cut off the actual filename in the end to get the relative path
            let relativePath = path.relative(original.pathname.substr(0, original.pathname.lastIndexOf('/')), target.pathname.substr(0, target.pathname.lastIndexOf('/')));

            // In case the relative path is empty (both path are equal) return the filename only. Otherwise add a slash in front of the filename
            const startIndexOffset = relativePath.length === 0 ? 1 : 0;
            relativePath += target.pathname.substr(target.pathname.lastIndexOf('/') + startIndexOffset, target.pathname.length - 1);

            // Build the other candidate, e.g. the 'host relative' path that starts with "/", and return the shortest of the two candidates.
            if (target.pathname.length < relativePath.length) {
                return target.pathname;
            }
            return relativePath;
        } catch (e) {
            return targetUrl
        }
    }

You can find the corresponding pull request here: #3747 . Please let me know if you encounter any issues. One specific thing to mention is that I treat origins with different protocols as if they have the same protocol (https vs http). If that should be changed let me know.

 it('Should return filename if origin differs in terms of SSL', () => {
     const a = 'https://localhost:3000/a/b/c.mp4';
     const b = 'http://localhost:3000/a/b/e.mp4';

     expect(Utils.getRelativeUrl(a,b)).to.be.equal('e.mp4');
})

@dsilhavy
Copy link
Collaborator

dsilhavy commented Sep 8, 2021

Closing this, please comment if you encounter any problems

@dsilhavy dsilhavy closed this as completed Sep 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants