Fix Performance issue for long videos.

[aneesijaz]

Is your Pull Request request related to another issue in this repository ?

ISSUE: Performance hit for media over 1 hour #150

Describe what the PR does

Added react-visibility-sensor and modified wrapperBlock.js file to load only the visible area which can be helpful for the long videos with long transcript.

State whether the PR is ready for review or whether it needs extra work

Ready

Additional context

This is my 1st PR so please let me know if I did anything wrong and I will amend my mistake.
Thanks.

[pietrop]

Thanks soo much for looking into this @aneesijaz !

Trying out this PR, here's a few things I noticed, testing with a 2 hour video

Local storage

Not necessary related to your PR, but in the demo app in storybook, while testing saving changes to local storage causes problems

So I commented out line 10, as this can be addressed as separate issue, more to do with how the parent application deals with the data etc..

// localStorage.setItem(`draftJs-${ mediaUrlName }`, JSON.stringify(data));

Shaking

This doesn't happen all the time, but some time, while playing the text starts to shake

Typing while playing

If I start typing while it's playing, there's a delay before the text shows up. And then it gets unresponsive for a bit afterwards, did you come across this as well?

[aneesijaz]

ok so the other problem of storage exceeds is not related to this of course.
And the shaking problem is fixed. just had to remove the min height from the loadingPlaceholder.
Tested on my end and not shaking anymore (Yes it was shaking a lot before).
Let me know if there is any other issue.

[aneesijaz]

And the issue that the editor hangs is I guess also not related to this. Because I had same experience with the visibility sensor and without the visibility sensor they both worked the same (some time's a little bit lag...)

[pietrop]

Thanks for the changes @aneesijaz

yeah, the point of issue #150 was to see if things like visibility sensor could remove the lag. As well as trying to narrow down what's causing it.

[aneesijaz]

the lag for what the issue was a huge one.. and that was because the draftjs rendered block have a huge dom tree which cause memory to overload when all the transcript is loaded at the same time.
But with this visibility sensor module the only visible screen is being rendered which makes a huge difference in performance.
And for the lag I think you have not tried it after the last commit , I think it is a small lag that may be considered non-existent.
I would suggest testing it for same two hours video with and without the visibility sensor, this will show the difference more clearly.
Thank you.

[pietrop]

Awesome, thanks, yes I haven't tried after the last commit, will try and let you know as soon as I get a chance

[jamesdools]

Hi @aneesijaz, thanks for the PR!

I just tried it locally and ran into an issue.

When Scroll Sync is turned on from the options, and say you click to further in the media via the progress bar, it will give you this.

Digging into the code it's because we use the querySelector to grab the word right word - but it won't find it in the DOM.

As below, I wrapped an if (currentWordElement) to stop the breaking error, but still need it to somehow navigate the right element even though it's not visible.

    if (currentWord.start !== 'NA') {
      if (this.props.isScrollIntoViewOn) {
        const currentWordElement = document.querySelector(`span.Word[data-start="${ currentWord.start }"]`);

        if (currentWordElement) {
          currentWordElement.scrollIntoView({
            block: 'nearest',
            inline: 'center'
          });
        }
      }
    }

I'm sure there's a way with react-visibility-sensor but haven't been able to dig through the library yet. Any thoughts?

[jamesdools]

On a sidenote: in the WrapperBlock I changed "Loading" to a simple "..."

And added an offset in the props of the visibility sensor:

      <VisibilitySensor
        onChange={ this.visibilityChanged }
        containment={ containmentDOMRect }
        offset={ { top: -1200, bottom: -1200 } }
        partialVisibility
        scrollCheck
      >

I found this gives you a buffer zone of about 2 screens-worth of height (~600px for the editor)!
This means when scrolling fast through the editor, you don't see so much loading of the blocks, which to me is quite jarring.

[aneesijaz]

I will look into that scroll sync , may be we can find any other method of doing that scroll. (I was thinking of adding those attributes of timings in the loading div and then when that the page is scrolled then the loading should be automatically converted back to the wrapper object.) Sent from Mailspring (https://link.getmailspring.com/link/[email protected]/0?redirect=https%3A%2F%2Fgetmailspring.com%2F&recipient=cmVwbHkrQUZVWTNHVVk1REpWRUZINVBVWDNFSE40R1JUSEpFVkJOSEhDQTRZQ0dZQHJlcGx5LmdpdGh1Yi5jb20%3D), the best free email app for work

…

On Jan 21 2020, at 10:25 pm, James Dooley ***@***.***> wrote: On a sidenote: in the WrapperBlock I changed "Loading" to a simple "..." And added an offset in the props of the visibility sensor: <VisibilitySensor onChange={ this.visibilityChanged } containment={ containmentDOMRect } offset={ { top: -1200, bottom: -1200 } } partialVisibility scrollCheck > I found this gives you a buffer zone of about 2 screens-worth of height (~600px for the editor)! This means when scrolling fast through the editor, you don't see so much loading of the blocks, which to me is quite jarring. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub ***@***.***/1?redirect=https%3A%2F%2Fgithub.com%2Fbbc%2Freact-transcript-editor%2Fpull%2F217%3Femail_source%3Dnotifications%26email_token%3DAFUY3GRCGDAQAINRBUATCW3Q64VXJA5CNFSM4KB5ED72YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJQRVTI%23issuecomment-576789197&recipient=cmVwbHkrQUZVWTNHVVk1REpWRUZINVBVWDNFSE40R1JUSEpFVkJOSEhDQTRZQ0dZQHJlcGx5LmdpdGh1Yi5jb20%3D), or unsubscribe ***@***.***/2?redirect=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAFUY3GQPUKVANVPNFDXLOHTQ64VXJANCNFSM4KB5ED7Q&recipient=cmVwbHkrQUZVWTNHVVk1REpWRUZINVBVWDNFSE40R1JUSEpFVkJOSEhDQTRZQ0dZQHJlcGx5LmdpdGh1Yi5jb20%3D).

[pbirsinger]

Wow - would love to have this PR merged! Any blockers still?

I'm finding a pretty sizable perf hit even starting around 25 mins. Does the input stt type make any difference?

[pietrop]

Hi @pbirsinger
I think this PR is not fully solving the issue #150 so until then best to hold off.

The input stt type should not make a difference as they all get converted to draftJs json.

But yeah, help wanted in figuring out what is the exact cause of the performance issue.

[pbirsinger]

Hi @pietrop - thanks for the fast response. Do you think this PR provides any perf gain now but is buggy, or just does not significantly improve performance?

[pietrop]

When I last tried it, it didn't provide any significant improvement eg with a 2 hour video. But it was a while back, you are welcome to test it out locally, and do a giff/screen recording if you want to try it out

[pbirsinger]

Just tried it out, and still extremely laggy on longer videos.