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

feat: LCEVC integration #4050

Merged
merged 25 commits into from
Oct 3, 2022
Merged

Conversation

fabio-murra
Copy link
Contributor

@fabio-murra fabio-murra commented Mar 22, 2022

Integration of MPEG-5 Part-2 LCEVC into Shaka Player.

A config must be enabled and a canvas element must be provided.

The Shaka Player UI will automatically provide an appropriate canvas.

Description

Integration of MPEG-5 Part-2 LCEVC into Shaka Player workflow as a config option to be enabled and the necessary configurations and elements to be passed through the constructor.
image

Documentation

All integration related documentation is available at docs/design/lcevc-integration.md

Executed Tasks

  • Signed Google CLA
  • Builds build/all.py successfully
  • Runs build/test.py successfully

@google-cla
Copy link

google-cla bot commented Mar 22, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

For more information, open the CLA check for this pull request.

@avelad
Copy link
Member

avelad commented Mar 22, 2022

@fabio-murra One question, would this work on which browsers (minimum versions), Tizen, WebOS, Xbox?

@fabio-murra
Copy link
Contributor Author

@avelad , good question. LCEVC requires WebASM & WebGL. The requirements are listed here: https://docs.v-nova.com/v-nova/lcevc/integrations/html5-integration#supported-browsers-and-platforms

@avelad
Copy link
Member

avelad commented Mar 22, 2022

I think something needs to be done to prevent this from being enabled in unsupported browsers. Right now I see that some versions of Tizen and WebOS do not support it.
See: https://developer.samsung.com/smarttv/develop/specifications/web-engine-specifications.html and https://webostv.developer.lge.com/discover/specifications/web-engine/

@sortonjay
Copy link

@fabio-murra One question, would this work on which browsers (minimum versions), Tizen, WebOS, Xbox?

Hi @avelad, thanks again for the feedback. I'm a product manager at V-Nova and will help with some of the discussion. We have checks within the minified LCEVC DIL library that is loaded as a dependency to check for the support of WASM and other requirements before the LCEVC decoder is created which ensure that the LCEVC decode pipeline is not activated when it is not possible. LCEVC is backwards compatible so playback will proceed with the unenhanced base codec stream in this scenario. We've taken this approach to minimize changes in Shaka itself. Does this approach work for you?

We do not currently support Tizen and WebOS and plan to add those browser based checks within the library itself shortly to once again prevent the LCEVC decode pipeline from activating. Please let me know if this resolves your comment. Many thanks.

