google / openrtb-doubleclick

Utilities for DoubleClick Ad Exchange, including OpenRTB mapping, DoubleClick cryptography, metadata and validation
Apache License 2.0
198 stars 82 forks source link

Video Duration - miliseconds to seconds and wrong mapper method for start delay #70

Closed marcelomalcher closed 9 years ago

marcelomalcher commented 9 years ago

Hey @opinali ,

Notice two things related to video ads:

1) DoubleClick specify values of video min and max duration in milliseconds and OpenRtb is in seconds.

DoubleClickOpenRtbMapper.java

  protected Video.Builder buildVideo(... {
    ...
    if (dcVideo.hasMinAdDuration()) {
      video.setMinduration(dcVideo.getMinAdDuration());
    }
    if (dcVideo.hasMaxAdDuration()) {
      video.setMaxduration(dcVideo.getMaxAdDuration());
    }
    ...
}

2) Despite not having any significant collateral effect due to both specs using the same values, when mapping the video start delay, you are using the toDoubleClick method instead of toOpenRtb

DoubleClickOpenRtbMapper.java

  protected Video.Builder buildVideo(... {
    ...
    if (dcVideo.hasVideoadStartDelay()) {
      video.setStartdelay(VideoStartDelayMapper.toDoubleClick(dcVideo.getVideoadStartDelay()));
    }
    ...
}
opinali commented 9 years ago

You are right, it seems I was blind to the documentation on these units :( I guess too much used to AdX's usage of milliseconds for everything. Fixing this soon, thanks for the report again.

marcelomalcher commented 9 years ago

haha don't worry... you did a fantastic job in these libs and I am super grateful! We had our own mapper implementation but it would be really difficult to maintain the way you do. So we decided to use this lib and that's why I am noticing things here and there.

Thanks again for all the support @opinali ! :)

PS: Next time I think I will open an PR. I think it is faster and it does not give much trouble to you.

opinali commented 9 years ago

Fixed in master, please test. BTW the inverted mapping of startdelay was broken for the special values too, the special values are not all the same...

marcelomalcher commented 9 years ago

Just tested and it is okay @opinali ! Thanks again! I will close this issue and wait for the new release.