Comment on lines 8 to 10
Shaka Player is an open-source JavaScript library for adaptive media. It plays adaptive media formats (such as [DASH](http://dashif.org/) and [HLS](https://developer.apple.com/streaming/)) in a browser, without using plugins or Flash. Instead, Shaka Player uses the open web standards [MediaSource Extensions](https://www.w3.org/TR/media-source/) and [Encrypted Media Extensions](https://www.w3.org/TR/encrypted-media/).

The official project GitHub repository is <https://github.com/google/shaka-player>. This integration is a fork from the Shaka Player open source project from version 3.3.0-pre with commit hash: 73b430248ba76038b466da902b776ecc920b2a35.
Copy link
Member

Choose a reason for hiding this comment

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

I think these lines can be dropped for this PR to be merged into the repo.

Copy link
Contributor

Choose a reason for hiding this comment

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

These lines are deleted.


In order to import the necessary V-Nova libraries we followed the approach that other externals libraries are using. The necessary V-Nova files need to be imported in the HTML page that is going to use the Shaka Player library.

We have at as an npm package : <https://www.npmjs.com/package/lcevc_dil.js>
Copy link
Member

Choose a reason for hiding this comment

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

Typo? "We have that as..."?

Copy link
Contributor

Choose a reason for hiding this comment

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

Typos Fixed.

Comment on lines 35 to 37
Externs are declarations that tell Closure Compiler the names of symbols that should not be renamed during advanced compilation. They are called externs because these symbols are most often defined by code outside the compilation, such a native code, or third-party libraries. For this reason, externs often also have type annotations, so that Closure Compiler can type check your use of those symbols.

In general, it is best to think of externs as an API contract between the implementor and the consumers of some piece of compiled code. The externs define what the implementor promises to supply, and what the consumers can depend on using. Both sides need a copy of the contract. Externs are similar to header files in other languages.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not certain this needs to be explained in context of the shaka-player project.

Copy link
Contributor

Choose a reason for hiding this comment

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

These lines are deleted.

externs/lcevc.js Outdated
*
* @externs
*/
'use strict';
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't do anything in externs AFAIK. They are only processed by the compiler for type info, not executed or included in compiled output.

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

externs/lcevc.js Outdated
/**
* @param {HTMLVideoElement} media
* @param {HTMLCanvasElement} canvas
* @param {Object=} dilConfig
Copy link
Member

Choose a reason for hiding this comment

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

I think we should be more specific about this. Object should be avoided if we can instead define the fields that are expected and what types they should contain.

Optional fields can be unions with undefined. For example: {(number|undefined)}

Copy link
Contributor

@v-nova-vinod v-nova-vinod Apr 7, 2022

Choose a reason for hiding this comment

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

Created a more specific type for the dilConfig in externs/lcevc.js - LcevcDil.LcevcDilConfig which is used for any further references of the dilConfig

Comment on lines 14 to 15
* lcevcDil provides all the operations related to the enhancement and rendering
* of LCEVC enabled streams and on to a canvas.
Copy link
Member

Choose a reason for hiding this comment

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

Readers have no idea why this is called "DIL". Can we either explain that or give it a clearer name?

Copy link
Contributor

Choose a reason for hiding this comment

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

We have added more documentation to this and also the details about what DIL stands for.

lib/player.js Outdated
Comment on lines 752 to 755
if (this.lcevcDil_.media_) {
this.lcevcDil_.media_.style.display = 'block';
this.lcevcDil_.canvas_.style.display = 'none';
}
Copy link
Member

Choose a reason for hiding this comment

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

You shouldn't reach into private variables of another class. Besides, we have this.video_ and this.canvas_, so you don't need to.

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 rectified (removed).

lib/player.js Outdated
this.lcevcDil_.canvas_.style.display = 'none';
}
this.lcevcDil_.close();
delete this.lcevcDil_;
Copy link
Member

Choose a reason for hiding this comment

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

Don't both delete and set to null. Setting to null should suffice for GC, and we generally don't delete fields.

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

lib/player.js Outdated Show resolved Hide resolved
@@ -78,6 +78,16 @@ shaka.util.Dom = class {
return shaka.util.Dom.asHTMLElement(elements[0]);
}

/**
* Returns the element with a given class name.
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 unnecessary. Why not call it directly?

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

@avelad
Copy link
Member

avelad commented Apr 22, 2022

@joeyparrish can you review it again?

@joeyparrish
Copy link
Member

joeyparrish commented Apr 22, 2022

Yes. I got half-way through it yesterday, and I'm hoping to complete it today on or Monday.

externs/lcevc.js Outdated
*
* @description LCEVC DIL Custom Config that can be passed
* through the constructor.
* @property {number} logLevel // LogLevel for LCEVC DIl Logs. Defaults to 0.
Copy link
Member

Choose a reason for hiding this comment

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

Please remove the // here. This is already in a block comment from the point of view of the compiler, and we don't need those comment characters in any generated docs.

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

*/
shaka.lcevc.Dil = class {
/**
* @param {HTMLMediaElement} media The video element that will be attached to
* LCEVC Dil for input.
* @param {HTMLElement} canvas The canvas element that will be attached to
* LCEVC Dil to render the enhanced frames.
* @param {Object} dilConfig The LCEVC DIL config object
* to initialize the LCEVC DIL.
* @param {LcevcDil.LcevcDilConfig | undefined | null} dilConfig The LCEVC DIL
Copy link
Member

Choose a reason for hiding this comment

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

This could be simplified to an optional parameter, like {LcevcDil.LcevcDilConfig=}, and default to null in the parameter list.

However, see my comments below on this.dilConfig_. I think for internal use, you should make this a non-nullable, non-optional parameter.

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

Comment on lines 44 to 45
this.media_ = /** @type {HTMLVideoElement} */ (media);
this.canvas_ = /** @type {HTMLCanvasElement} */ (canvas);
Copy link
Member

Choose a reason for hiding this comment

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

Instead of casting these on assignment, why not fix the types of the constructor parameters to match what you really want?

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 now removed. Just the typecasting of the media HTMLMediaElement to HTMLVideoElement is moved to player.js

Comment on lines 46 to 54
this.dilConfig_ = /** @type {LcevcDil.LcevcDilConfig} */ (dilConfig);
if (!this.dilConfig_) {
this.dilConfig_ = {};
}
// This line is optional forcing the config to hide the LCEVC watermark
// that is displayed on the left hand side top corner.
if (!this.dilConfig_['logo']) {
this.dilConfig_['logo'] = false;
}
Copy link
Member

Choose a reason for hiding this comment

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

None of this is necessary if you have types defined and require fields to be set. The compiler will enforce the types, and so long as this is an internal class that isn't exposed to the application directly, you can rely on the types and fields being set correctly when called. As it stands now, your externs don't allow any fields to be left out.

Copy link
Contributor

Choose a reason for hiding this comment

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

We have removed this and created a constant with the default parameters in case of config being null or undefined

Comment on lines 188 to 191
* @param {HTMLElement} lcevcContainer
* @param {LcevcDil.LcevcDIL} lcevcDil
* @param {HTMLVideoElement} mediaElement
* @param {HTMLCanvasElement} canvas
Copy link
Member

Choose a reason for hiding this comment

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

This method is a member, and is only ever called with the same parameters taken from this. The parameters don't seem necessary at all.

Am I overlooking some reason it should be parameterized like this?

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 removed. This function was created for a different use case where parameters were passed, but in current scenario its not required.

@@ -76,6 +76,7 @@ shaka.media.MediaSourceEngine = class {
number, ?number)} */
this.onMetadata_ = onMetadata || onMetadataNoOp;

/** @private {shaka.lcevc.Dil | undefined} */
Copy link
Member

Choose a reason for hiding this comment

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

The | undefined part appears to be unnecessary. You never set it to undefined, and it's nullable by default.

Copy link
Contributor

@v-nova-vinod v-nova-vinod Apr 27, 2022

Choose a reason for hiding this comment

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

In MediaSourceEngine, lcevcDil shaka.lcevc.Dil is an optional parameter. Hence the undefined, for instances where lcevcDil is not passed when creating MediaSourceEngine. would that be ok? or do you want all the creation of MediaSourceEngine include a null value for lcevcDil where not applicable?

Copy link
Member

Choose a reason for hiding this comment

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

It's just style at this point, but normally we would make this nullable, and set it to null when it's not given. Something like:

/** @private {?shaka.lcevc.Dil} */
this.lcevcDil_ = lcevcDil || null;

@avelad avelad added the type: enhancement New feature or request label May 2, 2022
@avelad avelad requested a review from joeyparrish May 3, 2022 09:47
@avelad
Copy link
Member

avelad commented May 5, 2022

@v-nova-vinod I see that there are conflicts to be able to merge, can you rebase? Thanks!

@vinod-balakrishnan
Copy link
Contributor

@avelad I have rebased to the latest main.

@avelad
Copy link
Member

avelad commented May 5, 2022

OK, I'll run the github actions (build and tests)

@avelad
Copy link
Member

avelad commented May 5, 2022

@vinod-balakrishnan It seems that there are errors in the tests, can you correct the errors?

@vinod-balakrishnan
Copy link
Contributor

@avelad I think all the errors were due to a null mediaSourceEngine, for which I have now put a check in. can you please run the tests again? thanks.

@avelad
Copy link
Member

avelad commented May 5, 2022

I've run them again but they keep having erros. Try running them locally with ./build/test.py

@vinod-balakrishnan
Copy link
Contributor

@avelad All tests have now passed. the issue was the creation of the jasmine.Spy for updateLcevcDil in fake_media_source used for unit tests.

@vinod-balakrishnan
Copy link
Contributor

Hi @joeyparrish @avelad, What are the next steps on this. I see that the Selenium script that failed is not related to the LCEVC changes. Can you please advice on the next steps. Thanks!

@avelad
Copy link
Member

avelad commented May 11, 2022

It has to pass a review of @joeyparrish, I know he's busy, but hopefully he can review it as soon as possible (no date known)


`constructor(media, canvas, dilConfig):` It receives the video element (media), the canvas, and the Dil configuration as a JSON string and creates the LcevcDil object.

`appendBuffer(data, type, level):` Appends the MP4 fragments before the they are appended to the Media Source Extensions SourceBuffer.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: extra "the" here. "before the they" -> "before they"

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 rectified

* which is called to inject mocks into the Player. Used for testing.
* @param {?HTMLCanvasElement} [canvas] Optional canvas where LCEVC
* video will be drawn.
* @param {?Object} [lcevcDilConfig] Optional canvas where LCEVC
Copy link
Contributor

Choose a reason for hiding this comment

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

This JSDoc is the same as the parameter above. Also, this is no longer a generic Object in the actual code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Removed code blocks from this code for the ease of maintenance.

lib/player.js Outdated
@@ -399,8 +400,12 @@ shaka.Player = class extends shaka.util.FakeEventTarget {
* will remain detached.
* @param {function(shaka.Player)=} dependencyInjector Optional callback
* which is called to inject mocks into the Player. Used for testing.
* @param {?HTMLCanvasElement} [canvas] Optional canvas where LCEVC
Copy link
Contributor

Choose a reason for hiding this comment

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

This parameter, and lcevcDilConfig below, should probably be optional, if they are going to be parameters on the Player constructor. Especially since they are coming after other optional parameters.
E.g. HTMLCanvasElement= and LcevcDil.LcevcDilConfig=.

Copy link
Contributor

Choose a reason for hiding this comment

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

Changed this.

lib/player.js Outdated
// This is the canvas element that will be used for rendering LCEVC
// enhanced frames.
/** @private {?HTMLCanvasElement} */
this.canvas_ = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

I would specify, in the variable name, that this is for lcevc. The name canvas is quite generic, and doesn't really describe why we want to have a canvas.

Copy link
Contributor

Choose a reason for hiding this comment

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

Name for this is changed from canvas_ to lcevcCanvas_

lib/player.js Outdated
@@ -399,8 +400,12 @@ shaka.Player = class extends shaka.util.FakeEventTarget {
* will remain detached.
* @param {function(shaka.Player)=} dependencyInjector Optional callback
* which is called to inject mocks into the Player. Used for testing.
* @param {?HTMLCanvasElement} [canvas] Optional canvas where LCEVC
* video will be drawn.
* @param {LcevcDil.LcevcDilConfig} [lcevcDilConfig] LCEVC DIL
Copy link
Contributor

Choose a reason for hiding this comment

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

I can understand why the lcevc canvas is in the parameters of the constructor- it's the same place we are putting the media element.
However, I'm not sure if the constructor is the right place to put the LcevcDil.LcevcDilConfig config. After all, that config is only used once we are well into loading an element, by which time we could probably expect the user to have called Player.configure(), so you could put the lcevc config into the Shaka Player configuration instead.

I would make a field in shaka.extern.PlayerConfiguration for a LcevcDil.LcevcDilConfig object, called lcevcDilConfig.
This would also require adding the LCEVC configuration values into the demo player (and the demo player's localized strings), but that would also let people test these configuration values in the demo so it'd be worth it.

Copy link
Contributor

Choose a reason for hiding this comment

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

We did have a PlayerConfiguration option for LCEVC in the first commit in this branch, having just the enable flag for LCEVC. Then we removed that config option as suggested by @joeyparrish to be triggered when the file is found and not to be dependent on the configuration option. Are you ok if the same approach is taken for LcevcDil.LcevcDilConfig in PlayerConfiguration ?

Copy link
Contributor

Choose a reason for hiding this comment

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

PlayerConfiguration now has lcevc with the parameters and removed from constructor

}

/**
* Create canvas for LCEVC Dil to render if not exists.
Copy link
Contributor

Choose a reason for hiding this comment

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

If I could propose an alternative design, perhaps...

You could pass in the canvas, not in the player constructor, but in some setter method. setLCEVCCanvas or something. And the UI library can have a similar method of its own.

Then make the LCEVC code not make a canvas at all, and the UI library can make the default canvas if the user doesn't provide a custom on.

If the user does not use the UI library and also doesn't provide a canvas, you could throw an error or something.

Admittedly that would also require some parts of the resize logic to be exposed to the UI, so it can know the aspect ratio of the DIL and such, which seems awkward design-wise...


/** @type {!jasmine.Spy} */
this.updateLcevcDil =
jasmine.createSpy('updateLcevcDil').and.stub();
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: This has an incorrect indentation level.

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

lib/player.js Outdated
}

/**
* Close a shaka.lcevc.Dil object
Copy link
Contributor

Choose a reason for hiding this comment

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

You should mention this also hides the canvas. Could be a surprise, for a user-defined canvas.

Copy link
Contributor

Choose a reason for hiding this comment

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

changed to Close a shaka.lcevc.Dil object if present and hide the canvas.

lib/player.js Show resolved Hide resolved
lib/player.js Outdated
*/
closeDIL_() {
if (this.lcevcDil_ != null) {
this.lcevcDil_.hideCanvas();
Copy link
Contributor

Choose a reason for hiding this comment

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

The release method already hides the canvas, so you don't need to call this explicitly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Removed hideCanvas call from closeDil

@avelad avelad requested a review from theodab May 17, 2022 10:54
@vinod-balakrishnan
Copy link
Contributor

hey @joeyparrish @avelad @theodab , can you guys please review and suggest the next steps.. Thanks.

@kevleyski
Copy link

Hi @joeyparrish I'm happy to review/test this too, let me know if I can be useful to help push this through

@joeyparrish
Copy link
Member

@vinod-balakrishnan, I will re-review it today. Apologies for the delay. It is a complex PR, though, and review is difficult. We thank you for your contribution and your patience.

@vinod-balakrishnan
Copy link
Contributor

Thanks @joeyparrish

Copy link
Member

@joeyparrish joeyparrish left a comment

Choose a reason for hiding this comment

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

The biggest issue in this PR now is that you still haven't moved the canvas creation to the UI layer. That is a blocking issue for me from an architectural point of view.

lib/media/media_source_engine.js Show resolved Hide resolved
@@ -76,6 +76,7 @@ shaka.media.MediaSourceEngine = class {
number, ?number)} */
this.onMetadata_ = onMetadata || onMetadataNoOp;

/** @private {shaka.lcevc.Dil | undefined} */
Copy link
Member

Choose a reason for hiding this comment

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

It's just style at this point, but normally we would make this nullable, and set it to null when it's not given. Something like:

/** @private {?shaka.lcevc.Dil} */
this.lcevcDil_ = lcevcDil || null;


// Append only video data to the LCEVC Dil.
if (contentType == ContentType.VIDEO && this.lcevcDil_) {
if (this.lcevcDil_) {
Copy link
Member

Choose a reason for hiding this comment

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

You've already checked lcevcDil_ one line above.

Copy link
Contributor

Choose a reason for hiding this comment

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

This check is removed.

lib/player.js Outdated
Comment on lines 745 to 782
const safariVersion = shaka.util.Platform.safariVersion();
if (safariVersion) {
if (config.streaming.useNativeHlsOnSafari) {
shaka.log.alwaysWarn('LCEVC Error: Only Safari with MSE enabled ' +
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't account for the fact that native HLS is the only option on iOS. You might want a separate check and warning for iOS.

...

But now that I look at your hooks for setupLcevc_() more closely, I see that this won't even trigger. In a native-HLS playback, we'll use src= instead of StreamingEngine and MediaSourceEngine, so we'll go through onSrcEquals_ instead of onLoad_.

Copy link
Contributor

Choose a reason for hiding this comment

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

this.setupLcevc_() is now added to onSrcEquals_ too.

Copy link
Member

Choose a reason for hiding this comment

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

You can't use LCEVC with src=. Without MediaSource playback, we can't send the necessary information into the DIL.

So you don't really even need this check here. If you want to warn about LCEVC being non-functional with src=, do it inside the src= setup instead of here. There's no point in calling this from src= or checking for Safari version anywhere. You can use src= mode with plain mp4s on any browser, not just for Safari and native HLS.

lib/player.js Outdated
Comment on lines 754 to 759
const edge = shaka.util.Platform.isEdge() ||
shaka.util.Platform.isLegacyEdge();
if (edge) {
if (!config.streaming.forceTransmuxTS) {
shaka.log.alwaysWarn('LCEVC Warning: For MPEG-2 TS decoding '+
'the config.streaming.forceTransmux must be enabled.');
Copy link
Member

Choose a reason for hiding this comment

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

It's not clear to me why you are checking for forceTransmuxTS on Edge only, and without respect for whether or not the content is using TS.

Copy link
Contributor

Choose a reason for hiding this comment

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

Edge on windows does not pass the lcevc data to the lib unless this config option is enabled for ts files. Hence this needs to be checked.

To check the mime type i have added an additional check from the getVariants() function and only trigger the warning when the videoMimeType is ts.

const aspectRatio = (this.dil_ && this.dil_.aspectRatio > 0 ?
this.dil_.aspectRatio :
this.media_.videoWidth / this.media_.videoHeight) || 1920 / 1080;
this.lcevcContainer_.style.width =
Copy link
Member

Choose a reason for hiding this comment

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

When you move this code to the UI layer, you don't need to set styles with an explicit width. Instead, use a layout in CSS that overlays the video and matches 100% of the size of the video. See styles in ui/less/, specifically the way shaka-text-container and shaka-controls-container overlay the video element and fill the same space.

In fact, if you do it this way, you really don't even need to hide the video element. Making the canvas visible will simply cover up the video element.

Copy link
Contributor

Choose a reason for hiding this comment

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

Created a new shaka-canvas-container class to be added to the canvas and append it to the videocontainer.

containerFormat = shaka.lcevc.Dil.ContainerFormat.WEBM;
break;
}
case 'video/mp4': {
Copy link
Member

Choose a reason for hiding this comment

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

No case for TS? I see you have an enum defined for it.

Copy link
Contributor

Choose a reason for hiding this comment

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

TS is the default value, removed from the enum

}

/**
* Create canvas for LCEVC Dil to render if not exists.
Copy link
Member

Choose a reason for hiding this comment

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

Please drop createCanvas_() and move this functionality to the UI layer. See my new comments above at the createCanvas_() call site.

externs/lcevc.js Outdated Show resolved Hide resolved
break;
}
}
this.updateLevel_(iterator, containerFormat);
Copy link
Member

Choose a reason for hiding this comment

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

Why are you using the index into the track list? Is it just an arbitrary numerical identifier that you intend to be stable? If so, index is a bad thing to use. We have numerical track IDs on the Track object, and those are stable even if tracks get removed for some reason.

Copy link
Contributor

Choose a reason for hiding this comment

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

Changed the updateLevel to use track ids instead of index.

@avelad avelad requested a review from joeyparrish June 20, 2022 14:25
@avelad
Copy link
Member

avelad commented Jun 28, 2022

@vinod-balakrishnan there seem to be conflicts, can you resolve them and rebase? thanks!

@github-actions
Copy link
Contributor

github-actions bot commented Sep 6, 2022

Incremental code coverage: 82.75%

@vinod-balakrishnan
Copy link
Contributor

Please review your CLA, there some issues. Thanks!

@avelad The commit from account that has the error is already included in the CLA agreement for individual and company. how can we debug that.

@avelad
Copy link
Member

avelad commented Sep 6, 2022

@vinod-balakrishnan
Copy link
Contributor

@avelad @joeyparrish Sorry for all that confusion with the CLA. It's all resolved now. And ready for review. Thanks!

@avelad
Copy link
Member

avelad commented Sep 12, 2022

@joeyparrish can you review it again? Thanks!

@avelad avelad added this to the v4.3 milestone Sep 12, 2022
demo/locales/en.json Outdated Show resolved Hide resolved
Updated en.json with source.json text to match the text and arranged it alphabetically.
@avelad avelad added the priority: P3 Useful but not urgent label Sep 26, 2022
joeyparrish
joeyparrish previously approved these changes Sep 30, 2022
@joeyparrish
Copy link
Member

I'm going to run the tests one more time in our lab before merging. Thanks for your patience!

@kevleyski
Copy link

kevleyski commented Sep 30, 2022 via email

@joeyparrish
Copy link
Member

Oh, man. Another PR that landed today has created merge conflicts with this one. Please merge with main and fix conflicts, and I'll re-review merge the PR. Thanks!

@vinod-balakrishnan
Copy link
Contributor

Oh, man. Another PR that landed today has created merge conflicts with this one. Please merge with main and fix conflicts, and I'll re-review merge the PR. Thanks!

@joeyparrish I have merged the main in, to resolve the conflicts. Please take a look. Thanks.

@joeyparrish joeyparrish changed the title feat: Shaka Player - LCEVC Integration feat: LCEVC integration Oct 3, 2022
@joeyparrish joeyparrish merged commit 284ea63 into shaka-project:main Oct 3, 2022
@kevleyski
Copy link

Great thanks all!

@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
priority: P3 Useful but not urgent status: archived Archived and locked; will not be updated type: enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants